[Extensions] Show default extension view in side panel [chromium/src : main]

Kelvin Jiang (Gerrit)

unread,

Dec 14, 2022, 4:33:05 AM12/14/22

to Caroline Rising, Devlin Cronin, chromium-a...@chromium.org, extension...@chromium.org

Attention is currently required from: Caroline Rising, Devlin Cronin.

Kelvin Jiang would like Caroline Rising and Devlin Cronin to review this change.

[Extensions] Show default extension view in side panel

This CL adds extension views to the side panel. An extension view is
registered if the extension has specified a side panel default path in
its manifest (documented here: https://docs.google.com/document/d/1bWM4oMj-3Bzp_ixtG5qkPWESCOfwAWJRUMXavxVswIM/edit#bookmark=id.v7z2yej4f1m).

And the side panel entry exists as long as that extension is enabled.

Bug: 1378048
Change-Id: Ib1cb107cdbe14d27c63c5d8d702daa76686eca93
---
M chrome/browser/extensions/extension_view_host_factory.cc
M chrome/browser/extensions/extension_view_host_factory.h
M chrome/browser/ui/BUILD.gn
A chrome/browser/ui/views/side_panel/extensions/extension_side_panel_coordinator.cc
A chrome/browser/ui/views/side_panel/extensions/extension_side_panel_coordinator.h
A chrome/browser/ui/views/side_panel/extensions/extension_side_panel_manager.cc
A chrome/browser/ui/views/side_panel/extensions/extension_side_panel_manager.h
A chrome/browser/ui/views/side_panel/extensions/extension_side_panel_utils.cc
A chrome/browser/ui/views/side_panel/extensions/extension_side_panel_utils.h
M chrome/browser/ui/views/side_panel/side_panel_util.cc
10 files changed, 318 insertions(+), 0 deletions(-)

To view, visit change 4104269. To unsubscribe, or for help writing mail filters, visit settings.

Kelvin Jiang (Gerrit)

unread,

Dec 14, 2022, 4:35:48 AM12/14/22

to chromium-a...@chromium.org, extension...@chromium.org, Devlin Cronin, Caroline Rising, Tricium, chromium...@chromium.org

Attention is currently required from: Caroline Rising, Devlin Cronin.

View Change

4 comments:

Patchset:
- Patch Set #2:
  Still figuri
Patchset:
- Patch Set #3:
  Still figuring out tests. Could I get a review/sanity check on the implementation itself?
File chrome/browser/ui/views/side_panel/extensions/extension_side_panel_coordinator.cc:
- Patch Set #1, Line 45:
```
  global_registry->Deregister(
      SidePanelEntry::Key(SidePanelEntry::Id::kExtension, extension_->id()));
```
  This always closes the side panel if this happens when the entry for the extension is shown. I feel that it's not meant to be intentional behavior. What might I be doing wrong here?
- Patch Set #1, Line 64:
```
  host_ =
      ExtensionViewHostFactory::CreateSidePanelHost(side_panel_url, browser_);
```
  Is there a way to make the view show without having to reset the host? Perhaps I could tell the host to reload the initial url instead of recreating it?

To view, visit change 4104269. To unsubscribe, or for help writing mail filters, visit settings.

Kelvin Jiang (Gerrit)

unread,

Dec 14, 2022, 4:39:08 AM12/14/22

to chromium-a...@chromium.org, extension...@chromium.org

Attention is currently required from: Caroline Rising, Devlin Cronin.

Kelvin Jiang uploaded patch set #4 to this change.

View Change

[Extensions] Show default extension view in side panel

This CL adds extension views to the side panel. An extension view is
registered if the extension has specified a side panel default path in
its manifest (documented here: https://docs.google.com/document/d/1bWM4oMj-3Bzp_ixtG5qkPWESCOfwAWJRUMXavxVswIM/edit#bookmark=id.v7z2yej4f1m).

And the side panel entry exists as long as that extension is enabled.

Screenshots: https://imgur.com/a/Tzmhuvo



Bug: 1378048
Change-Id: Ib1cb107cdbe14d27c63c5d8d702daa76686eca93
---
M chrome/browser/extensions/extension_view_host_factory.cc
M chrome/browser/extensions/extension_view_host_factory.h
M chrome/browser/ui/BUILD.gn
A chrome/browser/ui/views/side_panel/extensions/extension_side_panel_coordinator.cc
A chrome/browser/ui/views/side_panel/extensions/extension_side_panel_coordinator.h
A chrome/browser/ui/views/side_panel/extensions/extension_side_panel_manager.cc
A chrome/browser/ui/views/side_panel/extensions/extension_side_panel_manager.h
A chrome/browser/ui/views/side_panel/extensions/extension_side_panel_utils.cc
A chrome/browser/ui/views/side_panel/extensions/extension_side_panel_utils.h
M chrome/browser/ui/views/side_panel/side_panel_util.cc

10 files changed, 320 insertions(+), 0 deletions(-)

To view, visit change 4104269. To unsubscribe, or for help writing mail filters, visit settings.

Kelvin Jiang (Gerrit)

unread,

Dec 14, 2022, 6:48:55 PM12/14/22

to chromium-a...@chromium.org, extension...@chromium.org, Chromium LUCI CQ, Devlin Cronin, Caroline Rising, Tricium, chromium...@chromium.org

Attention is currently required from: Caroline Rising, Devlin Cronin.

Patch set 5:Quick-Run +1Commit-Queue +1

View Change

2 comments:

Patchset:
- Patch Set #5:
  Tests done
File chrome/browser/ui/views/side_panel/extensions/extension_side_panel_coordinator.cc:

Patch Set #1, Line 45:


  global_registry->Deregister(
      SidePanelEntry::Key(SidePanelEntry::Id::kExtension, extension_->id()));

- This always closes the side panel if this happens when the entry for the extension is shown. […]
  Never mind, this behavior seems to be intentional, as seen in the unit test: https://source.chromium.org/chromium/chromium/src/+/main:chrome/browser/ui/views/side_panel/side_panel_coordinator_unittest.cc;l=1034-1043?q=GlobalEntryDeregisteredWhenVisible&ss=chromium

To view, visit change 4104269. To unsubscribe, or for help writing mail filters, visit settings.

Devlin Cronin (Gerrit)

unread,

Dec 14, 2022, 6:50:11 PM12/14/22

to Kelvin Jiang, chromium-a...@chromium.org, extension...@chromium.org, Chromium LUCI CQ, Devlin Cronin, Caroline Rising, Tricium, chromium...@chromium.org

Attention is currently required from: Caroline Rising.

View Change

3 comments:

Patchset:
- Patch Set #4:
  Thanks, Kelvin! I took a quick pass at this one, just looking at some of the very high-level bits (I didn't leave any comments related to code style / cleanup — I figure those can come later). Most things look to be on track!
File chrome/browser/ui/views/side_panel/extensions/extension_side_panel_coordinator.cc:
- Patch Set #1, Line 64:

  host_ =
      ExtensionViewHostFactory::CreateSidePanelHost(side_panel_url, browser_);

- Is there a way to make the view show without having to reset the host? Perhaps I could tell the host […]
  When is this called / what is the lifetime of the view, with relation to the side panel open state?

File chrome/browser/ui/views/side_panel/extensions/extension_side_panel_manager.cc:

Patch Set #4, Line 48:

  coordinators_.emplace(extension->id(),
                        std::make_unique<ExtensionSidePanelCoordinator>(
                            browser_, extension, global_registry));

Either here or in the coordinator, do we need to filter on whether the extension has a side panel?

To view, visit change 4104269. To unsubscribe, or for help writing mail filters, visit settings.

Kelvin Jiang (Gerrit)

unread,

Dec 15, 2022, 6:09:01 AM12/15/22

to chromium-a...@chromium.org, extension...@chromium.org, Chromium LUCI CQ, Devlin Cronin, Caroline Rising, Tricium, chromium...@chromium.org

Attention is currently required from: Caroline Rising, Devlin Cronin.

Patch set 6:Commit-Queue +1

View Change

2 comments:

File chrome/browser/ui/views/side_panel/extensions/extension_side_panel_coordinator.cc:

Patch Set #1, Line 64:

  host_ =
      ExtensionViewHostFactory::CreateSidePanelHost(side_panel_url, browser_);

- When is this called / what is the lifetime of the view, with relation to the side panel open state?
  This is called when the side panel for the extension is about to be shown for the first time since the side panel was opened (this includes if the side panel is opened with the extension's view as the first view).
  Closing the side panel destroys all of its cached views and its active view.
  Note: if the view is shown in the side panel (extension entry is active), then a different entry is shown, then we switch back to the extension's entry, the view is not recreated as it is cached when switching between entries.
  @cori...@chromium.org lmk if I'm missing anything?
File chrome/browser/ui/views/side_panel/extensions/extension_side_panel_manager.cc:

Patch Set #4, Line 48:

  coordinators_.emplace(extension->id(),
                        std::make_unique<ExtensionSidePanelCoordinator>(
                            browser_, extension, global_registry));

Either here or in the coordinator, do we need to filter on whether the extension has a side panel?

- We should, so it's created only if we know the extension is capable of hosting side panel content. This would involve checking for a manifest entry or the side panel API permission.

To view, visit change 4104269. To unsubscribe, or for help writing mail filters, visit settings.

Devlin Cronin (Gerrit)

unread,

Dec 15, 2022, 9:09:21 PM12/15/22

to Kelvin Jiang, chromium-a...@chromium.org, extension...@chromium.org, Chromium LUCI CQ, Devlin Cronin, Caroline Rising, Tricium, chromium...@chromium.org

Attention is currently required from: Caroline Rising, Kelvin Jiang.

View Change

14 comments:

Patchset:
- Patch Set #6:
  This looks great, Kelvin! There's a few comments here — the most notable is around using the feature switch (sorry, I should have caught that before!). But I don't think anything here will change significantly, so once Caroline's on board, feel free to start layering CLs on this one.
File chrome/browser/ui/views/side_panel/extensions/extension_side_panel_browsertest.cc:
- Patch Set #6, Line 22: .
  wrapping fail : )
- Patch Set #6, Line 38:
```
  void Reset() {
    run_loop_.reset();
    run_loop_ = std::make_unique<base::RunLoop>();
  }
```
  nitty nit: I'd prefer we just make this a one-shot waiter, don't wrap the RunLoop in a unique ptr, and instantiate a new waiter for spin. (Having a Reset() / queued mechanism is useful when the observer has some more state associated with it that's costly to recreate, but here, since it's just passing in an entry, I think it's probably fine to create new instances)
- Patch Set #6, Line 63: .get()
  nitty nit: no need for `.get()`
  (also below)
- Patch Set #6, Line 69:
```
  SidePanelRegistry* global_registry =
      BrowserView::GetBrowserViewForBrowser(browser())
          ->side_panel_coordinator()
          ->GetGlobalSidePanelRegistry();
  ASSERT_TRUE(global_registry);
```
  Would it make sense to make a custom test suite for these and have helper methods to access the common variables? (I feel like we're doing it a lot in these tests, so getting rid of a bit of the boilerplate would help readability)
- Patch Set #6, Line 173:
  This is a great start for tests! There's one left here that I'd like for *this* CL, which is ensuring that without the corresponding feature, we don't create a side panel entry (see also other comment about integrating with the feature).
  In a future CL, we should also have a few tests for different incognito extensions (split vs spanning mode), since those will be... interesting : )
File chrome/browser/ui/views/side_panel/extensions/extension_side_panel_coordinator.cc:
- Patch Set #1, Line 45:


  global_registry->Deregister(
      SidePanelEntry::Key(SidePanelEntry::Id::kExtension, extension_->id()));

- Never mind, this behavior seems to be intentional, as seen in the unit test: https://source. […]
  (deferring to @cori...@chromium.org)
File chrome/browser/ui/views/side_panel/extensions/extension_side_panel_coordinator.cc:
- Patch Set #6, Line 34:
```
  if (service) {
    auto default_options =
        service->GetOptions(*extension, /*tab_id=*/absl::nullopt);
    if (default_options.enabled.has_value() && *default_options.enabled &&
        default_options.path.has_value()) {
      CreateAndRegisterEntry(global_registry,
                             extension->GetResourceURL(*default_options.path));
    }
  }
```
  We need to make sure this functionality is gated on the side panel feature [1]. The API currently doesn't allow dynamic setting of the side panel, but it looks like we still parse the manifest key (this is evidenced by the fact that your test passes without any feature overrides : )). Since this is still very much in development, we should make sure to only integrate if the feature is on.
  [1] https://source.chromium.org/chromium/chromium/src/+/main:extensions/common/extension_features.cc;l=112;drc=d632a0e263747345842d2a01f43179d711c4a5d4
- Patch Set #6, Line 57: // TODO: get the extension icon.
  TODOs should have a bug or person
- Patch Set #6, Line 61: ui::ImageModel::FromVectorIcon(omnibox::kDinoIcon, ui::kColorIcon),
  I love the dino, but for now let's use the default extension icon : ) (the puzzle piece)
File chrome/browser/ui/views/side_panel/extensions/extension_side_panel_manager.h:
- Patch Set #6, Line 62: Browser* browser_;
  should/could this be a raw_ptr?
File chrome/browser/ui/views/side_panel/extensions/extension_side_panel_manager.cc:
- Patch Set #6, Line 51: SidePanelInfo::HasSidePanel(extension) ||
  I feel like we should always require the API permission (to even specify the manifest key). Let's do that here, and, if it's not already checked in the manifest parsing / usage, add a bug for that.
File chrome/test/data/extensions/api_test/side_panel/simple_default/manifest.json:
- Patch Set #6, Line 5: "permissions": [],
  (and add sidePanel here)
- Patch Set #6, Line 7: "background": {"service_worker": "service_worker.js"}
  no need for this, since it doesn't have any logic

To view, visit change 4104269. To unsubscribe, or for help writing mail filters, visit settings.

Caroline Rising (Gerrit)

unread,

Dec 16, 2022, 10:41:00 AM12/16/22

to Kelvin Jiang, chromium-a...@chromium.org, extension...@chromium.org, Chromium LUCI CQ, Devlin Cronin, Tricium, chromium...@chromium.org

Attention is currently required from: Devlin Cronin, Kelvin Jiang.

View Change

4 comments:

Patchset:
- Patch Set #6:
  Overall looks good! I left a few comments. Also with this change could you update tools/metrics/actions/actions.xml to have a SidePanel.Extension.Shown action now that it is being used? This gets automatically recorded for all entries here https://source.chromium.org/chromium/chromium/src/+/main:chrome/browser/ui/views/side_panel/side_panel_util.cc;l=161
File chrome/browser/ui/views/side_panel/extensions/extension_side_panel_coordinator.h:
- Patch Set #6, Line 47: Browser*
  Can this be raw_ptr<Browser>? and for the extension* below?
File chrome/browser/ui/views/side_panel/extensions/extension_side_panel_coordinator.cc:

Patch Set #1, Line 45:


  global_registry->Deregister(
      SidePanelEntry::Key(SidePanelEntry::Id::kExtension, extension_->id()));

- (deferring to @corising@chromium. […]
  Yep this is intentional but expected to not happen all that often. This is partly in an effort to not be too aggressive in showing the side panel without user intent so if the entry that a user chose to look at is deregistered we default to closing the panel. But if it is a contextual entry that is deregistered we may switch to the last seen global entry instead
File chrome/browser/ui/views/side_panel/extensions/extension_side_panel_manager.h:
- Patch Set #6, Line 59: CreateExtensionSidePanelCoordinator
  Nit: since this only creates coordinators for extensions that use the side panel should this name reflect that? like MaybeCreateExtensionSidePanelCoordinator?

To view, visit change 4104269. To unsubscribe, or for help writing mail filters, visit settings.

Kelvin Jiang (Gerrit)

unread,

Dec 19, 2022, 1:34:55 AM12/19/22

to chromium-a...@chromium.org, extension...@chromium.org, Chromium LUCI CQ, Devlin Cronin, Caroline Rising, Tricium, chromium...@chromium.org

Attention is currently required from: Caroline Rising, Devlin Cronin.

Patch set 7:Commit-Queue +1

View Change

15 comments:

File chrome/browser/ui/views/side_panel/extensions/extension_side_panel_browsertest.cc:

- Patch Set #6, Line 22: .
  wrapping fail : )

- ah I see, I'll treat `panel.` as one word

Patch Set #6, Line 38:

  void Reset() {
    run_loop_.reset();
    run_loop_ = std::make_unique<base::RunLoop>();
  }

- nitty nit: I'd prefer we just make this a one-shot waiter, don't wrap the RunLoop in a unique ptr, a […]
  done

- Patch Set #6, Line 63: .get()

- nitty nit: no need for `.get()` […]
  Done

Patch Set #6, Line 69:


  SidePanelRegistry* global_registry =
      BrowserView::GetBrowserViewForBrowser(browser())
          ->side_panel_coordinator()
          ->GetGlobalSidePanelRegistry();
  ASSERT_TRUE(global_registry);

- Would it make sense to make a custom test suite for these and have helper methods to access the comm […]
  Done

- Patch Set #6, Line 173:

- This is a great start for tests! There's one left here that I'd like for *this* CL, which is ensuri […]
  Done
File chrome/browser/ui/views/side_panel/extensions/extension_side_panel_coordinator.h:

- Patch Set #6, Line 47: Browser*
  Can this be raw_ptr<Browser>? and for the extension* below?

- yes for the browser
File chrome/browser/ui/views/side_panel/extensions/extension_side_panel_coordinator.cc:

Patch Set #1, Line 45:


  global_registry->Deregister(
      SidePanelEntry::Key(SidePanelEntry::Id::kExtension, extension_->id()));

- Yep this is intentional but expected to not happen all that often. […]
  yeah it's hard to tell user intent when the extension global panel is gone. I think closing the side panel is a good default in this case.
  (feel free to resolve this comment)
File chrome/browser/ui/views/side_panel/extensions/extension_side_panel_coordinator.cc:

Patch Set #6, Line 34:

  if (service) {
    auto default_options =
        service->GetOptions(*extension, /*tab_id=*/absl::nullopt);
    if (default_options.enabled.has_value() && *default_options.enabled &&
        default_options.path.has_value()) {
      CreateAndRegisterEntry(global_registry,
                             extension->GetResourceURL(*default_options.path));
    }
  }

- We need to make sure this functionality is gated on the side panel feature [1]. […]
  Done, see `side_panel_util.cc`, where `ExtensionSidePanelManager::GetOrCreateForBrowser` is only called (i.e. ExtensionSidePanelManager only exists) if the kExtensionSidePanelIntegration feature is enabled.

- Patch Set #6, Line 57: // TODO: get the extension icon.
  TODOs should have a bug or person

- Done

- Patch Set #6, Line 61: ui::ImageModel::FromVectorIcon(omnibox::kDinoIcon, ui::kColorIcon),
  I love the dino, but for now let's use the default extension icon : ) (the puzzle piece)

- Done
File chrome/browser/ui/views/side_panel/extensions/extension_side_panel_manager.h:

- Patch Set #6, Line 59: CreateExtensionSidePanelCoordinator

- Nit: since this only creates coordinators for extensions that use the side panel should this name re […]
  Done

- Patch Set #6, Line 62: Browser* browser_;
  should/could this be a raw_ptr?

- yes
File chrome/browser/ui/views/side_panel/extensions/extension_side_panel_manager.cc:

- Patch Set #6, Line 51: SidePanelInfo::HasSidePanel(extension) ||

- I feel like we should always require the API permission (to even specify the manifest key). […]
  Done, and filed crbug.com/1402162
File chrome/test/data/extensions/api_test/side_panel/simple_default/manifest.json:

- Patch Set #6, Line 5: "permissions": [],
  (and add sidePanel here)

- Done

- Patch Set #6, Line 7: "background": {"service_worker": "service_worker.js"}
  no need for this, since it doesn't have any logic

- Done

To view, visit change 4104269. To unsubscribe, or for help writing mail filters, visit settings.

Devlin Cronin (Gerrit)

unread,

Dec 19, 2022, 8:44:02 PM12/19/22

to Kelvin Jiang, chromium-a...@chromium.org, extension...@chromium.org, Devlin Cronin, Chromium LUCI CQ, Caroline Rising, Tricium, chromium...@chromium.org

Attention is currently required from: Caroline Rising, Kelvin Jiang.

Patch set 7:Code-Review +1

View Change

7 comments:

Patchset:
- Patch Set #7:
  Rad! LGTM; thanks, Kelvin!
File chrome/browser/extensions/extension_view_host_factory.cc:
- Patch Set #7, Line 125: DCHECK(browser);
  let's throw a DCHECK(base::FeatureList::IsEnabled(<feature>)) here
File chrome/browser/ui/views/side_panel/extensions/extension_side_panel_browsertest.cc:
- Patch Set #7, Line 86: IN_PROC_BROWSER_TEST_P
  nit: I think it's a little misleading to have so many parameterized tests that are only parameterized with a single value (in reading this, I originally expected this to be running with both enabled and disabled, and was confused on how it was passing).
  Since these are mutually-exclusive test cases, let's just make separate test classes for them (i.e., an ExtensionSidePanelBrowserTest, where the feature is always enabled, and then an ExtensionSidePanelFeatureDisabledBrowserTest [feel free to rename] where it's always disabled)
- Patch Set #7, Line 140: ..
  one period is probably enough ; )
File chrome/browser/ui/views/side_panel/extensions/extension_side_panel_coordinator.cc:
- Patch Set #7, Line 31: : browser_(browser), extension_(extension) {
  and a DCHECK in this ctor
- Patch Set #7, Line 64: base::Unretained(this)
  nit: document why this base::Unretained is safe (although with a case like this, I also wouldn't mind just using a weak ptr...)
File chrome/browser/ui/views/side_panel/side_panel_util.cc:
- Patch Set #7, Line 16: #include "chrome/browser/ui/views/side_panel/extensions/extension_side_panel_manager.h"
  these should probably all go behind a BUILDFLAG(ENABLE_EXTENSIONS).
  (Both the includes here and at the usage on line 108)

To view, visit change 4104269. To unsubscribe, or for help writing mail filters, visit settings.

Kelvin Jiang (Gerrit)

unread,

Dec 20, 2022, 1:17:24 AM12/20/22

to chromium-a...@chromium.org, extension...@chromium.org, Devlin Cronin, Chromium LUCI CQ, Caroline Rising, Tricium, chromium...@chromium.org

Attention is currently required from: Caroline Rising.

View Change

9 comments:

Patchset:
- Patch Set #8:
  Over to you Caroline!
File chrome/browser/extensions/extension_view_host_factory.cc:

- Patch Set #7, Line 125: DCHECK(browser);
  let's throw a DCHECK(base::FeatureList::IsEnabled(<feature>)) here

- Done
File chrome/browser/ui/views/side_panel/extensions/extension_side_panel_browsertest.cc:

- Patch Set #7, Line 86: IN_PROC_BROWSER_TEST_P

- nit: I think it's a little misleading to have so many parameterized tests that are only parameterize […]
  Done

- Patch Set #7, Line 140: ..
  one period is probably enough ; )

- Done
File chrome/browser/ui/views/side_panel/extensions/extension_side_panel_coordinator.cc:

Patch Set #1, Line 45:


  global_registry->Deregister(
      SidePanelEntry::Key(SidePanelEntry::Id::kExtension, extension_->id()));

- yeah it's hard to tell user intent when the extension global panel is gone. […]
  (resolving)
File chrome/browser/ui/views/side_panel/extensions/extension_side_panel_coordinator.cc:

- Patch Set #7, Line 31: : browser_(browser), extension_(extension) {
  and a DCHECK in this ctor

- Done

- Patch Set #7, Line 64: base::Unretained(this)

- nit: document why this base::Unretained is safe (although with a case like this, I also wouldn't min […]
  Done: CreateView() can only be called when the SidePanelEntry exists/is registered for the extension, and whenever `this` gets destroyed, it always deregisters the SidePanelEntry, so there's no risk of CreateView() being called at that point
File chrome/browser/ui/views/side_panel/extensions/extension_side_panel_coordinator.cc:
- Patch Set #8, Line 50:
```
  if (!global_registry) {
    return;
  }
```
  huh git cl format enforces this now
File chrome/browser/ui/views/side_panel/side_panel_util.cc:

- Patch Set #7, Line 16: #include "chrome/browser/ui/views/side_panel/extensions/extension_side_panel_manager.h"

- these should probably all go behind a BUILDFLAG(ENABLE_EXTENSIONS). […]
  Done

To view, visit change 4104269. To unsubscribe, or for help writing mail filters, visit settings.

Caroline Rising (Gerrit)

unread,

Dec 20, 2022, 3:27:50 PM12/20/22

to Kelvin Jiang, chromium-a...@chromium.org, extension...@chromium.org, Devlin Cronin, Chromium LUCI CQ, Tricium, chromium...@chromium.org

Attention is currently required from: Kelvin Jiang.

Patch set 8:Code-Review +1

View Change

1 comment:

Patchset:
- Patch Set #8:
  lgtm!

To view, visit change 4104269. To unsubscribe, or for help writing mail filters, visit settings.

Kelvin Jiang (Gerrit)

unread,

Dec 20, 2022, 3:32:32 PM12/20/22

to chromium-a...@chromium.org, extension...@chromium.org, Caroline Rising, Devlin Cronin, Chromium LUCI CQ, Tricium, chromium...@chromium.org

Attention is currently required from: Kelvin Jiang.

Patch set 8:Commit-Queue +2

View Change

To view, visit change 4104269. To unsubscribe, or for help writing mail filters, visit settings.

Chromium LUCI CQ (Gerrit)

unread,

Dec 20, 2022, 4:06:12 PM12/20/22

to Kelvin Jiang, chromium-a...@chromium.org, extension...@chromium.org, Caroline Rising, Devlin Cronin, Tricium, chromium...@chromium.org

Chromium LUCI CQ submitted this change.

View Change

Approvals:
  Kelvin Jiang: Commit
  Devlin Cronin: Looks good to me
  Caroline Rising: Looks good to me

[Extensions] Show default extension view in side panel

This CL adds extension views to the side panel. An extension view is
registered if the extension has specified a side panel default path in
its manifest (documented here: https://docs.google.com/document/d/1bWM4oMj-3Bzp_ixtG5qkPWESCOfwAWJRUMXavxVswIM/edit#bookmark=id.v7z2yej4f1m).

And the side panel entry exists as long as that extension is enabled.

Screenshots: https://imgur.com/a/Tzmhuvo

Bug: 1378048
Change-Id: Ib1cb107cdbe14d27c63c5d8d702daa76686eca93

Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4104269
Reviewed-by: Devlin Cronin <rdevlin...@chromium.org>
Reviewed-by: Caroline Rising <cori...@chromium.org>
Commit-Queue: Kelvin Jiang <kelvi...@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1085629}


---
M chrome/browser/extensions/extension_view_host_factory.cc
M chrome/browser/extensions/extension_view_host_factory.h
M chrome/browser/ui/BUILD.gn

A chrome/browser/ui/views/side_panel/extensions/extension_side_panel_browsertest.cc


A chrome/browser/ui/views/side_panel/extensions/extension_side_panel_coordinator.cc
A chrome/browser/ui/views/side_panel/extensions/extension_side_panel_coordinator.h
A chrome/browser/ui/views/side_panel/extensions/extension_side_panel_manager.cc
A chrome/browser/ui/views/side_panel/extensions/extension_side_panel_manager.h
A chrome/browser/ui/views/side_panel/extensions/extension_side_panel_utils.cc
A chrome/browser/ui/views/side_panel/extensions/extension_side_panel_utils.h
M chrome/browser/ui/views/side_panel/side_panel_util.cc

M chrome/test/BUILD.gn
A chrome/test/data/extensions/api_test/side_panel/simple_default/default_path.html
A chrome/test/data/extensions/api_test/side_panel/simple_default/manifest.json
A chrome/test/data/extensions/api_test/side_panel/simple_default/side_panel_script.js
15 files changed, 605 insertions(+), 0 deletions(-)

To view, visit change 4104269. To unsubscribe, or for help writing mail filters, visit settings.

Johann Koenig (Gerrit)

unread,

Dec 20, 2022, 6:46:49 PM12/20/22

to Chromium LUCI CQ, Kelvin Jiang, chromium-a...@chromium.org, extension...@chromium.org, Caroline Rising, Devlin Cronin, Tricium, chromium...@chromium.org

Johann Koenig has created a revert of this change.

View Change

To view, visit change 4104269. To unsubscribe, or for help writing mail filters, visit settings.

Reply all

Reply to author

Forward