1208596 - Implement sidebar extension point

Status: RESOLVED FIXED

(WebExtensions :: Frontend, enhancement, P3)

Description

[Andrew Sutherland [:asuth] (he/him)]

For vertical-tab style extensions and other cool things (vertical cats telling you to hang in there?), :bwinton has indicated that the plan is to implement the opr.sidebarAction extension point documented at https://dev.opera.com/extensions/sidebarAction.html.

Comment 1

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

I don't think we've decided exactly decided what to implement. Google has proposed a separate sidebar API that I think is a bit nicer from a UI perspective (it doesn't require you to always have a bar on the side of the screen as Opera's does).

Comment 2

[Andrew Sutherland [:asuth] (he/him)]

Okie-dokie, making the bug more generic.

Some brief research reveals that https://code.google.com/p/chromium/issues/detail?id=477424 seems to be the currently proposed chrome.sidebar API, with https://code.google.com/p/chromium/issues/detail?id=477424#c68 having the links to the current code review discussions.  The more interesting google doc on the proposed implementation is https://docs.google.com/document/d/18Tr8DPj9wh-BPZyfSa7ssFLuMJ3uGGhb9tDVwyIbNV4 which is linked to from the first google doc.

The implementation is indeed extremely minimal.  Instead of having a toolbar button trigger a popup, it instead is able to request being displayed in the sidebar by extending the browserAction extension point (https://developer.chrome.com/extensions/browserAction) by including an optional "open_in_sidebar" flag in the manifest:
  "browser_action": {
    "open_in_sidebar": true
  }

They currently spec the sidebar to always be on the right for LTR languages and on the left for RTL languages.  (More details are found at the "Proposed extension component" section of the Google Doc.)

If you're talking about a different API, if you could provide a link, that would be great.

Note that the on-demand sidebar-as-popup does seem desirable for many uses-cases as described by the "Chrome Sidebar PRD" google doc.  But for some use-cases like vertical-tabs, Firefox might want to allow an extension to be persistently displayed as a separate, persistent sidebar on the left-side of the screen.

Comment 3

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

That's the proposal I meant. I think there was a lot of pushback about only allowing sidebars on the right. One option would be "right side for persistent sidebars, left for transient sidebars, and maybe reverse it for RTL languages". This is really a UX decision though. CCing Blake.

Comment 4

[Blake Winton (:bwinton) (:☕️)]

Perhaps we could extend the API to allow the user (or developer, I suppose) to decide which side to put the sidebar on?  I know that Hello is hoping to have a sidebar on the right, but Vertical Tabs wants theirs on the left…

Since it's a UX decision, we should also add Markus, who is also thinking about this kind of thing.

Comment 5

[Markus Jaritz [:designakt] (UX)]

Thanks Blake for looping me in. I am working on understanding sidebars and currently see multiple ways to implement. (As all browsers out there have slightly different ways.) 
My thoughts on the topic can be seen here: https://firefox-ux.etherpad-mozilla.org/sidebars
Will update as soon as I have a first stable concept. (Currently I favor a model like chrome, because it more easily allows for sidebars on both sides.)

Comment 6

[Sören Hentzschel]

(In reply to Bill McCloskey (:billm) from comment #1)
> Google has proposed a separate sidebar API that I think is a bit nicer from a UI
> perspective (it doesn't require you to always have a bar on the side of the
> screen as Opera's does).

Google has WONTFIXED the proposal:
https://code.google.com/p/chromium/issues/detail?id=477424#c82

I don't know what it means for Mozilla and WebExtensions, just for information.

Comment 7

[YUKI "Piro" Hiroshi]

I think it is good that this feature is landed and Firefox's sidebar panels are re-implemented based on it, with new "about:" URIs like "about:bookmarks-sidebar", "about:history-sidebar", and so on. Then meta-addons around the sidebar will be implemented easily with simple iframes. For example:
https://addons.mozilla.org/firefox/addon/all-in-one-sidebar/
https://addons.mozilla.org/firefox/addon/unified-sidebar/
https://addons.mozilla.org/firefox/addon/2-pane-bookmarks/

Comment 8

[Andy McKay]

I think we should do this. As Piro says in comment 7, this would give developers a chance to have something in the UI that's persistent and sticking around. Sidebars do get tricky though, this might become a big bug.

Comment 9

[Andy McKay]

Here is the Opera API: https://dev.opera.com/extensions/sidebar-action-manual/

Comment 10

[Shane Caraveo (:mixedpuppy)]

Thinking about this today and reading through stuff here.  Unfortunately the etherpad from comment 5 is not loading.  I went through quite a lot of iterations with the socialapi sidebar, my thoughts here are derived from that experience.  

The simplest path to having a sidebar is using what already exists, the webpanel sidebar[1], and reusing the opera api where it makes sense.  There is a toolbar button in customization that provides a list of sidebars (excluding aforementioned bookmark feature), as well as the view->sidebars menu.  Those are sufficient for an initial api implementation, ui could be updated after ux looking into this deeper.

Some tidbits from the old socialapi sidebar;

- favicon was supported in titlebar
- sidebar selection list was in the titlebar
- on install [new addon with sidebar], sidebar is opened and loaded
- addons cannot force their sidebar to be loaded and viewed, nor can they close the sidebar


[1]This is only (afaik) available by creating a bookmark, editing its settings, and choosing to "open in sidebar".  That's a feature probably used by all of 2 people (maybe more).

Comment 11

[Shane Caraveo (:mixedpuppy)]

Attachment: proof of concept sidebarAPI

Here's the basic api implementation as a proof of concept.  There are things I haven't tested, things I don't like (ie. changes in web-panels.js).  It is derived from browserAction with stuff removed that wasn't necessary or usable at this time.

Feedback/thoughts welcome.

Comment 12

[Shane Caraveo (:mixedpuppy)]

Basic proposal here is:

- follow opera api for some cross-browser compat
- use existing infrastructure to land API (only start/left-side sidebar)
  - should we auto-open sidebars on addon install?
  - should we auto-move the sidebar toolbar button into view on install of a sidebar addon?
- allow UX to follow up with new ui designs if desired
- see how the addon community uses the sidebar to decide if more features are necessary

I'll post a couple screenshots to show existing sidebar use.

Opinions/Thoughts?

Comment 13

[Shane Caraveo (:mixedpuppy)]

Attachment: sidebar.png

Comment 14

[Shane Caraveo (:mixedpuppy)]

Attachment: toolbarbutton.png

Comment 15

[Andy McKay]

(In reply to Shane Caraveo (:mixedpuppy) from comment #12)
> Basic proposal here is:
> 
> - follow opera api for some cross-browser compat

+1 and its a reasonable start to an API too.

> - use existing infrastructure to land API (only start/left-side sidebar)
>   - should we auto-open sidebars on addon 
>   - should we auto-move the sidebar toolbar button into view on install of a
> sidebar addon?

Assuming that there's a method to show the side bar, we could let the add-on developer trigger this from one of many ways, eg: runtime.onInstalled or a browserAction etc.

> - allow UX to follow up with new ui designs if desired
> - see how the addon community uses the sidebar to decide if more features
> are necessary
> 
> I'll post a couple screenshots to show existing sidebar use.
> 
> Opinions/Thoughts?

Do we need to label anything in the sidebar indicating the add-on it came from. I'm a big believer in showing end users that UI elements come from an add-on so they don't think its Firefox.

A user can still only have one sidebar open at a time, so they can have this or history or bookmarks right?

I'm all for making a quick start, getting something useable and iterating in ways that should be transparent and backwards compatible. This seems like it checks those boxes.

Comment 16

[Pete]

Great to see the progress on this, thanks.

>addons cannot force their sidebar to be loaded and viewed

That sounds fine for simple sidebar addons, but may be too restrictive for more complex cases.

For example: if an addon has a toolbar button popup, it would be useful to allow the user to toggle (or at least open) the sidebar from there too.

Comment 17

[Shane Caraveo (:mixedpuppy)]

(In reply to Pete from comment #16)
> Great to see the progress on this, thanks.
> 
> >addons cannot force their sidebar to be loaded and viewed
> 
> That sounds fine for simple sidebar addons, but may be too restrictive for
> more complex cases.
> 
> For example: if an addon has a toolbar button popup, it would be useful to
> allow the user to toggle (or at least open) the sidebar from there too.

Excellent point, and I think we can allow that via browserAction.onClicked or other user initiated actions (clicking on link in panel content).  My intention with that comment is that an addon cannot rely on the sidebar being opened, and cannot control/force that without user interaction.

Comment 18

[Shane Caraveo (:mixedpuppy)]

(In reply to Andy McKay [:andym] from comment #15)
> (In reply to Shane Caraveo (:mixedpuppy) from comment #12)
> > - use existing infrastructure to land API (only start/left-side sidebar)
> >   - should we auto-open sidebars on addon 
> >   - should we auto-move the sidebar toolbar button into view on install of a
> > sidebar addon?
> 
> Assuming that there's a method to show the side bar, we could let the add-on
> developer trigger this from one of many ways, eg: runtime.onInstalled or a
> browserAction etc.

One of the things we did in SocialAPI was to open sidebars on install so users were aware that a sidebar was available.  It may not be as important in web extensions, but it was certainly useful, especially if the provider only had a sidebar.

> > Opinions/Thoughts?
> 
> Do we need to label anything in the sidebar indicating the add-on it came
> from. I'm a big believer in showing end users that UI elements come from an
> add-on so they don't think its Firefox.

Right now the addon name or page title is used, and an API is available to change the title.  I think we have a similar problem in the url bar, moz-extension doesn't help identify what addon it actually is.  IMO a ui solution is needed in both cases.

> A user can still only have one sidebar open at a time, so they can have this
> or history or bookmarks right?

Right.  This is not changing anything about how sidebars in Firefox are implemented.

> I'm all for making a quick start, getting something useable and iterating in
> ways that should be transparent and backwards compatible. This seems like it
> checks those boxes.

That's the approach I'd like to take.

Comment 19

[Andy McKay]

(In reply to Shane Caraveo (:mixedpuppy) from comment #18)
> One of the things we did in SocialAPI was to open sidebars on install so
> users were aware that a sidebar was available.  It may not be as important
> in web extensions, but it was certainly useful, especially if the provider
> only had a sidebar.

Long term, one of my focuses is to get a good vertical tabs solution and that would require hiding the horizontal tab strip. In that case you'd really want to show the side bar containing the tabs...

But this is the sort of thing that we can file in follow on bugs.

> Right now the addon name or page title is used, and an API is available to
> change the title.  I think we have a similar problem in the url bar,
> moz-extension doesn't help identify what addon it actually is.  IMO a ui
> solution is needed in both cases.

We've got a series of bugs on that, so it should be consistent in windows (bug 1296365), url bar (bug 1266012), alerts (bug 1318114) and so on. We should apply the same here.

Comment 20

[Markus Jaritz [:designakt] (UX)]

(In reply to Shane Caraveo (:mixedpuppy) from comment #17)
> Excellent point, and I think we can allow that via browserAction.onClicked
> or other user initiated actions (clicking on link in panel content).

Does this mean that an extension can have a sidebar and a browserAction?
Looking at Opera it seams like an extension can either have a sidebar or a browserAction, but not both. Which helps to ensure that users know what to expect as most users understand extensions as icons added to the UI. And having 2 UIs for one extension might mess with that.

Comment 21

[Markus Jaritz [:designakt] (UX)]

Attachment: Sidebar_ExtensionInstallConfirmation.PNG

(In reply to Shane Caraveo (:mixedpuppy) from comment #12)
> - use existing infrastructure to land API (only start/left-side sidebar)
>   - should we auto-open sidebars on addon install?
>   - should we auto-move the sidebar toolbar button into view on install of a

Doing both will result in people probably not noticing the added sidebar toolbar button, which defeats the purpose of adding it as people now know of the sidebar, but after closing it do not know how to get it back
.
Thinking about the installation experience makes me wonder how we best introduce sidebar extensions to users: If we use the sidebar toolbar button as the main method of opening closed sidebar, we should make sure to introduce it to users after the install of a sidebar extension. Alternatively we could use a toolbar button to trigger opening the sidebar and introduce that to users. (and additionally list the sidebar in the sidebars button)

Comment 22

[Shane Caraveo (:mixedpuppy)]

(In reply to Markus Jaritz [:designakt prev :maritz] (UX) from comment #20)
> (In reply to Shane Caraveo (:mixedpuppy) from comment #17)
> > Excellent point, and I think we can allow that via browserAction.onClicked
> > or other user initiated actions (clicking on link in panel content).
> 
> Does this mean that an extension can have a sidebar and a browserAction?
> Looking at Opera it seams like an extension can either have a sidebar or a
> browserAction, but not both. Which helps to ensure that users know what to
> expect as most users understand extensions as icons added to the UI. And
> having 2 UIs for one extension might mess with that.

Where do you see that limitation?  I've copied the opera api for this.

Comment 23

[Markus Jaritz [:designakt] (UX)]

(In reply to Shane Caraveo (:mixedpuppy) from comment #22)
> Where do you see that limitation?  I've copied the opera api for this.

My mistake, there is no limitation to that.
I looked through opera extensions and did not find any that used both, therefor I assumed it is a limitation. But turns out it is not.

Comment 24

[Shane Caraveo (:mixedpuppy)]

Attachment: WIP sidebarAPI

This is a fairly complete implementation of the webext sidebar, but still a few things to do.  I'd like to get feedback on a number of items.  f?Gijs for a couple CUI bits.

TODO and/or followups:
- tests.  I've an extension I've used to manually test
- Sidebar button management (ie. auto placement into toolbar only on first time, rtl support)
- Sidebar button door hanger per comment 21
- sidebar open api
- Sidebar button css is not quite right (no padding right of image, image size is not constrained)
- Icon does not appear in main menu View->Sidebars

Items of note for feedback:
- "style" is now copied in CustomizableUI when copying menus
- reason is passed through for extension shutdown event
- sidebar startup is further delayed to allow extensions time to startup and initialize, allowing sidebars to be restored on startup.
- webext-panels is nearly identical to web-panels (used by bookmarks), done to ensure bookmark sidebars do not get any webext treatment.
- SidebarAction API started [literally] as a copy of BrowserAction as that is what sidebar was based on.
- Not implementing badge support since we don't have a sufficient icon for sidebars to support that.

Comment 25

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Generally this seems fine to me. Can we keep it on Nightly/Aurora for a few releases to avoid getting locked into a specific API before it's ready?

Comment 26

[Shane Caraveo (:mixedpuppy)]

(In reply to Bill McCloskey (:billm) from comment #25)
> Generally this seems fine to me. Can we keep it on Nightly/Aurora for a few
> releases to avoid getting locked into a specific API before it's ready?

We can, but the basic goal was to be compatible with opera, the only other browser implementing a sidebar api.

Comment 27

[:Gijs (he/him)]

(In reply to Shane Caraveo (:mixedpuppy) from comment #26)
> (In reply to Bill McCloskey (:billm) from comment #25)
> > Generally this seems fine to me. Can we keep it on Nightly/Aurora for a few
> > releases to avoid getting locked into a specific API before it's ready?
> 
> We can, but the basic goal was to be compatible with opera, the only other
> browser implementing a sidebar api.

Do we know how many add-ons are using that API and/or have we gotten feedback from any authors?


My apologies for not having gotten to this yet, I blame the avalanche of bugmail that awaited me when I got back to work yesterday. I hope to do it later today or perhaps tomorrow.

Comment 28

[Perry Wagle]

Now that I have a sidebar to play with, I can start trying to make progress porting my bookmark tag query extension.  I don't know yet what I need, but I'll find out now.  I will need to either reroute the existing bookmark browseraction's (the star, etc) to my version/overlay, or to replace them with my own.  So I'd like not to get too rigid about not allowing me to do both sidebar and browseraction.

Basically, I want to replace bookmarks with something better.  My extension was clobbered in June 2016, and I have until March 2017 to get something going outside of ESR45, but I think it will take me 1 to 2 years to get something worthy to replace bookmarks with.  In the meantime I want to be an extension that can grab all the hooks the bookmarks subsystem does.  We'll see how that goes.  8/

Comment 29

[Shane Caraveo (:mixedpuppy)]

(In reply to Perry Wagle from comment #28)
> So I'd like not to get too rigid about not allowing me to do
> both sidebar and browseraction.

This allows for both to exist.

Comment 30

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

(In reply to Shane Caraveo (:mixedpuppy) from comment #26)
> (In reply to Bill McCloskey (:billm) from comment #25)
> > Generally this seems fine to me. Can we keep it on Nightly/Aurora for a few
> > releases to avoid getting locked into a specific API before it's ready?
> 
> We can, but the basic goal was to be compatible with opera, the only other
> browser implementing a sidebar api.

Compatibility can mean a lot of things. The Opera API is pretty simple and uncontroversial. However, there are a lot of UX issues that we might change as things evolve. Changing the UX probably won't completely break an extension, but it could make it much less usable. I just don't want to get locked into a particular set of choices about where, when, or how the sidebar should appear simply because we started with something that works for add-on X and changing things will break X.

Comment 31

[Shane Caraveo (:mixedpuppy)]

(In reply to :Gijs from comment #27)
> (In reply to Shane Caraveo (:mixedpuppy) from comment #26)
> > (In reply to Bill McCloskey (:billm) from comment #25)
> > > Generally this seems fine to me. Can we keep it on Nightly/Aurora for a few
> > > releases to avoid getting locked into a specific API before it's ready?
> > 
> > We can, but the basic goal was to be compatible with opera, the only other
> > browser implementing a sidebar api.
> 
> Do we know how many add-ons are using that API and/or have we gotten
> feedback from any authors?

Opera seems to have 59 sidebar addons right now.  AMO has over 1200 (not webextensions of course).  

There is only minimal input on sidebars [two or three people], however, as far as the api goes, what is here is the minimal necessary to be able to setup a sidebar at all [including support for tab specific sidebar pages].  It makes use of the current sidebar support in Firefox and makes no assumption on what the sidebar UX/UI is (other than there is a sidebar).  The moving of the sidebar button into the toolbar [assuming we choose to do that], or the panel described in comment 21 have not affect on the api itself, those are just moving parts withing Firefox.  There is minimal risk of this level of implementation being wrong, but the benefit of being compatible with another browser. I'd expect feature requests along the lines of being able to open the sidebar [e.g. browser.sidebarAction.open] and being able to place it on the other side.  The later will definitely have to wait.

> My apologies for not having gotten to this yet, I blame the avalanche of
> bugmail that awaited me when I got back to work yesterday. I hope to do it
> later today or perhaps tomorrow.

You mean you weren't waiting to pounce on my feedback requests over the holidays?  slacker. :)

Comment 32

[Shane Caraveo (:mixedpuppy)]

(In reply to Bill McCloskey (:billm) from comment #30)
> (In reply to Shane Caraveo (:mixedpuppy) from comment #26)
> > (In reply to Bill McCloskey (:billm) from comment #25)
> > > Generally this seems fine to me. Can we keep it on Nightly/Aurora for a few
> > > releases to avoid getting locked into a specific API before it's ready?
> > 
> > We can, but the basic goal was to be compatible with opera, the only other
> > browser implementing a sidebar api.
> 
> Compatibility can mean a lot of things. The Opera API is pretty simple and
> uncontroversial. However, there are a lot of UX issues that we might change
> as things evolve. Changing the UX probably won't completely break an
> extension, but it could make it much less usable. I just don't want to get
> locked into a particular set of choices about where, when, or how the
> sidebar should appear simply because we started with something that works
> for add-on X and changing things will break X.

I fully expect (and hope) that the sidebars would get a UX overhaul.  I don't see any way a change from that perspective would break sidebars or the ability to continue using this api, unless an overhaul were to remove sidebars completely.

Again, we can keep this on nightly/aurora for a while, I just don't see anything in this part of the api that would need to change for basic sidebar support.

Comment 33

[:Gijs (he/him)]

Comment on attachment 8822788 [details] [diff] [review]
WIP sidebarAPI

Review of attachment 8822788 [details] [diff] [review]:
-----------------------------------------------------------------

I would really prefer to have 1 instance of the web browser sidebar pane if possible. Otherwise this looks not unreasonable. More detailed feedback below.

Pretty please can we have the next revision in mozreview so interdiffs will work?

::: browser/base/content/browser-sidebar.js
@@ +106,5 @@
> +      let commandID = this._box.getAttribute("sidebarcommand");
> +      if (commandID) {
> +        let command = document.getElementById(commandID);
> +        if (command) {
> +          this._delayedLoad = true;

It doesn't really make sense to keep this bool named _delayedLoad. At first I also thought this was a multiple-initialization guard (which it isn't). Can we rename it to something that describes what we're checking/waiting for, specifically?

@@ +244,5 @@
>  
>            // Run the original function for backwards compatibility.
>            sidebarOnLoad(event);
>  
> +          setTimeout(resolve, 0);

Why this change? Can you add a comment with details about why we can't resolve immediately?

::: browser/base/content/webext-panels.js
@@ +1,4 @@
> +/* -*- indent-tabs-mode: nil; js-indent-level: 4 -*- */
> +/* This Source Code Form is subject to the terms of the Mozilla Public
> + * License, v. 2.0. If a copy of the MPL was not distributed with this
> + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */

Hohum about calling these "panels"... Looking around, it looks like this is a clone of the web-panels thing we already have. Why can't we reuse that for the extension sidebars? AIUI the panels loaded by webextensions don't have any particular privileges / APIs available to them. If that is not the case, as long as they are always on distinctive origins, it should be possible to have the webextensions APIs available / not available as necessary. We shouldn't need a completely separate XUL file and JS for that.


If we really cannot unify these (though tbh, even if we could...), can we move all of it into a "sidebars" directory or something? Less clutter in browser/base/content/, and more organization, would be nice. :-)

@@ +3,5 @@
> + * License, v. 2.0. If a copy of the MPL was not distributed with this
> + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
> +
> +XPCOMUtils.defineLazyModuleGetter(this, "ExtensionParent",
> +                              "resource://gre/modules/ExtensionParent.jsm");

indenting

::: browser/base/content/webext-panels.xul
@@ +1,1 @@
> +<?xml version="1.0"?>

I'm assuming this is a modified copy of the web-panels one - can you use hg cp so it's easier to see the difference, if we really end up duplicating this?

::: browser/components/extensions/ext-sidebarAction.js
@@ +12,5 @@
> +
> +const XUL_NS = "http://www.mozilla.org/keymaster/gatekeeper/there.is.only.xul";
> +
> +// WeakMap[Extension -> SidebarAction]
> +var sidebarActionMap = new WeakMap();

let?

@@ +23,5 @@
> +  // Add the extension to the sidebar menu.  The sidebar widget will copy
> +  // from that when it is viewed, so we shouldn't need to update that.
> +  let widgetId = makeWidgetId(extension.id);
> +  this.id = `${widgetId}-sidebar-action`;
> +  this.menuId = `menu-${this.id}`;

This has the potential of conflicting with existing items, right, if the add-on id matches any of the builtin items? Can we make more sure it is unique, including 'webext' in the name or something? Also, convention for menu items is using _ rather than -.

@@ +31,5 @@
> +  this.defaults = {
> +    enabled: true,
> +    title: options.default_title || extension.name,
> +    badgeText: "",
> +    badgeBackgroundColor: null,

What are the badge bits used for in this context, or is this a copy/paste oversight?

@@ +46,5 @@
> +    // If the sidebar button has never been moved to the toolbar, move it now
> +    // so the user can see/access the sidebars.
> +    let widget = CustomizableUI.getWidget("sidebar-button");
> +    // let placement = CustomizableUI.getPlacementOfWidget("sidebar-button", false, true);
> +    if (!widget.areaType) {

This will make the button impossible to completely remove (back to the palette) while the add-on is installed, because this code will always move it back. It should keep track of whether we've moved the button before using a pref or something similar.

@@ +48,5 @@
> +    let widget = CustomizableUI.getWidget("sidebar-button");
> +    // let placement = CustomizableUI.getPlacementOfWidget("sidebar-button", false, true);
> +    if (!widget.areaType) {
> +      CustomizableUI.addWidgetToArea("sidebar-button", CustomizableUI.AREA_NAVBAR);
> +      CustomizableUI.moveWidgetWithinArea("sidebar-button", 0);

I'm not sold on moving this before the URL bar. We don't normally let other add-ons do this. Is this really necessary?

Also, you can just pass the index directly to addWidgetToArea if we keep this. (if that doesn't work, we should fix it! :-) )

@@ +51,5 @@
> +      CustomizableUI.addWidgetToArea("sidebar-button", CustomizableUI.AREA_NAVBAR);
> +      CustomizableUI.moveWidgetWithinArea("sidebar-button", 0);
> +    }
> +
> +    this.tabContext.on("tab-select", // eslint-disable-line mozilla/balanced-listeners

I'm assuming you're getting someone else to review this and they know why this comment is necessary everywhere (ie why aren't we balancing listeners)?

@@ +75,5 @@
> +    broadcaster.setAttribute("group", "sidebar");
> +    broadcaster.setAttribute("label", details.title);
> +    broadcaster.setAttribute("webpanelurl", details.panel);
> +    broadcaster.setAttribute("sidebarurl", "chrome://browser/content/webext-panels.xul");
> +    broadcaster.setAttribute("oncommand", `WebSidebarUI.toggle("${this.id}")`);

Where and how is `this.id` validated? Wouldn't want anyone escaping out of this quoted string and running arbitrary chrome-privileged JS when the user clicks things. :-\

@@ +100,5 @@
> +    }
> +
> +    // Update the broadcaster first, it will update both menus.
> +    let broadcaster = document.getElementById(this.id);
> +    broadcaster.setAttribute("tooltiptext", title);

If the title and tooltip are the same, just omit the tooltip.

@@ +130,5 @@
> +
> +    // These URLs should already be properly escaped, but make doubly sure CSS
> +    // string escape characters are escaped here, since they could lead to a
> +    // sandbox break.
> +    let escape = str => str.replace(/[\\\s"]/g, encodeURIComponent);

I would expect this needs to check for a bunch of other stuff, so I would sooner limit what's allowed rather than what's forbidden, and have the regexp escape everything that's not in: [a-zA-Z0-9_./:-].

Also, what globals encodeURIComponent here? Is that available by default on a JSM? I would have thought you needed window.encodeURIComponent.

@@ +132,5 @@
> +    // string escape characters are escaped here, since they could lead to a
> +    // sandbox break.
> +    let escape = str => str.replace(/[\\\s"]/g, encodeURIComponent);
> +
> +    let getIcon = size => escape(IconDetails.getPreferredIcon(tabData.icon, this.extension, size).icon);

Nit: rather than the separate escape function/lambda, just make this a braced function and use a single temp var:

let {icon} = IconDetails.getPreferredIcon(...);
return icon.replace(...);

@@ +166,5 @@
> +  // prop should be one of "icon", "title", or "panel".
> +  setProperty(tab, prop, value) {
> +    if (tab == null) {
> +      this.defaults[prop] = value;
> +    } else if (value != null) {

Here and elsewhere, do not compare to null or "", just falsy/truthy-check.

if (!tab) {
...
} else if (value) {
...
}

If there's supposed to be a difference between "" and null and undefined etc., then shame on whoever designed this API. :'-(
In which case, please comment and use a strict (===) comparison for the thing you're looking for first (e.g. value === "").

@@ +227,5 @@
> +  let {extension} = context;
> +  return {
> +    sidebarAction: {
> +      setTitle: function(details) {
> +        let tab = details.tabId !== null ? TabManager.getTab(details.tabId, context) : null;

What if tabId === undefined, ie not present on the details object?

::: browser/components/extensions/schemas/sidebar_action.json
@@ +1,1 @@
> +// Copyright (c) 2012 The Chromium Authors. All rights reserved.

Not sure this makes sense here?

@@ +40,5 @@
> +    "description": "Use sidebar actions to add a sidebar to Firefox.",
> +    "permissions": ["manifest:sidebar_action"],
> +    "types": [
> +      {
> +        "id": "ColorArray",

This seems to be unused?

@@ +121,5 @@
> +      },
> +      {
> +        "name": "setIcon",
> +        "type": "function",
> +        "description": "Sets the icon for the sidebar action. The icon can be specified either as the path to an image file or as the pixel data from a canvas element, or as dictionary of either one of those. Either the <b>path</b> or the <b>imageData</b> property must be specified.",

Nit: <strong> or <code> rather than <b> . :-\

@@ +148,5 @@
> +                    "additionalProperties": {"type": "string"}
> +                  }
> +                ],
> +                "optional": true,
> +                "description": "Either a relative image path or a dictionary {size -> relative image path} pointing to icon to be set. If the icon is specified as a dictionary, the actual image to be used is chosen depending on screen's pixel density. If the number of image pixels that fit into one screen space unit equals <code>scale</code>, then image with size <code>scale</code> * 19 will be selected. Initially only scales 1 and 2 will be supported. At least one image must be specified. Note that 'details.path = foo' is equivalent to 'details.imageData = {'19': foo}'"

Is this sizing still right? I suspect sidebar icons use 16x16 favicon sizes.

@@ +179,5 @@
> +              "tabId": {
> +                "type": "integer",
> +                "optional": true,
> +                "minimum": 0,
> +                "description": "Limits the change to when a particular tab is selected. Automatically resets when the tab is closed."

I don't understand what "the change" refers to. Can you clarify?

@@ +183,5 @@
> +                "description": "Limits the change to when a particular tab is selected. Automatically resets when the tab is closed."
> +              },
> +              "panel": {
> +                "type": "string",
> +                "description": "The html file to show in a sidebar.  If set to the empty string (''), no sidebar is shown."

Is this supposed to be markup or a URL?

@@ +198,5 @@
> +      },
> +      {
> +        "name": "getPanel",
> +        "type": "function",
> +        "description": "Gets the html document set as the panel for this sidebar action.",

What is "the html document"? Do you pass a reference to the document object, or a URL, or something else?

::: toolkit/mozapps/extensions/internal/WebExtensionBootstrap.js
@@ +32,5 @@
>  }
>  
>  function shutdown(data, reason)
>  {
> +  extension.shutdown(BOOTSTRAP_REASON_TO_STRING_MAP[reason]);

This feels like a change that might want splitting out to a different bug?

Comment 34

[Kris Maglione [:kmag]]

Comment on attachment 8822788 [details] [diff] [review]
WIP sidebarAPI

Review of attachment 8822788 [details] [diff] [review]:
-----------------------------------------------------------------

::: browser/base/content/browser-sidebar.js
@@ +319,5 @@
> +      let sidebarBroadcaster = document.getElementById(commandID);
> +      let webpanel = sidebarBroadcaster.getAttribute("webpanelurl");
> +      SidebarUI.browser.contentWindow.loadWebPanel(webpanel);
> +    });
> +  },

Can we upgrade these to async functions?

::: browser/base/content/webext-panels.xul
@@ +64,5 @@
> +    </menupopup>
> +  </popupset>
> +
> +  <commandset id="editMenuCommands"/>
> +  <browser id="webext-panels-browser" persist="cachedurl" type="content" flex="1"

This browser is going to need to be remote, and with the correct remoteType, based on the same logic as our other browsers.

::: browser/components/extensions/ext-sidebarAction.js
@@ +16,5 @@
> +var sidebarActionMap = new WeakMap();
> +
> +// Responsible for the sidebar_action section of the manifest as well
> +// as the associated panel.
> +function SidebarAction(options, extension) {

Please use ES6 class and JSDoc doc comments.

@@ +51,5 @@
> +      CustomizableUI.addWidgetToArea("sidebar-button", CustomizableUI.AREA_NAVBAR);
> +      CustomizableUI.moveWidgetWithinArea("sidebar-button", 0);
> +    }
> +
> +    this.tabContext.on("tab-select", // eslint-disable-line mozilla/balanced-listeners

The tabContext object is unique to this instance, and we destroy it when the instance is destroyed, so there's not really any point in also removing the individual listeners.

@@ +75,5 @@
> +    broadcaster.setAttribute("group", "sidebar");
> +    broadcaster.setAttribute("label", details.title);
> +    broadcaster.setAttribute("webpanelurl", details.panel);
> +    broadcaster.setAttribute("sidebarurl", "chrome://browser/content/webext-panels.xul");
> +    broadcaster.setAttribute("oncommand", `WebSidebarUI.toggle("${this.id}")`);

Agree. If we generate an oncommand string, we need to escape using something like `JSON.stringify`. I'd rather we just use a dummy oncommand attribute (e.g., "//") and add a real event listener using `addEventListener`, though.

@@ +124,5 @@
> +
> +      if (result.size % 18 == 0) {
> +        baseSize = 18;
> +        icon = result.icon;
> +      }

This is just a compatibility hack for extensions that were created with 18px browserAction icons before we changed the preferred size to 16px. Any extension that's adding a sidebar should be able to use a 16px icon from the start, so we don't need to worry about breaking them, and we don't need this special case.

Also, the bestSize logic and default `icon` value aren't used here.

@@ +130,5 @@
> +
> +    // These URLs should already be properly escaped, but make doubly sure CSS
> +    // string escape characters are escaped here, since they could lead to a
> +    // sandbox break.
> +    let escape = str => str.replace(/[\\\s"]/g, encodeURIComponent);

This should handle all of the cases permitted by the CSS spec, and we have tests for this escaping function in the browserAction code. We should move that helper to somewhere shareable, though, rather than duplicating it here.

@@ +136,5 @@
> +    let getIcon = size => escape(IconDetails.getPreferredIcon(tabData.icon, this.extension, size).icon);
> +
> +    menu.setAttribute("style", `
> +      --webextension-menuitem-image: url("${getIcon(32)}");
> +      --webextension-menuitem-image-2x: url("${getIcon(64)}");

Shouldn't this be 16 and 32?

@@ +166,5 @@
> +  // prop should be one of "icon", "title", or "panel".
> +  setProperty(tab, prop, value) {
> +    if (tab == null) {
> +      this.defaults[prop] = value;
> +    } else if (value != null) {

Yes, there is a difference between null and an empty string. One sets the value to an empty string, the other falls back to the default value. We inherited this behavior from Chrome. So actually checking for null is what we want.

@@ +227,5 @@
> +  let {extension} = context;
> +  return {
> +    sidebarAction: {
> +      setTitle: function(details) {
> +        let tab = details.tabId !== null ? TabManager.getTab(details.tabId, context) : null;

We normalize function parameters so that missing or undefined properties are normalized to `null`. The strict equality checks are for the sake of ensuring sanity of that code.

@@ +251,5 @@
> +        let icon = IconDetails.normalize(details, extension, context);
> +        SidebarAction.for(extension).setProperty(tab, "icon", icon);
> +      },
> +
> +      setPanel: function(details) {

I'm not sure "Panel" is the right name for this. How about just "setURL"?

@@ +258,5 @@
> +        // Note: Chrome resolves arguments to setIcon relative to the calling
> +        // context, but resolves arguments to setPanel relative to the extension
> +        // root.
> +        // For internal consistency, we currently resolve both relative to the
> +        // calling context.

This comment isn't really relevant here, since Chrome does not have an equivalent API.

::: browser/components/extensions/schemas/sidebar_action.json
@@ +21,5 @@
> +              "default_icon": {
> +                "$ref": "IconPath",
> +                "optional": true
> +              },
> +              "default_panel": {

Same here. "url" rather than "panel"?

@@ +24,5 @@
> +              },
> +              "default_panel": {
> +                "type": "string",
> +                "format": "relativeUrl",
> +                "optional": true,

I don't think this should be optional. Or, if it is, we at least need to hide the sidebar item when no URL is set.

@@ +86,5 @@
> +            "type": "function",
> +            "name": "callback",
> +            "optional": true,
> +            "parameters": []
> +          }

No callback functions in APIs that don't have an equivalent in Chrome.

Comment 35

[Shane Caraveo (:mixedpuppy)]

(In reply to Kris Maglione [:kmag] from comment #34)

> @@ +251,5 @@
> > +        let icon = IconDetails.normalize(details, extension, context);
> > +        SidebarAction.for(extension).setProperty(tab, "icon", icon);
> > +      },
> > +
> > +      setPanel: function(details) {
> 
> I'm not sure "Panel" is the right name for this. How about just "setURL"?

> ::: browser/components/extensions/schemas/sidebar_action.json
> @@ +21,5 @@
> > +              "default_icon": {
> > +                "$ref": "IconPath",
> > +                "optional": true
> > +              },
> > +              "default_panel": {
> 
> Same here. "url" rather than "panel"?
> 
> @@ +24,5 @@
> > +              },
> > +              "default_panel": {
> > +                "type": "string",
> > +                "format": "relativeUrl",
> > +                "optional": true,
> 
> I don't think this should be optional. Or, if it is, we at least need to
> hide the sidebar item when no URL is set.
> 
> @@ +86,5 @@
> > +            "type": "function",
> > +            "name": "callback",
> > +            "optional": true,
> > +            "parameters": []
> > +          }
> 
> No callback functions in APIs that don't have an equivalent in Chrome.

One objective is that this is compatible with Opera, which basically created sidebarAction based on browserAction, so all the nuances and naming need to remain the same.  For other APIs where there is no browser supporting it, we can do better naming.

Comment 36

[Shane Caraveo (:mixedpuppy)]

> @@ +75,5 @@
> > +    broadcaster.setAttribute("group", "sidebar");
> > +    broadcaster.setAttribute("label", details.title);
> > +    broadcaster.setAttribute("webpanelurl", details.panel);
> > +    broadcaster.setAttribute("sidebarurl", "chrome://browser/content/webext-panels.xul");
> > +    broadcaster.setAttribute("oncommand", `WebSidebarUI.toggle("${this.id}")`);
> 
> Agree. If we generate an oncommand string, we need to escape using something
> like `JSON.stringify`. I'd rather we just use a dummy oncommand attribute
> (e.g., "//") and add a real event listener using `addEventListener`, though.

I'm not really clear why we need to do anything with this.id.  We use the same logic in lots of places in webext, including browserAction, and the id is used in node attributes/etc.

Comment 37

[Kris Maglione [:kmag]]

(In reply to Shane Caraveo (:mixedpuppy) from comment #36)
> I'm not really clear why we need to do anything with this.id.  We use the
> same logic in lots of places in webext, including browserAction, and the id
> is used in node attributes/etc.

In this case, we're not just using it in an attribute. We're using it in a script.
This is always safe:

    node.setAttribute("id", foo);

This is not:

    node.setAttribute("oncommand", `frob("${foo}")`);

Comment 38

[Shane Caraveo (:mixedpuppy)]

(In reply to :Gijs from comment #33)
> Comment on attachment 8822788 [details] [diff] [review]
> WIP sidebarAPI
> 
> Review of attachment 8822788 [details] [diff] [review]:
> -----------------------------------------------------------------

> Pretty please can we have the next revision in mozreview so interdiffs will
> work?

Yes.

> Hohum about calling these "panels"... Looking around, it looks like this is
> a clone of the web-panels thing we already have. Why can't we reuse that for
> the extension sidebars? AIUI the panels loaded by webextensions don't have
> any particular privileges / APIs available to them. If that is not the case,
> as long as they are always on distinctive origins, it should be possible to
> have the webextensions APIs available / not available as necessary. We
> shouldn't need a completely separate XUL file and JS for that.

The api, which we want to keep compatible with opera, already calls them panels.  I'd like to remain consistent in that.  I know, it's generic.

I've asked Kev to look at [talk to the right people] whether the bookmark sidebar functionality should be removed.  I recall discussion of that a couple years ago, and it's a hard to find bit of functionality that doesn't seem maintained.  As well, there are things we may want to add that will be specific to webextension sidebars, so it's just easier to separate them.  I can move sidebar stuff in a followup patch (perhaps even in this bug as a second patch).

> @@ +23,5 @@
> > +  // Add the extension to the sidebar menu.  The sidebar widget will copy
> > +  // from that when it is viewed, so we shouldn't need to update that.
> > +  let widgetId = makeWidgetId(extension.id);
> > +  this.id = `${widgetId}-sidebar-action`;
> > +  this.menuId = `menu-${this.id}`;
> 
> This has the potential of conflicting with existing items, right, if the
> add-on id matches any of the builtin items? Can we make more sure it is
> unique, including 'webext' in the name or something? Also, convention for
> menu items is using _ rather than -.

I'd find it difficult to see that conflicting, the extension id is a generated uuid.

> @@ +48,5 @@
> > +    let widget = CustomizableUI.getWidget("sidebar-button");
> > +    // let placement = CustomizableUI.getPlacementOfWidget("sidebar-button", false, true);
> > +    if (!widget.areaType) {
> > +      CustomizableUI.addWidgetToArea("sidebar-button", CustomizableUI.AREA_NAVBAR);
> > +      CustomizableUI.moveWidgetWithinArea("sidebar-button", 0);
> 
> I'm not sold on moving this before the URL bar. We don't normally let other
> add-ons do this. Is this really necessary?
> 
> Also, you can just pass the index directly to addWidgetToArea if we keep
> this. (if that doesn't work, we should fix it! :-) )

This is fixed up in the next patch (only moves it once ever, first install of webextension with sidebar), and it may yet be removed, but will wait for feedback from Kev again, and that will happen prior to landing this (or I'll disable or remove that code).

Comment 39

[Shane Caraveo (:mixedpuppy)]

Attachment: Bug 1208596 implement sidebar api for webextensions, f?kmag, gijs

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

Comment 40

[Shane Caraveo (:mixedpuppy)]

I didn't see a way to ask for feedback on reviewboard, so r? it is.  Gijs, you can limit your focus to CUI related issues if you would like to shorten it, but feel free to dig into the rest if you want.

Comment 41

[:Gijs (he/him)]

Comment on attachment 8826753 [details]
Bug 1208596 implement sidebar api for webextensions, f?kmag, gijs

https://reviewboard.mozilla.org/r/104628/#review105682

::: browser/base/content/browser-sidebar.js
(Diff revision 1)
> -    // Note: we're setting 'src' on this._box, which is a <vbox>, not on
> -    // the <browser id="sidebar">. This lets us delay the actual load until
> -    // delayedStartup().

Keep this comment as we're still doing this, looks like? If that's not necessary, remove the setting of `src` on the box and do it directly on the browser, later on?

::: browser/components/customizableui/CustomizableWidgets.jsm:105
(Diff revision 1)
>  }
>  
>  function fillSubviewFromMenuItems(aMenuItems, aSubview) {
>    let attrs = ["oncommand", "onclick", "label", "key", "disabled",
>                 "command", "observes", "hidden", "class", "origin",
> -               "image", "checked"];
> +               "image", "checked", "style"];

This is the only CUI change, right? This seems fine... I haven't reviewed the rest again in detail per your comment.

::: browser/components/extensions/ext-sidebarAction.js:64
(Diff revision 1)
> +      Services.prefs.setBoolPref("extensions.sidebar-button.shown", true);
> +      // If the sidebar button has never been moved to the toolbar, move it now
> +      // so the user can see/access the sidebars.
> +      let widget = CustomizableUI.getWidget("sidebar-button");
> +      if (!widget.areaType) {
> +        CustomizableUI.addWidgetToArea("sidebar-button", CustomizableUI.AREA_NAVBAR, 0);

On Windows/Linux, the back button is fitts' law'd to the side (ie clicking just inside the window edge (on the screen edge in maximized mode) will activate it). I expect this won't work with the sidebar button (plus it'll break that feature for the back button). That and the fact that all the other items in the nav bar go on the end side mean I'm reluctant about this change, but if we think it's important I guess we can do it...

Comment 42

[Shane Caraveo (:mixedpuppy)]

(In reply to :Gijs from comment #41)
> Comment on attachment 8826753 [details]
> Bug 1208596 implement sidebar api for webextensions, f?kmag, gijs

> ::: browser/components/extensions/ext-sidebarAction.js:64
> (Diff revision 1)
> > +      Services.prefs.setBoolPref("extensions.sidebar-button.shown", true);
> > +      // If the sidebar button has never been moved to the toolbar, move it now
> > +      // so the user can see/access the sidebars.
> > +      let widget = CustomizableUI.getWidget("sidebar-button");
> > +      if (!widget.areaType) {
> > +        CustomizableUI.addWidgetToArea("sidebar-button", CustomizableUI.AREA_NAVBAR, 0);
> 
> On Windows/Linux, the back button is fitts' law'd to the side (ie clicking
> just inside the window edge (on the screen edge in maximized mode) will
> activate it). I expect this won't work with the sidebar button (plus it'll
> break that feature for the back button). That and the fact that all the
> other items in the nav bar go on the end side mean I'm reluctant about this
> change, but if we think it's important I guess we can do it...

After chatting with Kev, we're going to leave it in at that insertion point, but get feedback and possibly some user testing.  I'll add code to prevent insertion on beta/release.

Comment 43

[Shane Caraveo (:mixedpuppy)]

Comment on attachment 8826753 [details]
Bug 1208596 implement sidebar api for webextensions, f?kmag, gijs

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/104628/diff/1-2/

Comment 44

[Shane Caraveo (:mixedpuppy)]

Comment on attachment 8826753 [details]
Bug 1208596 implement sidebar api for webextensions, f?kmag, gijs

https://reviewboard.mozilla.org/r/104628/#review105682

> This is the only CUI change, right? This seems fine... I haven't reviewed the rest again in detail per your comment.

Aside from the sidebar-button injection, yes.

Comment 45

[Shane Caraveo (:mixedpuppy)]

Comment on attachment 8826753 [details]
Bug 1208596 implement sidebar api for webextensions, f?kmag, gijs

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/104628/diff/2-3/

Comment 46

[Shane Caraveo (:mixedpuppy)]

Comment on attachment 8826753 [details]
Bug 1208596 implement sidebar api for webextensions, f?kmag, gijs

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/104628/diff/3-4/

Comment 47

[Kris Maglione [:kmag]]

Comment on attachment 8826753 [details]
Bug 1208596 implement sidebar api for webextensions, f?kmag, gijs

https://reviewboard.mozilla.org/r/104628/#review107154

::: browser/base/content/browser-sidebar.js:102
(Diff revision 4)
> -    if (!this._delayedLoad) {
> +    let sourceWindow = window.opener;
> +    // No source window means this is the initial window.  If we're being
> +    // opened from another window, check that it is one we might open a sidebar
> +    // for.
> +    if (sourceWindow) {
> +      if (sourceWindow.closed || !sourceWindow.document.documentURIObject.schemeIs("chrome") ||

How about `sourceWindow.location.protocol != "chrome:"`? It's a fair bit shorter.

::: browser/base/content/browser-sidebar.js:103
(Diff revision 4)
> +    // No source window means this is the initial window.  If we're being
> +    // opened from another window, check that it is one we might open a sidebar
> +    // for.
> +    if (sourceWindow) {
> +      if (sourceWindow.closed || !sourceWindow.document.documentURIObject.schemeIs("chrome") ||
> +        PrivateBrowsingUtils.isWindowPrivate(window) != PrivateBrowsingUtils.isWindowPrivate(sourceWindow)) {

Can we destructure `isWindowPrivate` into a variable? This is a bit hard to read.

::: browser/base/content/browser-sidebar.js:335
(Diff revision 4)
> +   * @see SidebarUI note.
> +   *
> +   * @param {string} commandID ID of the xul:broadcaster element to use.
> +   */
> +  async show(commandID) {
> +    await SidebarUI.show(commandID).then(() => {

No need for the `.then()` clause. Just `await SidebarUI.show(...)` followed by the current contents of the result handler.

::: browser/base/content/browser-sidebar.js:337
(Diff revision 4)
> +      let webpanel = sidebarBroadcaster.getAttribute("webpanelurl");
> +      let remote = sidebarBroadcaster.getAttribute("remotepanel");
> +      SidebarUI.browser.contentWindow.loadWebPanel(webpanel, remote);

This feels a bit flaky. Can we just embed the source URL as a query parameter in the sidebar URL, or something like that?

::: browser/base/content/browser-sidebar.js:353
(Diff revision 4)
>   * @deprecated
>   */
>  function fireSidebarFocusedEvent() {}
>  
>  /**
> - * This exists for backards compatibility - it gets called when a sidebar has
> + * This exists for backwards compatibility - it gets called when a sidebar has

So pedantic...

::: browser/base/content/webext-panels.js:11
(Diff revision 4)
> +  if (remote) {
> +    browser.setAttribute("remote", "true");
> +  } else {
> +    browser.removeAttribute("remote");
> +  }

We need to handle the remote and remoteType attributes the same as we do for other browsers. See ext-utils.js for an example.

::: browser/components/extensions/ext-sidebarAction.js:6
(Diff revision 4)
> +XPCOMUtils.defineLazyModuleGetter(this, "CustomizableUI",
> +                                  "resource:///modules/CustomizableUI.jsm");
> +XPCOMUtils.defineLazyModuleGetter(this, "AppConstants",
> +                                  "resource://gre/modules/AppConstants.jsm");

Please keep imports sorted.

::: browser/components/extensions/ext-sidebarAction.js:65
(Diff revision 4)
> +    // Bug 1331507: UX review/analysis of sidebar-button injection.
> +    if (AppConstants.RELEASE_OR_BETA) {
> +      return;
> +    }
> +
> +    if (install && !Services.prefs.prefHasUserValue("extensions.sidebar-button.shown")) {

I think we want this to be per-extension, the first startup after the add-on is installed.

::: browser/components/extensions/ext-sidebarAction.js:134
(Diff revision 4)
> +    }
> +
> +    // These URLs should already be properly escaped, but make doubly sure CSS
> +    // string escape characters are escaped here, since they could lead to a
> +    // sandbox break.
> +    let escape = str => str.replace(/[\\\s"]/g, encodeURIComponent);

Again, I'd rather we move this to ExtensionUtils, or somewhere similar, rather than duplicate it this many times.

::: browser/components/extensions/ext-sidebarAction.js:146
(Diff revision 4)
> +    `);
> +
> +    // Update the sidebar if this extension is the current sidebar.
> +    if (SidebarUI.currentID == this.id) {
> +      SidebarUI.title = title;
> +      if (SidebarUI.isOpen) {

We probably don't want to do this unless the URL has changed, or we're going to wind up unnecessarily reloading it.

::: browser/components/extensions/ext-sidebarAction.js:221
(Diff revision 4)
> +      document.getElementById(this.menuId).remove();
> +      document.getElementById(this.id).remove();

Let's check that these exist before trying to remove them, or we may wind up throwing when one doesn't, and failing to cleanup all windows.

::: browser/components/extensions/ext-sidebarAction.js:250
(Diff revision 4)
> +
> +extensions.on("shutdown", (type, extension) => {
> +  if (sidebarActionMap.has(extension)) {
> +    // Don't remove everything on app shutdown so session restore can handle
> +    // restoring open sidebars.
> +    if (extension.shutdownReason != "APP_SHUTDOWN") {

`!==` please. Same elsewhere.

::: browser/components/extensions/schemas/sidebar_action.json:27
(Diff revision 4)
> +                "$ref": "IconPath",
> +                "optional": true
> +              },
> +              "default_panel": {
> +                "type": "string",
> +                "format": "relativeUrl",

"strictRelativeUrl"

::: browser/components/extensions/schemas/sidebar_action.json:28
(Diff revision 4)
> +                "optional": true
> +              },
> +              "default_panel": {
> +                "type": "string",
> +                "format": "relativeUrl",
> +                "optional": true,

Does it make sense for this to be optional? We don't support these being disabled, and we don't handle the case where it doesn't exist.

::: browser/components/extensions/schemas/sidebar_action.json:74
(Diff revision 4)
> +          {
> +            "type": "function",
> +            "name": "callback",
> +            "optional": true,
> +            "parameters": []
> +          }

No callback, please. Just `"async": true`

::: browser/components/extensions/schemas/sidebar_action.json:126
(Diff revision 4)
> +              "imageData": {
> +                "choices": [
> +                  { "$ref": "ImageDataType" },
> +                  {
> +                    "type": "object",
> +                    "additionalProperties": {"$ref": "ImageDataType"}

These should probably be `"patternProperties"` like we use for `"IconPath"` in manifest.json

::: browser/components/extensions/schemas/sidebar_action.json:134
(Diff revision 4)
> +                "optional": true,
> +                "description": "Either an ImageData object or a dictionary {size -> ImageData} representing icon to be set. If the icon is specified as a dictionary, the actual image to be used is chosen depending on screen's pixel density. If the number of image pixels that fit into one screen space unit equals <code>scale</code>, then image with size <code>scale</code> * 19 will be selected. Initially only scales 1 and 2 will be supported. At least one image must be specified. Note that 'details.imageData = foo' is equivalent to 'details.imageData = {'19': foo}'"
> +              },
> +              "path": {
> +                "choices": [
> +                  { "type": "string" },

These should probably have `"format": "strictRelativeUri"`

::: browser/components/extensions/schemas/sidebar_action.json:175
(Diff revision 4)
> +                "optional": true,
> +                "minimum": 0,
> +                "description": "Sets the sidebar url for the tab specified by tabId. Automatically resets when the tab is closed."
> +              },
> +              "panel": {
> +                "type": "string",

`"format": "strictRelativeUri"`, probably.

::: browser/components/extensions/test/browser/browser_ext_sidebarAction_context.js:183
(Diff revision 4)
> +         "title": "Default Title 2",
> +        },
> +      ];
> +
> +      return [
> +        async expect => {

I'd kind of like to refactor tests that use this pattern now that we have async functions, but I guess it's fine for now.

::: browser/components/privatebrowsing/test/browser/head.js:20
(Diff revision 4)
>  
>    return win;
>  }
>  
> -function openWindow(aParent, aOptions, a3) {
> -  let { Promise: { defer } } = Components.utils.import("resource://gre/modules/Promise.jsm", {});
> +function openWindow(aParent, aOptions) {
> +  return TestUtils.topicObserved("browser-delayed-startup-finished",

It looks like we're not actually opening the window here, anymore. I assume this is a typo.

Either way, can we just use `BrowserTestUtils.openBrowserWindow` instead?

Comment 48

[Shane Caraveo (:mixedpuppy)]

Comment on attachment 8826753 [details]
Bug 1208596 implement sidebar api for webextensions, f?kmag, gijs

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/104628/diff/4-5/

Comment 49

[Shane Caraveo (:mixedpuppy)]

Comment on attachment 8826753 [details]
Bug 1208596 implement sidebar api for webextensions, f?kmag, gijs

https://reviewboard.mozilla.org/r/104628/#review107154

> It looks like we're not actually opening the window here, anymore. I assume this is a typo.
> 
> Either way, can we just use `BrowserTestUtils.openBrowserWindow` instead?

Heh, I pushed the patch that fixed that and didn't publish it.

Comment 50

[Shane Caraveo (:mixedpuppy)]

Comment on attachment 8826753 [details]
Bug 1208596 implement sidebar api for webextensions, f?kmag, gijs

https://reviewboard.mozilla.org/r/104628/#review107154

> This feels a bit flaky. Can we just embed the source URL as a query parameter in the sidebar URL, or something like that?

Doing that would cause two browser elements to reload, rather than just the one for the content.

> I think we want this to be per-extension, the first startup after the add-on is installed.

We actually only want the button to be injected once ever.  However this may change entirely with Bug 1331507, so leaving as is for now.

Comment 51

[Shane Caraveo (:mixedpuppy)]

Comment on attachment 8826753 [details]
Bug 1208596 implement sidebar api for webextensions, f?kmag, gijs

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/104628/diff/5-6/

Comment 52

[Shane Caraveo (:mixedpuppy)]

Comment on attachment 8826753 [details]
Bug 1208596 implement sidebar api for webextensions, f?kmag, gijs

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/104628/diff/6-7/

Comment 53

[Shane Caraveo (:mixedpuppy)]

Comment on attachment 8826753 [details]
Bug 1208596 implement sidebar api for webextensions, f?kmag, gijs

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/104628/diff/7-8/

Comment 54

[Shane Caraveo (:mixedpuppy)]

Comment on attachment 8826753 [details]
Bug 1208596 implement sidebar api for webextensions, f?kmag, gijs

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/104628/diff/8-9/

Comment 55

[Shane Caraveo (:mixedpuppy)]

Comment on attachment 8826753 [details]
Bug 1208596 implement sidebar api for webextensions, f?kmag, gijs

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/104628/diff/9-10/

Comment 56

[Kris Maglione [:kmag]]

Comment on attachment 8826753 [details]
Bug 1208596 implement sidebar api for webextensions, f?kmag, gijs

https://reviewboard.mozilla.org/r/104628/#review107154

> Doing that would cause two browser elements to reload, rather than just the one for the content.

I'm not convinced this is worth worrying about. It should only ever be an issue when directly between two WebExtension sidebars, or chaning the URL of the current sidebar. And even in that case, I doubt the performance difference will be significant. The outer sidebar document will be loaded from the XUL cache, and should happen very quickly (probably imperceptibly). Next to the time it takes to load the inner HTML document, it shouldn't be an issue.

Comment 57

[Kris Maglione [:kmag]]

Comment on attachment 8826753 [details]
Bug 1208596 implement sidebar api for webextensions, f?kmag, gijs

https://reviewboard.mozilla.org/r/104628/#review110900

Close, but I'm still worried about the handling of the sidebar document, and I think we're missing a couple of things.

::: browser/base/content/webext-panels.js:13
(Diff revision 10)
> +
> +function loadWebPanel(aURI, remote) {
> +  let browser = document.getElementById("webext-panels-browser");
> +  if (remote) {
> +    browser.setAttribute("remote", "true");
> +    browser.setAttribute("remoteType", E10SUtils.EXTENSION_REMOTE_TYPE);

We should use `E10SUtils.getRemoteTypeForURI` here.

::: browser/base/content/webext-panels.js:28
(Diff revision 10)
> +  let browser = document.getElementById("webext-panels-browser");
> +  browser.messageManager.loadFrameScript("chrome://browser/content/content.js", true);
> +  ExtensionParent.apiManager.emit("extension-browser-inserted", browser);
> +
> +  let cachedurl = browser.getAttribute("cachedurl");
> +  browser.setAttribute("src", cachedurl);

Please use `loadURI` rather than setting "src".

Also, this means we'll be loading the document twice if `loadWebPanel` is called before the load event fires.

::: browser/components/extensions/ext-browserAction.js:356
(Diff revision 10)
>          icon = result.icon;
>          node.classList.add(LEGACY_CLASS);
>        }
>      }
>  
> -    // These URLs should already be properly escaped, but make doubly sure CSS
> +    let getIcon = size => IconDetails.escapeUrl(IconDetails.getPreferredIcon(tabData.icon, this.extension, size).icon);

Nit: Please add a line break after the first paren.

::: browser/components/extensions/ext-sidebarAction.js:36
(Diff revision 10)
> +    let widgetId = makeWidgetId(extension.id);
> +    this.id = `${widgetId}-sidebar-action`;
> +    this.menuId = `menu_${this.id}`;
> +
> +    this.tabManager = TabManager.for(extension);
> +

This should just be `extension.tabManager` after the tab backend changes.

::: browser/components/extensions/ext-sidebarAction.js:52
(Diff revision 10)
> +
> +  build() {
> +    this.tabContext.on("tab-select", // eslint-disable-line mozilla/balanced-listeners
> +                       (evt, tab) => { this.updateWindow(tab.ownerGlobal); });
> +
> +    let install = this.extension.startupReason == "ADDON_INSTALL";

Nit: s/==/&=/

::: browser/components/extensions/ext-sidebarAction.js:53
(Diff revision 10)
> +  build() {
> +    this.tabContext.on("tab-select", // eslint-disable-line mozilla/balanced-listeners
> +                       (evt, tab) => { this.updateWindow(tab.ownerGlobal); });
> +
> +    let install = this.extension.startupReason == "ADDON_INSTALL";
> +    for (let window of WindowListManager.browserWindows()) {

s/WindowListManager/windowTracker/

::: browser/components/extensions/ext-sidebarAction.js:54
(Diff revision 10)
> +    this.tabContext.on("tab-select", // eslint-disable-line mozilla/balanced-listeners
> +                       (evt, tab) => { this.updateWindow(tab.ownerGlobal); });
> +
> +    let install = this.extension.startupReason == "ADDON_INSTALL";
> +    for (let window of WindowListManager.browserWindows()) {
> +      this.updateWindow(window);

It doesn't look like we actually handle windows that were created after the extension was initialized. We should add tests for that.

::: browser/components/extensions/ext-sidebarAction.js:79
(Diff revision 10)
> +    if (!details || !details.panel) {
> +      details = this.defaults;
> +    }

This shouldn't be necessary.

::: browser/components/extensions/ext-sidebarAction.js:109
(Diff revision 10)
> +   * Update the broadcaster and menuitem |node| with the tab context data
> +   * in |tabData|.

Nit: Please use backticks (JSDoc doesn't know about them without plugins, but they're more consistent with our other docs) rather than pipes. Also, I'm not sure what `node` refers to here.

Same elsewhere.

::: browser/components/extensions/ext-sidebarAction.js:112
(Diff revision 10)
> +   * @param {object} window
> +   * @param {object} tabData

Nit: Please use more specific types (`DOMWindow` or `Window` for the window parameter), and describe the parameters. Same elsewhere.

::: browser/components/extensions/ext-sidebarAction.js:134
(Diff revision 10)
> +      broadcaster.setAttribute("remotepanel", "true");
> +    } else {
> +      broadcaster.removeAttribute("remotepanel");
> +    }
> +
> +    let getIcon = size => IconDetails.escapeUrl(IconDetails.getPreferredIcon(tabData.icon, this.extension, size).icon);

Nit: Please add a line break after the first paren.

::: browser/components/extensions/ext-sidebarAction.js:144
(Diff revision 10)
> +    `);
> +
> +    // Update the sidebar if this extension is the current sidebar.
> +    if (SidebarUI.currentID === this.id) {
> +      SidebarUI.title = title;
> +      if (SidebarUI.isOpen && tabData.panel !== this.defaults.panel) {

I don't think this check is helpful. It's possible to change the default panel via the API, too, and we should also update the existing panels in that case.

::: browser/components/extensions/ext-sidebarAction.js:165
(Diff revision 10)
> +  /**
> +   * Update the broadcaster and menuitem when the extension changes the icon,
> +   * title, url, etc. If it only changes a parameter for a single
> +   * tab, |tab| will be that tab. Otherwise it will be null.
> +   *
> +   * @param {object} tab

s/object/XULElement/

::: browser/components/extensions/ext-sidebarAction.js:173
(Diff revision 10)
> +    if (tab) {
> +      if (tab.selected) {
> +        this.updateWindow(tab.ownerGlobal);
> +      }
> +    } else {
> +      for (let window of WindowListManager.browserWindows()) {

w/WindowListManager/windowTracker/

::: browser/components/extensions/ext-sidebarAction.js:188
(Diff revision 10)
> +   * @param {string} prop String property to retrieve ["icon", "title", or "panel"]
> +   * @param {string} value
> +   */
> +  setProperty(tab, prop, value) {
> +    if (tab === null) {
> +      this.defaults[prop] = value;

We should make sure not to null out the panel here.

::: browser/components/extensions/ext-sidebarAction.js:214
(Diff revision 10)
> +    return this.tabContext.get(tab)[prop];
> +  }
> +
> +  shutdown() {
> +    this.tabContext.shutdown();
> +    for (let window of WindowListManager.browserWindows()) {

s/WindowListManager/windowTracker/

::: browser/components/extensions/ext-sidebarAction.js:266
(Diff revision 10)
> +
> +extensions.registerSchemaAPI("sidebarAction", "addon_parent", context => {
> +  let {extension} = context;
> +  return {
> +    sidebarAction: {
> +      setTitle: function(details) {

These setters should probably all be async and return promises.

::: browser/components/extensions/ext-sidebarAction.js:267
(Diff revision 10)
> +extensions.registerSchemaAPI("sidebarAction", "addon_parent", context => {
> +  let {extension} = context;
> +  return {
> +    sidebarAction: {
> +      setTitle: function(details) {
> +        let tab = details.tabId !== null ? TabManager.getTab(details.tabId, context) : null;

Please add a helper for this, and s/TabManager/tabTracker/. See ext-tabs.js.

::: browser/components/extensions/schemas/sidebar_action.json:79
(Diff revision 10)
> +      },
> +      {
> +        "name": "getTitle",
> +        "type": "function",
> +        "description": "Gets the title of the sidebar action.",
> +        "async": "callback",

Please use `"async": true` and omit the callback parameter.

::: browser/components/extensions/test/browser/browser_ext_sidebarAction.js:20
(Diff revision 10)
> +      <!DOCTYPE html>
> +      <html>
> +      <head><meta charset="utf-8"/></head>
> +      <body>
> +      A Test Sidebar
> +      <script src="sidebar.js"></script>

Please move to <head>.

::: browser/components/extensions/test/browser/browser_ext_sidebarAction.js:81
(Diff revision 10)
> +  // We just close the sidebar on uninstall of the current sidebar.
> +  ok(document.getElementById("sidebar-box").hidden, "sidebar box is not visible");
> +
> +  yield extension2.unload();
> +});
> +

Please test opening a new window after the extension has initialized.

::: browser/components/extensions/test/browser/browser_ext_sidebarAction_context.js:149
(Diff revision 10)
> +        <!DOCTYPE html>
> +        <html>
> +        <head><meta charset="utf-8"/></head>
> +        <body>
> +        A Test Sidebar
> +        <script src="sidebar.js"></script>

Please move to <head>.

Comment 58

[Shane Caraveo (:mixedpuppy)]

Comment on attachment 8826753 [details]
Bug 1208596 implement sidebar api for webextensions, f?kmag, gijs

https://reviewboard.mozilla.org/r/104628/#review110900

> Nit: s/==/&=/

Isn't startupReason a string here?  WebExtensionBootstrap.js#13  Though it should probably be ===

Comment 59

[Kris Maglione [:kmag]]

Comment on attachment 8826753 [details]
Bug 1208596 implement sidebar api for webextensions, f?kmag, gijs

https://reviewboard.mozilla.org/r/104628/#review110900

> Isn't startupReason a string here?  WebExtensionBootstrap.js#13  Though it should probably be ===

It is, yes. But strict equality operators are preferred in WebExtension code.

Comment 60

[Shane Caraveo (:mixedpuppy)]

Comment on attachment 8826753 [details]
Bug 1208596 implement sidebar api for webextensions, f?kmag, gijs

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/104628/diff/10-11/

Comment 61

[Shane Caraveo (:mixedpuppy)]

Comment on attachment 8826753 [details]
Bug 1208596 implement sidebar api for webextensions, f?kmag, gijs

https://reviewboard.mozilla.org/r/104628/#review111708

Comment 62

[Kris Maglione [:kmag]]

Comment on attachment 8826753 [details]
Bug 1208596 implement sidebar api for webextensions, f?kmag, gijs

https://reviewboard.mozilla.org/r/104628/#review111756

::: browser/base/content/webext-panels.js:16
(Diff revision 11)
> +  let uri = sidebarURI.searchParams.get("panel");
> +  let remote = sidebarURI.searchParams.get("remote");
> +  let browser = document.getElementById("webext-panels-browser");
> +  if (remote) {
> +    let remoteType = E10SUtils.getRemoteTypeForURI(uri,
> +                                                   gMultiProcessBrowser,

Please just pass `true` for the second argument. If we've gotten to this point, we definitely want a remote browser.

::: browser/base/content/webext-panels.xul:68
(Diff revision 11)
> +#include browser-context.inc
> +    </menupopup>
> +  </popupset>
> +
> +  <commandset id="editMenuCommands"/>
> +  <browser id="webext-panels-browser" persist="remote,remoteType"

I'm not sure the `persist` is actually necessary. Or desirable.

::: browser/components/extensions/ext-browserAction.js:359
(Diff revision 11)
>          node.classList.add(LEGACY_CLASS);
>        }
>      }
>  
> -    // These URLs should already be properly escaped, but make doubly sure CSS
> -    // string escape characters are escaped here, since they could lead to a
> +    let getIcon = size => IconDetails.escapeUrl(
> +                            IconDetails.getPreferredIcon(tabData.icon, this.extension, size).icon);

Nit: This should be one indentation level past the start of the previous line.

::: browser/components/extensions/ext-sidebarAction.js:51
(Diff revision 11)
> +    // We need to ensure our elements are available before session restore.
> +    this.windowBeforeShow = (window, topic) => {
> +      if (window.gBrowser) {
> +        this.createMenuItem(window);
> +      }
> +    };
> +    Services.obs.addObserver(this.windowBeforeShow,
> +                             "browser-window-before-show",
> +                             false);

Please use `windowTracker.addOpenListener` for this. It should have the same effect with less noise.

And this listener needs to be removed when the extension is destroyed.

::: browser/components/extensions/ext-sidebarAction.js:53
(Diff revision 11)
> +    this.tabContext = new TabContext(tab => Object.create(this.defaults),
> +                                     extension);
> +
> +    // We need to ensure our elements are available before session restore.
> +    this.windowBeforeShow = (window, topic) => {
> +      if (window.gBrowser) {

Not necessary.

::: browser/components/extensions/ext-sidebarAction.js:66
(Diff revision 11)
> +
> +  build() {
> +    this.tabContext.on("tab-select", // eslint-disable-line mozilla/balanced-listeners
> +                       (evt, tab) => { this.updateWindow(tab.ownerGlobal); });
> +
> +    let install = this.extension.startupReason == "ADDON_INSTALL";

s/==/===/

::: browser/components/extensions/ext-sidebarAction.js:93
(Diff revision 11)
> +    if (!details || !details.panel) {
> +      details = this.defaults;
> +    }

Again, not necessary. We should always get a details object here, and fallback to the defaults happens automatically.

::: browser/components/extensions/ext-sidebarAction.js:105
(Diff revision 11)
> +    broadcaster.setAttribute("id", this.id);
> +    broadcaster.setAttribute("autoCheck", "false");
> +    broadcaster.setAttribute("type", "checkbox");
> +    broadcaster.setAttribute("group", "sidebar");
> +    broadcaster.setAttribute("label", details.title);
> +    broadcaster.setAttribute("sidebarurl", `${sidebarURL}?panel=${details.panel}`);

`encodeURIComponent(details.panel)`. Same elsewhere.

Also, please don't duplicate this logic. It's already present in `updateButton` (and slightly different there), so you should probably just skip adding the property here.

::: browser/components/extensions/ext-sidebarAction.js:122
(Diff revision 11)
> +   * Update the broadcaster and menuitem |node| with the tab context data
> +   * in |tabData|.

Again, please use backticks rather than pipes.

::: browser/components/extensions/ext-sidebarAction.js:125
(Diff revision 11)
> +   * @param {object} window
> +   * @param {object} tabData

Again, please use more specific types, and add parameter descriptions.

::: browser/components/extensions/ext-sidebarAction.js:147
(Diff revision 11)
> +    } else {
> +      broadcaster.setAttribute("sidebarurl", `${sidebarURL}?&panel=${tabData.panel}`);
> +    }
> +
> +    let getIcon = size => IconDetails.escapeUrl(
> +                            IconDetails.getPreferredIcon(tabData.icon, this.extension, size).icon);

Nit: One indentation level past the start of the previous line, please.

::: browser/components/extensions/ext-sidebarAction.js:158
(Diff revision 11)
> +
> +    // Update the sidebar if this extension is the current sidebar.
> +    if (SidebarUI.currentID === this.id) {
> +      SidebarUI.title = title;
> +      if (SidebarUI.isOpen) {
> +        SidebarUI.show(this.id);

We should only do this if the panel URL changes from what it was at the start of the function.

::: browser/components/extensions/ext-sidebarAction.js:169
(Diff revision 11)
> +   * Update the broadcaster and menuitem for a given window.
> +   *
> +   * @param {object} window
> +   */
> +  updateWindow(window) {
> +    let tab = window.gBrowser.selectedTab;

Nit: s/tab/nativeTab/

Same elsewhere, to match the naming in other modules.

::: browser/components/extensions/ext-sidebarAction.js:201
(Diff revision 11)
> +   * @param {string} prop String property to retrieve ["icon", "title", or "panel"]
> +   * @param {string} value
> +   */
> +  setProperty(tab, prop, value) {
> +    if (tab === null) {
> +      this.defaults[prop] = value;

We still need to make sure we're not accepting an empty default panel URL here. That should probably be done in `setPanel`, though.

::: browser/components/extensions/ext-sidebarAction.js:296
(Diff revision 11)
> +        return Task.spawn(function* () {
> +          SidebarAction.for(extension).setProperty(tab, "title", title);
> +        }).catch(error => Promise.reject({message: error.message}));

None of this, please. Unexpected errors are sanitized intentionally.

::: browser/components/extensions/test/browser/browser_ext_sidebarAction.js:20
(Diff revision 11)
> +      <!DOCTYPE html>
> +      <html>
> +      <head><meta charset="utf-8"/></head>
> +      <body>
> +      A Test Sidebar
> +      <script src="sidebar.js"></script>

Please move to <head>

Comment 63

[Shane Caraveo (:mixedpuppy)]

Comment on attachment 8826753 [details]
Bug 1208596 implement sidebar api for webextensions, f?kmag, gijs

https://reviewboard.mozilla.org/r/104628/#review111756

> `encodeURIComponent(details.panel)`. Same elsewhere.
> 
> Also, please don't duplicate this logic. It's already present in `updateButton` (and slightly different there), so you should probably just skip adding the property here.

updateButton is only called on install for open windows or when a property is changed via the api.  When new windows are opened, we only call createMenuItem.

> Again, please use backticks rather than pipes.

hrm.  Some of these I had done, but must have reverted at some point.

Comment 64

[Shane Caraveo (:mixedpuppy)]

Comment on attachment 8826753 [details]
Bug 1208596 implement sidebar api for webextensions, f?kmag, gijs

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/104628/diff/11-12/

Comment 65

[Kris Maglione [:kmag]]

Comment on attachment 8826753 [details]
Bug 1208596 implement sidebar api for webextensions, f?kmag, gijs

https://reviewboard.mozilla.org/r/104628/#review112586

Looks good. Thanks. r=me with the remaining issues addressed.

::: browser/components/extensions/ext-sidebarAction.js:202
(Diff revisions 11 - 12)
>     * @param {string} prop String property to retrieve ["icon", "title", or "panel"]
> -   * @param {string} value
> +   * @param {string} value Value for property
>     */
> -  setProperty(tab, prop, value) {
> -    if (tab === null) {
> +  setProperty(nativeTab, prop, value) {
> +    if (nativeTab === null) {
> +      if (value || prop !== "panel") {

Nit: Probably not necessary with the check already done in `setPanel`.

::: browser/components/extensions/ext-sidebarAction.js:217
(Diff revisions 11 - 12)
> -   * @param {object} tab webextension tab object, may be null
> +   * @param {XULElement} nativeTab browser tab object, may be null
>     * @param {string} prop String property to retrieve ["icon", "title", or "panel"]

Nit: Please move descriptions to the next line, align with opening `{`, capitalize, and add full stop. Same for other doc comments.

::: browser/components/extensions/ext-sidebarAction.js:217
(Diff revisions 11 - 12)
>    }
>  
>    /**
>     * Retrieve a property from the tab or defaults if tab is null.
>     *
> -   * @param {object} tab webextension tab object, may be null
> +   * @param {XULElement} nativeTab browser tab object, may be null

Nit: `{XULElement|null}`

Same for `setProperty`, `updateOnChange`, and anywhere else that I missed.

::: browser/components/extensions/ext-sidebarAction.js:299
(Diff revisions 11 - 12)
>          let title = details.title;
>          // Clear the tab-specific title when given a null string.
> -        if (tab && title === "") {
> +        if (nativeTab && title === "") {
>            title = null;
>          }
>          return Task.spawn(function* () {

No need for the task.spawn. Same for the other uses. But please switch to async functions (`async setTitle(details) { ... }`)

::: browser/components/extensions/ext-sidebarAction.js:330
(Diff revisions 11 - 12)
> +        if (nativeTab && details.panel === "") {
> +          url = null;
> +        } else if (details.panel !== "") {
> +          url = context.uri.resolve(details.panel);
> +        } else {
> +          return Promise.reject("Invalid url for sidebar panel.");

Need to reject with an object. Either an `ExtensionError` instance of `{message: "Invalid..."}`

Should also add a test for this.

::: browser/components/extensions/test/browser/browser_ext_sidebarAction_context.js:35
(Diff revisions 11 - 12)
> -        () => browser.sidebarAction.setTitle({tabId, title: "foo"}),
> -        () => browser.sidebarAction.setIcon({tabId, path: "foo.png"}),
> -        () => browser.sidebarAction.setPanel({tabId, panel: "foo.html"}),
> +        async () => { await browser.sidebarAction.setTitle({tabId, title: "foo"}); },
> +        async () => { await browser.sidebarAction.setIcon({tabId, path: "foo.png"}); },
> +        async () => { await browser.sidebarAction.setPanel({tabId, panel: "foo.html"}); },

No need for the asyncification. These APIs already return promises.

::: browser/components/extensions/test/browser/browser_ext_sidebarAction_context.js:227
(Diff revisions 11 - 12)
> -          browser.sidebarAction.setIcon({tabId, path: "2.png"});
> -          browser.sidebarAction.setPanel({tabId, panel: "2.html"});
> -          browser.sidebarAction.setTitle({tabId, title: "Title 2"});
> +          await browser.sidebarAction.setIcon({tabId, path: "2.png"});
> +          await browser.sidebarAction.setPanel({tabId, panel: "2.html"});
> +          await browser.sidebarAction.setTitle({tabId, title: "Title 2"});

Nit: `await Promise.all([...])`. Not that it matters much, but there's no need to force an entire round trip between each call. Same for below.

Comment 66

[Shane Caraveo (:mixedpuppy)]

Comment on attachment 8826753 [details]
Bug 1208596 implement sidebar api for webextensions, f?kmag, gijs

https://reviewboard.mozilla.org/r/104628/#review112586

> Need to reject with an object. Either an `ExtensionError` instance of `{message: "Invalid..."}`
> 
> Should also add a test for this.

This was good to add a test for, it actually caught a bug.  Because we used strictrelativeurl in the schema, there was no way to pass an empty string (ie. to unset the tab-specific panel).  Tests added for tab specific panel handling as well as the error case.

Comment 67

[Shane Caraveo (:mixedpuppy)]

Comment on attachment 8826753 [details]
Bug 1208596 implement sidebar api for webextensions, f?kmag, gijs

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/104628/diff/12-13/

Comment 68

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/1e0dc444f8e3
implement sidebar api for webextensions, f?kmag, gijs r=Gijs,kmag

Comment 69

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/1e0dc444f8e3

Comment 70

[Steph]

I'm late to the show unfortunately.

Does this API allow for the following use case ? It's covered by some custom config of Tree Style Tab but I'll describe it:
- Put a vertical tab bar on the left
- Make it hide itself so it takes no real estate from content at all
- When the mouse gets near the edge of the browser window, slide the bar over content, without displacing it, with a degree of transparency so we can see content below; only tabs are not transparent and they don't take the entire bar.
- When the mouse leaves, the tab bar slides back and becomes invisible again

Comment 71

[Shane Caraveo (:mixedpuppy)]

(In reply to Steph from comment #70)
> I'm late to the show unfortunately.
> 
> Does this API allow for the following use case ? It's covered by some custom
> config of Tree Style Tab but I'll describe it:
> - Put a vertical tab bar on the left

The api does not dictate what is in the sidebar, but tab management is one of the use cases considered for sidebars.

> - Make it hide itself so it takes no real estate from content at all

No, UI control remains with the user.

> - When the mouse gets near the edge of the browser window, slide the bar
> over content, without displacing it, with a degree of transparency so we can
> see content below; only tabs are not transparent and they don't take the
> entire bar.
> - When the mouse leaves, the tab bar slides back and becomes invisible again

Please file a new bug for that, could be a nice feature.  We have a tracking bug for sidebar improvements, see bug 1329022

Comment 72

[Steph]

I might do so, thanks. Since I really suck at searching Bugzilla, there's no bug related to turning Tree Style Tab into a WebExtension right ? Because while I'm at it, I could just list all UI requirements for the existence of this add-on.

> No, UI control remains with the user.

So the sliding thing could be considered, but it would not slide completely ? Isn't the "mouse getting near the edge" part user control enough ?

Comment 73

[Botond Ballo [:botond]]

(In reply to Steph from comment #72)
> Since I really suck at searching Bugzilla, there's no
> bug related to turning Tree Style Tab into a WebExtension right ? Because
> while I'm at it, I could just list all UI requirements for the existence of
> this add-on.

There is a TST issue tracking it [1], which lists prerequisite Firefox issues.

[1] https://github.com/piroor/treestyletab/issues/1224

Comment 74

[Shane Caraveo (:mixedpuppy)]

(In reply to Steph from comment #72)
> I might do so, thanks. Since I really suck at searching Bugzilla, there's no
> bug related to turning Tree Style Tab into a WebExtension right ? Because
> while I'm at it, I could just list all UI requirements for the existence of
> this add-on.
> 
> > No, UI control remains with the user.
> 
> So the sliding thing could be considered, but it would not slide completely
> ? Isn't the "mouse getting near the edge" part user control enough ?

It wouldn't be something controlled by the addon [except perhaps some option in the manifest], but rather a change in the sidebar ui.  For example, right now you can change the width down to a minimum size.  We could consider a change in that minimum or some other change in how that sizing works.  It would have to go through UX, but if the question isn't asked it wouldn't get answered.

Comment 75

[Marko Zabreznik]

Just want to drop this link as it is relevant: https://testpilot.firefox.com/experiments/tab-center

Comment 76

[Pete]

(In reply to Shane Caraveo (:mixedpuppy) from comment #17)
> (In reply to Pete from comment #16)
> > For example: if an addon has a toolbar button popup, it would be useful to
> > allow the user to toggle (or at least open) the sidebar from there too.
> 
> Excellent point, and I think we can allow that via browserAction.onClicked
> or other user initiated actions (clicking on link in panel content).

Did this get implemented by any chance? A quick skim of the linked patch didn't throw up an obvious way of opening a sidebar from a browserAction popup instead of via sidebarAction.

Comment 77

[:Gijs (he/him)]

(In reply to Pete from comment #76)
> (In reply to Shane Caraveo (:mixedpuppy) from comment #17)
> > (In reply to Pete from comment #16)
> > > For example: if an addon has a toolbar button popup, it would be useful to
> > > allow the user to toggle (or at least open) the sidebar from there too.
> > 
> > Excellent point, and I think we can allow that via browserAction.onClicked
> > or other user initiated actions (clicking on link in panel content).
> 
> Did this get implemented by any chance? A quick skim of the linked patch
> didn't throw up an obvious way of opening a sidebar from a browserAction
> popup instead of via sidebarAction.

--> ni :mixedpuppy.

Comment 78

[Shane Caraveo (:mixedpuppy)]

No, this bug does not implement support for that.  I'll have to think about how to best handle that since the mechanism I'm using to add open* APIs (nsIDOMWindowUtils.isHandlingUserInput) may not be set during that event.  See Bug 1341126.

Comment 79

[Pete]

Attachment: Sidebar from popup.png

Thanks, I didn't spot that bugzilla.

One more query: in the case where an extension is designed to open its sidebar from a browserAction popup, it will be necessary to prevent the sidebarAction icon from appearing in the toolbar. Will this be possible?

To give a concrete example: the attached mockup shows two toolbar icons added by the webextension:
 1) The one on the left is the sidebarAction
 2) The one on the right is the browserAction

Since the browserAction popup contains a button to open the sidebar, the sidebarAction icon is unnecessary and confusing.

Is this a use case that you're happy to support? Should I open a new bugzilla for this, or is there an easy way to achieve this already?

Comment 80

[Shane Caraveo (:mixedpuppy)]

This bug is closed, please use new bugs if you are looking for new features.

The sidebar will be available in the sidebar button and menu regardless, it is not decided yet by UX if the sidebar button will be moved into the toolbar.  That bit only happens in nightly/deved for now.

Comment 81

[Will Bamberg [:wbamberg]]

Docs:

https://developer.mozilla.org/en-US/Add-ons/WebExtensions/Sidebars
https://developer.mozilla.org/en-US/Add-ons/WebExtensions/API/sidebarAction
https://developer.mozilla.org/en-US/Add-ons/WebExtensions/manifest.json/sidebar_action

Please let me know if this covers it.

Comment 82

[Shane Caraveo (:mixedpuppy)]

(In reply to Will Bamberg [:wbamberg] from comment #81)
> Docs:
> 
> https://developer.mozilla.org/en-US/Add-ons/WebExtensions/Sidebars

"Note that it's not possible for add-ons to open sidebars programmatically: sidebars can only be opened by the user."

FYI Bug 1341126 will change that (as well as for page and browser actions), but will require user interaction.

> https://developer.mozilla.org/en-US/Add-ons/WebExtensions/API/sidebarAction

This probably needs some compatibility notes since Opera supports a couple more APIs than we do.  See badge related APIs at https://dev.opera.com/extensions/sidebar-action-api/

Comment 83

[Will Bamberg [:wbamberg]]

(In reply to Shane Caraveo (:mixedpuppy) from comment #82)
> (In reply to Will Bamberg [:wbamberg] from comment #81)
> 
> This probably needs some compatibility notes since Opera supports a couple
> more APIs than we do.  See badge related APIs at
> https://dev.opera.com/extensions/sidebar-action-api/

I've added a paragraph noting these omissions.