1469843 - refactor sidebar to remove webext-panel.xhtml

Status: NEW

(WebExtensions :: Frontend, enhancement, P3)

Description

[Shane Caraveo (:mixedpuppy)]

We should probably refactor the sidebar, it shouldn't be necessary to load another xul page to load another browser element.  It would probably simplify a lot of code.

Comment 1

[Shane Caraveo (:mixedpuppy)]

Attachment: Bug 1469843 WIP refactor extension sidebar support and remove webext-panels

Review commit: https://reviewboard.mozilla.org/r/255074/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/255074/

Comment 2

[Shane Caraveo (:mixedpuppy)]

Attached patch is a WIP.  It works, but:

- unlikely tests all pass right now
- devtools sidebar would be broken
- handling of different browsers for the different sidebar panels is probably non-optimal, looking for feedback here (and in general)

For the browsers/deck issue, I could just remove the browsers from browser.xul and add/remove them as necessary in browser-sidebar.js.  Otherwise we could use the deck as I've done (primarily as a shortcut while working out moving needed webext-panels logic into browser-sidebar.js).

Comment 3

[:Gijs (he/him)]

Apologies for the delay here, I will try and get to this on Monday.

Comment 4

[:Gijs (he/him)]

Comment on attachment 8990056 [details]
Bug 1469843 WIP refactor extension sidebar support and remove webext-panels

https://reviewboard.mozilla.org/r/255074/#review262538

(In reply to Shane Caraveo (:mixedpuppy) from comment #2)
> Attached patch is a WIP.  It works, but:
> 
> - unlikely tests all pass right now
> - devtools sidebar would be broken

Not sure I understand this. Why would the devtools sidebar be broken? It doesn't use the "normal" sidebar infrastructure?

> - handling of different browsers for the different sidebar panels is
> probably non-optimal, looking for feedback here (and in general)

Not sure what you mean here, either. You mean that instead of having N browsers for N sidebars we could have 1 and just load different stuff in 1 browser?

> For the browsers/deck issue, I could just remove the browsers from
> browser.xul and add/remove them as necessary in browser-sidebar.js. 
> Otherwise we could use the deck as I've done (primarily as a shortcut while
> working out moving needed webext-panels logic into browser-sidebar.js).

Hm. I guess with the deck, contents of browsers remember state (so switching sidebars doesn't un-collapse the bookmarks/history/tabs' trees, and wouldn't lose context/state in the webextension). Is that right? Do we lose that if we lose the deck?

I'd like to keep separate browsers for the webextension vs. the others because it "feels" safer - it avoids the webextension managing to priv-esc by navigating the browser to the (chrome-privileged) bookmark/history/synced-tab sidebar.

Anyway, this seems reasonable as a first step, though I'm not sure whether from this patch it's hard to go to just having browsers we add/remove...

::: browser/base/content/browser-sidebar.js:397
(Diff revision 1)
>    showInitially(commandID) {
> -    return this._show(commandID).then((sidebarBroadcaster) => {
> -      this._loadSidebarExtension(sidebarBroadcaster);
> +    return this._show(commandID);
> +  },
> +
> +  _prepareExtensionBrowser(policy, url, sidebarBroadcaster) {
> +    if (this._deck.selectedIndex === 0) {

Is there some way to check this with something other than an index, to make it more obvious what this does and less fragile in case someone messes with the contents of the panel? Maybe `deck.selectedPanel && deck.selectedPanel.id` or something?

::: browser/base/content/browser-sidebar.js:401
(Diff revision 1)
> +    // XXX
> +    ChromeUtils.import("resource://gre/modules/ExtensionUtils.jsm");

Lazy import? :-)

::: browser/base/content/browser-sidebar.js:405
(Diff revision 1)
> +      promiseEvent,
> +    } = ExtensionUtils;

Doesn't need to be here, but maybe we should move this onto PromiseUtils or onto Node or something, as a chromeonly thing... we have it in test util code as well as ExtensionUtils right now and we should just make it more widely available, I think.

::: browser/base/content/browser-sidebar.js:454
(Diff revision 1)
> +      newIndex = 1;
> +    }
> +    if (newIndex !== this._deck.selectedIndex) {
> +      // clear the current panel then switch
> +      this.browser.setAttribute("src", "about:blank");
> +      this.browser.docShell.createAboutBlankContentViewer(null);

browser.docShell is null for a remote browser, I think?

::: browser/base/content/browser-sidebar.js:475
(Diff revision 1)
> +        this.browser.loadURI(aUrl, {triggeringPrincipal});
> +        return ready;
> +      }
> +      throw new Error("moz-extension url loading in sidebar without extensionId");
> +    }
> +    // XXX we have a content url?

We removed web content sidebars, I think?

::: browser/base/content/browser-sidebar.js:524
(Diff revision 1)
>        }
>  
>        let url = sidebarBroadcaster.getAttribute("sidebarurl");
> -      this.browser.setAttribute("src", url); // kick off async load
> -
> -      if (this.browser.contentDocument.location.href != url) {
> +      // this.browser.setAttribute("src", url); // kick off async load
> +      this._setUrl(url, sidebarBroadcaster).then(() => {
> +        if (!this.browser.contentDocument ||

I would explicitly check `browser.isRemote` here.

::: browser/base/content/browser.xul:1222
(Diff revision 1)
>            <spacer flex="1000"/>
>            <toolbarbutton id="sidebar-close" class="close-icon tabbable" tooltiptext="&sidebarCloseButton.tooltip;" oncommand="SidebarUI.hide();"/>
>          </sidebarheader>
> +        <deck id="sidebar-deck" flex="1" persist="selectedIndex">
> -        <browser id="sidebar" flex="1" autoscroll="false" disablehistory="true" disablefullscreen="true"
> +          <browser id="sidebar" flex="1" autoscroll="false" disablehistory="true" disablefullscreen="true"
> -                  style="min-width: 14em; width: 18em; max-width: 36em;" tooltip="aHTMLTooltip"/>
> +                   style="min-width: 14em; width: 18em; max-width: 36em;" tooltip="aHTMLTooltip"/>

Should the style attributes migrate to the deck?

Comment 5

[:Gijs (he/him)]

I did a casual review, plus some comments about the deck stuff... let me know if you want to chat on vidyo/irc or needinfo if you have more specific questions. :-)

Comment 6

[Luca Greco [:rpl] [:luca] [:lgreco]]

(In reply to :Gijs (he/him) from comment #4)
> Comment on attachment 8990056 [details]
> Bug 1469843 WIP refactor extension sidebar support and remove webext-panels
> 
> https://reviewboard.mozilla.org/r/255074/#review262538
> 
> (In reply to Shane Caraveo (:mixedpuppy) from comment #2)
> > Attached patch is a WIP.  It works, but:
> > 
> > - unlikely tests all pass right now
> > - devtools sidebar would be broken
>
> Not sure I understand this. Why would the devtools sidebar be broken? It
> doesn't use the "normal" sidebar infrastructure?

Actually, it doesn't affect the devtools sidebar, but the devtools panels that are created from the WebExtensions API.

The devtools UI (as well as the UI elements created from a WebExtensions devtools API) does not use the browser
sidebar infrastructure (even when the devtools is docked on the side, it doesn't use that).

The only part of the WebExtensions devtools APIs that is currently using the webext-panels.xul file is the implementation of the `browser.devtools.panels.create` API method:

- https://searchfox.org/mozilla-central/rev/28daa2806c89684b3dfa4f0b551db1d099dda7c2/browser/components/extensions/parent/ext-devtools-panels.js#81-98

which is currently using the webext-panels.xul file because it needs to handle the "wiring" for the select, autocompletion and context menu popups when the extension panel is running out of process, and it had to be handled in a way that is pretty similar to what we were already doing for the extension sidebars.

Comment 7

[Shane Caraveo (:mixedpuppy)]

(In reply to :Gijs (he/him) from comment #4)
> Comment on attachment 8990056 [details]
> Bug 1469843 WIP refactor extension sidebar support and remove webext-panels

> > - handling of different browsers for the different sidebar panels is
> > probably non-optimal, looking for feedback here (and in general)
> 
> Not sure what you mean here, either. You mean that instead of having N
> browsers for N sidebars we could have 1 and just load different stuff in 1
> browser?

I'm not sure what kind of impact there might be with having the extra browser elements in browser.xul when they may never be used.  I was about to go with always adding/removing the sidebar browser, but wanted to get some input/reaction first.  

> > For the browsers/deck issue, I could just remove the browsers from
> > browser.xul and add/remove them as necessary in browser-sidebar.js. 
> > Otherwise we could use the deck as I've done (primarily as a shortcut while
> > working out moving needed webext-panels logic into browser-sidebar.js).
> 
> Hm. I guess with the deck, contents of browsers remember state (so switching
> sidebars doesn't un-collapse the bookmarks/history/tabs' trees, and wouldn't
> lose context/state in the webextension). Is that right? Do we lose that if
> we lose the deck?

I'm not really trying to change how state works (thus about:blank loading on switch), though I guess it could be a benefit in some cases if we left it loaded (e.g. tab manager sidebar extensions).  Could also be a disadvantage, e.g. high memory use in a seldom used extension sidebar.

> I'd like to keep separate browsers for the webextension vs. the others
> because it "feels" safer - it avoids the webextension managing to priv-esc
> by navigating the browser to the (chrome-privileged)
> bookmark/history/synced-tab sidebar.
> 
> Anyway, this seems reasonable as a first step, though I'm not sure whether
> from this patch it's hard to go to just having browsers we add/remove...

I don't think it would be hard to add/remove a browser instead of having the deck.  One concern may be visual glitches when adding/removing.  Could probably be solved by having a box wrapping the sidebar browser that maintains the width.

Thanks for the rest of the feedback on the patch as well, my first concern is picking an approach that

Comment 8

[:Gijs (he/him)]

(In reply to Shane Caraveo (:mixedpuppy) from comment #7)
> Thanks for the rest of the feedback on the patch as well, my first concern
> is picking an approach that

You seem to have got cut off here. :-)


One middle-way you could potentially pick here is having two browsers in the markup and initially mark both `hidden=true` and then unhide as necessary.