Closed Bug 1395387 Opened 7 years ago Closed 7 years ago

Reconcile WebExtension page actions and Photon page actions

Categories

(Firefox :: Address Bar, enhancement, P1)

Product:
Firefox
Component:
Address Bar
Type:
enhancement

Tracking

()

Status:
RESOLVED FIXED
Milestone:
Firefox 58
Tracking Flags:
Tracking Status
firefox57 --- wontfix
firefox58 --- fixed

People

(Reporter: adw, Assigned: adw)

References

(Depends on 1 open bug, Blocks 1 open bug)

Details

(Whiteboard: [reserve-photon-structure])

Attachments

(3 files)

Bug 1395387 - Reconcile WebExtension page actions and Photon page actions: Photon page actions changes.
59 bytes, text/x-review-board-request
Gijs
: review+
Details
Bug 1395387 - Reconcile WebExtension page actions and Photon page actions: WebExtensions changes.
59 bytes, text/x-review-board-request
mixedpuppy
: review+
Details
Bug 1395387 - Reconcile WebExtension page actions and Photon page actions: Photon page actions changes part 2, purge cache on shutdown.
59 bytes, text/x-review-board-request
mikedeboer
: review+
Details 
For Photon in 57 we added a page action panel with items that can be copied to the urlbar, but they're separate from WebExtension page actions.  WebExtension page actions get appended to the urlbar, where the Photon page actions happen to be, so it might look like they're the same, but they don't get added to the Photon panel, and they can't be removed from the urlbar in the same way.  WebExtensions that want to use a Photon page action have to do so in their bootstrap.js instead of using the WebExtension page action API, which is a pain.

We should reconcile them somehow so that page actions added with the WebExtension API are added as full-fledged Photon page actions.  Browser chrome needs access to page actions, too, which happens via PageActions.jsm, so I suggest modifying our WebExtension implementation to call into PageActions.jsm.

We should beef up the PageActions.jsm API so that it supports everything that the WebExtension implementation needs.  However, I don't think it's important to make the PageActions.jsm implement the WebExtension API, especially considering that it's used by browser chrome.  It's fine to have the WebExtension implementation as a go-between.
Whiteboard: [photon-structure] → [photon-structure] [triage]
WebExtension API is, by design, in between...
Flags: qe-verify-
Priority: -- → P4
Whiteboard: [photon-structure] [triage] → [reserve-photon-structure]
Assignee: nobody → adw
Status: NEW → ASSIGNED
Iteration: --- → 57.3 - Sep 19
Priority: P4 → P1
WIP.  I'll split this into a browser chrome PageActions patch and a WebExtension patch when it's ready for review.  I've been modifying the screenshots extension to test this manually.  Will update automated tests as necessary of course.

Done/in-progress so far:

* ext-pageAction.js creates and adds a PageActions.PageAction.

* Change PageActions.Action.icon so you can set separate 16px and 32px images (and ImageData, not yet implemented).

* Hook up the new PageAction's onCommand to ext-pageAction's handleClick method.

* Allow PageActions consumers to provide their own activated-action panel.  ext-pageAction.js can then use its own panel, but it will still be anchored correctly (depending on whether the action is in the urlbar or not), etc.
More WIP.  The main thing this does is start to add support for setting action properties per tab, since the WebExtensions API supports that.  Right now we just assume that an action's title, icon, etc. are the same across all tabs.
Hmm although now that I type that summary, that's maybe the wrong way to do this.  Instead, just give the WebExtensions implementation access to a button, a node, pretty much the same as it has now, except it wouldn't create the button.  It would set its properties the same way it's doing that now.  That would result in a two-tiered system of actions though, but maybe that's OK.  Hmm.
More WIP.  I think that building in support for per-window action properties is probably the way to go, so I'm continuing with that.

Ideally this work would make 57, but it's probably wiser to hold off until 58 or later since these are fairly extensive changes.  I'd like to make 58.
Progress on getting some of the WebExtension page action tests passing.  A few of them do so far.  Working on browser_ext_pageAction_context.js right now.  I think it's failing because hide() is actually hiding the icon in the urlbar, but right now I have that hooked up to disable it instead.  I think we need clarification from UX on whether we want that to still actually hide icons because now there's also a page action panel that lets you show/hide items in the urlbar as you'd like.  I notice that Chrome's documentation for hide() says, "Hidden page actions still appear in the Chrome toolbar, but are grayed out."
One other thing that Panos reminded me of is icon color/opacity.  The urlbar icons of Photon page actions set fill-opacity="context-fill-opacity" and fill="context-fill" in their SVG, and in browser CSS we have `fill-opacity: 0.6` and `fill: currentColor`.  That's why those icons are slightly lighter than black.  Presumably we'll want that same effect for WebExtension actions.  Not sure yet whether that deserves its own bug.  I'll think more about it.
(In reply to Drew Willcoxon :adw from comment #11)
> One other thing that Panos reminded me of is icon color/opacity.  The urlbar
> icons of Photon page actions set fill-opacity="context-fill-opacity" and
> fill="context-fill" in their SVG, and in browser CSS we have `fill-opacity:
> 0.6` and `fill: currentColor`.  That's why those icons are slightly lighter
> than black.  Presumably we'll want that same effect for WebExtension
> actions.  Not sure yet whether that deserves its own bug.  I'll think more
> about it.


The platform folks are against exposing context-fill and context-fill-opacity to extensions because it is non-standard. Atm, only mozilla signed extensions can use those keywords.
Iteration: 57.3 - Sep 19 → ---
(In reply to Tim Nguyen :ntim from comment #12)
> The platform folks are against exposing context-fill and
> context-fill-opacity to extensions because it is non-standard. Atm, only
> mozilla signed extensions can use those keywords.

That's true, thanks.  We'll have to do something else for that.
Note to self: principals are important when loading moz-extension URIs, like images.  In particular they can be loaded from content CSS but not skin CSS.  (Thanks Kris)
(In reply to Drew Willcoxon :adw from comment #13)
> (In reply to Tim Nguyen :ntim from comment #12)
> > The platform folks are against exposing context-fill and
> > context-fill-opacity to extensions because it is non-standard. Atm, only
> > mozilla signed extensions can use those keywords.
> 
> That's true, thanks.  We'll have to do something else for that.

Ah I forgot though, moz-extension URIs *are* allowed to use context fill opacity and color.  Those are the URIs I'm interested in for this bug.

And as for what to do about extensions that don't specify those in their SVG (comment 11) -- I'm sure the WebExtensions folks have already thought about that! :-)  I'd guess it's a best practice thing rather than being enforced by our extensions implementation, if that's even possible.  If you don't want your icon to look bad, then specify those attributes.
(In reply to Drew Willcoxon :adw from comment #15)
> Ah I forgot though, moz-extension URIs *are* allowed to use context fill
> opacity and color.  Those are the URIs I'm interested in for this bug.

Dang it, I'm wrong again.  It's only Mozilla-signed extensions.  I was thinking of this great code comment here: https://dxr.mozilla.org/mozilla-central/rev/db7f19e26e571ae1dd309f5d2f387b06ba670c30/layout/svg/SVGContextPaint.cpp#39
All the WebExtension page action tests and my browser page action tests pass locally with this.  It's still messy and needs to be cleaned up though.  Hopefully I can ask for review early next week.
Aaron, WebExtension page actions can hide/show their icons in the urlbar via their API, the pageAction.hide() and pageAction.show() functions.  Currently Firefox does actually hide/show icons when these functions are called.

I'm thinking we may want to change them so that they disable/enable their icons instead, since now hiding/showing page action icons in the urlbar is under the user's control.  FWIW it sounds like disabling/enabling is what Chrome does according to the Chrome doc, but I haven't tested: "You make a page action appear and be grayed out using the pageAction.show and pageAction.hide methods", "Hidden page actions still appear in the Chrome toolbar, but are grayed out."  https://developer.chrome.com/extensions/pageAction#method-hide

Do you think disabling/enabling is what hide()/show() should do now, or do you have another idea?
Flags: needinfo?(abenson)
Hmm, maybe? I need a better understanding of what this would look like in practice. It might be confusing to refer to something as "hidden" but still show it in the list (albeit disabled). 

If I have a web extension that's using the Page Action Menu (PAM), and that web extension is disabled (I assume through the web extension manager), I would see that item in the PAM but it would be disabled? If that's the case, then I should be able to enable it easily from there, too. 

Am I thinking about this right?
Flags: needinfo?(abenson) → needinfo?(adw)
(In reply to Aaron Benson from comment #21)
> Hmm, maybe? I need a better understanding of what this would look like in
> practice. It might be confusing to refer to something as "hidden" but still
> show it in the list (albeit disabled).

To be clear though only developers see these "hide" and "show" terms, not users.  It's up to each individual extension when/whether to call these functions.  For example, an RSS extension would show() itself when the current page offers an RSS feed and hide() itself otherwise.

> If I have a web extension that's using the Page Action Menu (PAM), and that
> web extension is disabled (I assume through the web extension manager), I
> would see that item in the PAM but it would be disabled?

That's a good question, and I think it's up to us.  Should it appear in the PAM?  I would think not, since traditionally when an extension is disabled in Firefox, it's like it's not even installed, except it's still listed in the extensions manager in case you want to enable it again.  I presume that WebExtensions behave the same.  So we wouldn't show disabled WebExtensions in the UI anywhere except in the extensions manager, and the PAM is not the extensions manager.

So what I'm proposing is that for an *enabled* WebExtension, its icon's disabled state in both the urlbar and PAM is up to the extension via hide()/show(), and whether it appears in the urlbar at all is up to the user.  (Which is what Chrome does, judging by their docs, but they don't have a PAM AFAICT, just the urlbar icons.)
Flags: needinfo?(adw) → needinfo?(abenson)
(In reply to Drew Willcoxon :adw from comment #22)

> So what I'm proposing is that for an *enabled* WebExtension, its icon's
> disabled state in both the urlbar and PAM is up to the extension via
> hide()/show(), and whether it appears in the urlbar at all is up to the
> user.  (Which is what Chrome does, judging by their docs, but they don't
> have a PAM AFAICT, just the urlbar icons.)

Cool, that all makes sense. Just want to clarify whether this means the WebExtension would move out of the URL bar if it was hidden, or would it appear as disabled inside the URL bar?
Flags: needinfo?(abenson) → needinfo?(adw)
Well, both... Sorry if I'm rewording myself here, but I'm not sure what you're asking, and I want to make myself clear.  I'm suggesting that we need two independent states for icons in the urlbar (for *enabled* extensions), a visibility state and an enabled state.

(1) Visibility:  Icon visibility in the urlbar is controlled by the user.  (Icons are always visible in the PAM.)  Once the user adds an icon to the urlbar, then it's always visible until the user removes it.  (Except for when page actions as a group are hidden, like on some about: pages.)  That's the behavior we have now, no different.

(2) Enabled:  This is controlled by the extension via show()/hide().  The enabled state applies to both the icon in the urlbar and the icon in the PAM.  A disabled icon basically means that an action doesn't apply to the current page.  A disabled icon is drawn with a dimmed opacity, or something like that, but it remains visible in the urlbar if the user added it to the urlbar.

That's just my suggestion.  The problem I'm trying to address is that with WebExtensions, we have two ways of "showing" and "hiding" an icon in the urlbar, one controlled by the user, one controlled by the extension.  IMO it would be confusing for the user if for the latter we actually changed icon visibility since that's what we do for the former.  But maybe I'm wrong and it's not a problem, or there's a better way to address it than what I've suggested.
Flags: needinfo?(adw) → needinfo?(abenson)
BTW we currently have the concept of disabled actions.  The Send to Device submenu is disabled on pages that aren't sendable, for example.  (There's a separate bug where the urlbar icon of a disabled action isn't disabled though, only its item in the PAM.  But that's an implementation bug I'm going to fix as part of this bug.)
That makes total sense. I just wanted to be clear that we didn't have a case of an icon dissapearing from the URL bar without direct user action. Sorry to pester you with follow-up questions, I just wanted to know for sure. 

I think what you're suggesting here sounds absolutely on point.
Flags: needinfo?(abenson)
Thanks Aaron.

The latest commit should fix the errors on try.  One thing they revealed is that WebExtension page actions can have context menus and I'm not handling that.  I'd like to fix that in a follow-up bug I think.  Right now Photon page actions also have a context menu to let you remove them from the urlbar.  I need more UX input on how to handle that, not that it should be a big deal though.
Blocks: 1386231
Attachment #8913566 - Flags: review?(amckay)
Andy, would you mind reviewing or redirecting?  I'm not sure who to ask on the WebExtensions side.
Gijs, summary of changes:

* Let actions have state per browser window in addition to global state.  Per-window state takes precedence

* De-underscore BrowserPageActions's panel-toggling method and let it take a panel so that the WebExtensions implementation can pass in its own

* Add a method to BrowserPageActions that updates an action's DOM nodes given the names of properties that have changed, or all properties

* For actions that specify icon URLs (as opposed to styling via CSS), let them give an object that maps pixel size to URL, and set a couple of CSS vars and use them to give icons to the panel and urlbar nodes depending on dppx

* Support disabled actions and their icons in the urlbar

* De-underscore a couple of BrowserPageActions methods since the WebExtensions implementation uses them (and tests too actually)
eslint fixes.
Comment on attachment 8904776 [details]
Bug 1395387 - Reconcile WebExtension page actions and Photon page actions: Photon page actions changes.

https://reviewboard.mozilla.org/r/176552/#review190172

Looks good, just some nits and a philosophical question about sorting in the face of per-window titles. Sorry for the slow review. :-(

::: browser/modules/PageActions.jsm:210
(Diff revision 7)
>        // A non-built-in action, like a non-bundled extension potentially.
>        // Keep this list sorted by title.
>        let index = BinarySearch.insertionIndexOf((a1, a2) => {
> -        return a1.title.localeCompare(a2.title);
> +        return a1.getTitle().localeCompare(a2.getTitle());

Should we sort these per-window in popupshowing when we actually show the main panel, given that the title can now be different per-window?

I could see arguments either way - it'll look messy if we sort based on a title that's not reflected in the relevant window, but equally it'll mean the sorting might be different in different windows which breaks muscle memory. Maybe we just shouldn't worry about it and leave it like this until/unless it becomes a problem?

::: browser/modules/PageActions.jsm:676
(Diff revision 7)
> +   */
> +  _getProperty(name, browserWindow) {
> +    if (browserWindow) {
> +      // Try the per-window state.
> +      let props = this._propertiesByBrowserWindow.get(browserWindow);
> +      if (props && name in props) {

Nit: `props.hasOwnProperty(name)` to avoid issues with 'special' properties (though we don't use those here).

::: browser/modules/PageActions.jsm:681
(Diff revision 7)
> +      if (props && name in props) {
> +        return props[name];
> +      }
> +    }
> +    // Fall back to the global state.
> +    return this[`_${name}`];

Nit: `"_" + name` is just as short and more performant, and property access should probably just be fast.

::: browser/base/content/browser-pageActions.js:477
(Diff revision 8)
> +      "title",
> +      "tooltip",
> +    ];
> +    for (let name of names) {
> +      let upper = name[0].toUpperCase() + name.substr(1);
> +      this[`_updateAction${upper}`](action);

Here too, because it's a prefix, could just use `"_updateAction" + upper`.
Attachment #8904776 - Flags: review?(gijskruitbosch+bugs) → review+
Attachment #8913566 - Flags: review?(amckay) → review?(mixedpuppy)
Comment on attachment 8913566 [details]
Bug 1395387 - Reconcile WebExtension page actions and Photon page actions: WebExtensions changes.

https://reviewboard.mozilla.org/r/184946/#review191166

Overall this is looking good but am concerned about icons and context menus.  I need to look at how the icons were/are working and whether this will work with theme icons per my comment.

::: browser/components/extensions/ExtensionPopups.jsm:385
(Diff revision 2)
>   * WeakMap[window -> WeakMap[Extension -> BasePopup]]
>   */
>  BasePopup.instances = new DefaultWeakMap(() => new WeakMap());
>  
>  class PanelPopup extends BasePopup {
> -  constructor(extension, imageNode, popupURL, browserStyle) {
> +  constructor(extension, document, popupURL, browserStyle) {

not important, but it might be nice to make PanelPopup and ViewPopup consistent by making the argument here window rather than document.

::: browser/components/extensions/ExtensionPopups.jsm:396
(Diff revision 2)
>      panel.setAttribute("role", "group");
>      if (extension.remote) {
>        panel.setAttribute("remote", "true");
>      }
>  
> -    document.getElementById("mainPopupSet").appendChild(panel);
> +    panel.addEventListener("popupshowing", () => {

Why is this change necessary? This seems like a change in behavior from having the content loaded (thus the browser resize) to potentially happening earlier (and potentially conflicting with test support per below).

OTOH, this also seems like it is used only by tests, I wonder if it's necessary at all given we wait on popupshown and content read in the same test function.  

http://searchfox.org/mozilla-central/source/browser/components/extensions/test/browser/head.js#207

::: browser/components/extensions/ext-pageAction.js:69
(Diff revision 2)
>        () => IconDetails.normalize({path: options.default_icon}, extension));
>  
> -    this.iconData.set(
> -      this.defaults.icon,
> -      await StartupCache.get(
> -        extension, ["pageAction", "default_icon_data"],
> +    this.browserPageAction = PageActions.addAction(new PageActions.Action({
> +      id: widgetId,
> +      title: this.defaults.title,
> +      iconURL: this.defaults.icon,

I have to think about what is going on with icons here.  Lacking the svg support, we're left with finishing the theme icon support from Bug 1398156.

::: browser/components/extensions/ext-pageAction.js:85
(Diff revision 2)
>  
>      this.tabContext.shutdown();
>  
> -    for (let window of windowTracker.browserWindows()) {
> -      if (this.buttons.has(window)) {
> -        this.buttons.get(window).remove();
> +    // Removing the browser page action causes the browser to forget about it
> +    // across app restarts, so remove it only when the extension is uninstalled
> +    // or disabled, not on a normal shutdown.

Do we need to consider ADDON_DOWNGRADE or ADDON_UPGRADE in onManifest?  It seems in this case we wont remove the page action, but we'll potentially call addAction again.

::: browser/components/extensions/ext-pageAction.js:122
(Diff revision 2)
>    // Updates the page action button in the given window to reflect the
>    // properties of the currently selected tab:
>    //
>    // Updates "tooltiptext" and "aria-label" to match "title" property.
>    // Updates "image" to match the "icon" property.
> -  // Shows or hides the icon, based on the "show" property.
> +  // Enables or disables the icon, based on the "show" property.

saw the bug comments on enable/disable, I think that it is fine to do that rather than remove.

::: browser/components/extensions/test/browser/browser_ext_menus.js:71
(Diff revision 2)
>    const tab = await BrowserTestUtils.openNewForegroundTab(gBrowser, "http://example.com/");
>  
>    await extension.startup();
>    const tabId = await extension.awaitMessage("ready");
>  
> -  for (const kind of ["page", "browser"]) {
> +  // TODO: Allow WebExtensions to hook into the browser page action context menu.

This is a regression in functionality, can we get a bug # if it's a followup, and will that land within the same fx version?
(In reply to :Gijs from comment #38)
> Looks good, just some nits and a philosophical question about sorting in the
> face of per-window titles. Sorry for the slow review. :-(

Not slow at all, thanks!

> ::: browser/modules/PageActions.jsm:210
> (Diff revision 7)
> >        // A non-built-in action, like a non-bundled extension potentially.
> >        // Keep this list sorted by title.
> >        let index = BinarySearch.insertionIndexOf((a1, a2) => {
> > -        return a1.title.localeCompare(a2.title);
> > +        return a1.getTitle().localeCompare(a2.getTitle());
> 
> Should we sort these per-window in popupshowing when we actually show the
> main panel, given that the title can now be different per-window?
> 
> I could see arguments either way

Me too.  I came down on the side of it's more important not to change the ordering of actions, but we should probably ask UX to be sure.
Thanks Shane.

(In reply to Shane Caraveo (:mixedpuppy) from comment #39)
> ::: browser/components/extensions/ExtensionPopups.jsm:396
> (Diff revision 2)
> >      panel.setAttribute("role", "group");
> >      if (extension.remote) {
> >        panel.setAttribute("remote", "true");
> >      }
> >  
> > -    document.getElementById("mainPopupSet").appendChild(panel);
> > +    panel.addEventListener("popupshowing", () => {
> 
> Why is this change necessary? This seems like a change in behavior from
> having the content loaded (thus the browser resize) to potentially happening
> earlier (and potentially conflicting with test support per below).

All the WebExtensions side has to do now is just give the Photon page actions API a panel and ask it to open it.  And then the Photon page action side will make sure it's anchored at the right place, closing the currently open panel first if there is one, setting the "open" attribute on the corresponding page action node, etc.  So the panel shouldn't be opened here anymore.

It shouldn't make content loading happen earlier, but maybe I'm not following what you mean.  The current code waits for contentReady, then opens the popup, then dispatches WebExtPopupLoaded.  My patch does the same thing but in a different way: waits for contentReady (now in handleClick), opens the popup (via BrowserPageActions.togglePanelForAction), and on popupshowing (here in ExtensionPopups.jsm) dispatches WebExtPopupLoaded.

> ::: browser/components/extensions/ext-pageAction.js:69
> (Diff revision 2)
> >        () => IconDetails.normalize({path: options.default_icon}, extension));
> >  
> > -    this.iconData.set(
> > -      this.defaults.icon,
> > -      await StartupCache.get(
> > -        extension, ["pageAction", "default_icon_data"],
> > +    this.browserPageAction = PageActions.addAction(new PageActions.Action({
> > +      id: widgetId,
> > +      title: this.defaults.title,
> > +      iconURL: this.defaults.icon,
> 
> I have to think about what is going on with icons here.  Lacking the svg
> support, we're left with finishing the theme icon support from Bug 1398156.

Not sure what you mean by lacking the svg support?

I did see that themes for icons are a todo in ext-pageAction.js but are implemented in ext-browserAction.js.  Right now, this `iconURL` property can be an object that maps pixel sizes to URIs.  I think it would be straightforward to extend that so it could include theme-specific URIs.  And then the Photon page actions stuff would probably handle that the same way that ext-browserAction.js does (setting CSS variables).

> ::: browser/components/extensions/ext-pageAction.js:85
> (Diff revision 2)
> >  
> >      this.tabContext.shutdown();
> >  
> > -    for (let window of windowTracker.browserWindows()) {
> > -      if (this.buttons.has(window)) {
> > -        this.buttons.get(window).remove();
> > +    // Removing the browser page action causes the browser to forget about it
> > +    // across app restarts, so remove it only when the extension is uninstalled
> > +    // or disabled, not on a normal shutdown.
> 
> Do we need to consider ADDON_DOWNGRADE or ADDON_UPGRADE in onManifest?  It
> seems in this case we wont remove the page action, but we'll potentially
> call addAction again.

OK, I should probably check that the action doesn't already exist before trying to add it.

> ::: browser/components/extensions/test/browser/browser_ext_menus.js:71
> (Diff revision 2)
> >    const tab = await BrowserTestUtils.openNewForegroundTab(gBrowser, "http://example.com/");
> >  
> >    await extension.startup();
> >    const tabId = await extension.awaitMessage("ready");
> >  
> > -  for (const kind of ["page", "browser"]) {
> > +  // TODO: Allow WebExtensions to hook into the browser page action context menu.
> 
> This is a regression in functionality, can we get a bug # if it's a
> followup, and will that land within the same fx version?

Yeah, I'll file a bug for it.  I'd definitely like to land it in the same version.  We can hold off landing this one until that one is ready.  It shouldn't be so hard to implement in terms of code, but I need to ask Aaron about the UX, how extension menus should integrate with the current Photon page action context menu.  But even that should be pretty simple to figure out.
(In reply to Drew Willcoxon :adw (Away at the moment) from comment #41)
> Thanks Shane.
> 
> (In reply to Shane Caraveo (:mixedpuppy) from comment #39)
> > ::: browser/components/extensions/ExtensionPopups.jsm:396
> > (Diff revision 2)
> > >      panel.setAttribute("role", "group");
> > >      if (extension.remote) {
> > >        panel.setAttribute("remote", "true");
> > >      }
> > >  
> > > -    document.getElementById("mainPopupSet").appendChild(panel);
> > > +    panel.addEventListener("popupshowing", () => {
> > 
> > Why is this change necessary? This seems like a change in behavior from
> > having the content loaded (thus the browser resize) to potentially happening
> > earlier (and potentially conflicting with test support per below).
> 
> The current code waits for contentReady, then
> opens the popup, then dispatches WebExtPopupLoaded.  My patch does the same
> thing but in a different way: waits for contentReady (now in handleClick),
> opens the popup (via BrowserPageActions.togglePanelForAction), and on
> popupshowing (here in ExtensionPopups.jsm) dispatches WebExtPopupLoaded.

Ok, makes sense now.  But the other part of my question on this...I'm wondering if the tests shouldn't just listen on popupshowing themselves, rather than having this test-support code here.  Would that be easy to do?

> > > +      iconURL: this.defaults.icon,
> > 
> > I have to think about what is going on with icons here.  Lacking the svg
> > support, we're left with finishing the theme icon support from Bug 1398156.
> 
> Not sure what you mean by lacking the svg support?

I'm referring to the support that is only allowed for our extensions.  ie. comment 11 and comment 16
If we allowed that we wouldn't need this other theme_icon stuff, but seems that possibility is a dead end.  Given several other issues with icons I need to put some time into examining it again.
(In reply to Drew Willcoxon :adw (Away at the moment) from comment #41)

> > ::: browser/components/extensions/ext-pageAction.js:85
> > (Diff revision 2)
> > >  
> > >      this.tabContext.shutdown();
> > >  
> > > -    for (let window of windowTracker.browserWindows()) {
> > > -      if (this.buttons.has(window)) {
> > > -        this.buttons.get(window).remove();
> > > +    // Removing the browser page action causes the browser to forget about it
> > > +    // across app restarts, so remove it only when the extension is uninstalled
> > > +    // or disabled, not on a normal shutdown.
> > 
> > Do we need to consider ADDON_DOWNGRADE or ADDON_UPGRADE in onManifest?  It
> > seems in this case we wont remove the page action, but we'll potentially
> > call addAction again.
> 
> OK, I should probably check that the action doesn't already exist before
> trying to add it.

Keep in mind that on upgrade or downgrade, the action may no longer be needed, the action url or other options may have changed.  If you just removed the button on up/downgrade, and it happened to be re-added right away, do we loose any state (e.g. button placement)?  If not, I think it's probably safe to just remove it.
Attachment #8913566 - Flags: review?(mixedpuppy)
Just r? me again when you're ready.
Attachment #8913566 - Flags: review?(mixedpuppy)
Just unbitrotting both patches after a couple of weeks of absence.  Still need to address Shane's comments.
Attachment #8913566 - Flags: review?(mixedpuppy)
This should fix the TV test failure on that last try push.
Mike, since Gijs is away, could you please review this new PageActions patch?  PageActions keeps a persistent cache of action IDs that it's seen and action IDs that are in the urlbar.  Currently, when an action is remove()ed, its ID is removed from that cache immediately, so if the action were to be added again, its state would be reset -- namely, its location in the urlbar would be lost.  That's what we want when an action is truly removed, for example when an extension is uninstalled.  But when an extension is upgraded or downgraded, it goes through a remove-and-add-again cycle.  Its placement in the urlbar should not be lost in that case.

So what this patch does is defer removing actions from the cache until shutdown.
Shane, this is ready for review.  It's what we talked about: always remove the browserPageAction on shutdown, and also the WebExtPopupLoaded fix.
Comment on attachment 8913566 [details]
Bug 1395387 - Reconcile WebExtension page actions and Photon page actions: WebExtensions changes.

https://reviewboard.mozilla.org/r/184946/#review198722

::: browser/components/extensions/ext-pageAction.js:70
(Diff revision 5)
>  
> -    this.iconData.set(
> -      this.defaults.icon,
> -      await StartupCache.get(
> -        extension, ["pageAction", "default_icon_data"],
> -        () => this.getIconData(this.defaults.icon)));
> +    if (!this.browserPageAction) {
> +      this.browserPageAction = PageActions.addAction(new PageActions.Action({
> +        id: widgetId,
> +        title: this.defaults.title,
> +        iconURL: this.defaults.icon,

I'm still leery of this, it doesn't seem to handle icon size like you are doing in updateButton.  How is 2x being handled?

::: browser/components/extensions/test/browser/browser_ext_menus.js:71
(Diff revision 5)
>    const tab = await BrowserTestUtils.openNewForegroundTab(gBrowser, "http://example.com/");
>  
>    await extension.startup();
>    const tabId = await extension.awaitMessage("ready");
>  
> -  for (const kind of ["page", "browser"]) {
> +  // TODO: Allow WebExtensions to hook into the browser page action context menu.

Provide an open bug # in this TODO for a followup.
(In reply to Shane Caraveo (:mixedpuppy) from comment #58)
> I'm still leery of this, it doesn't seem to handle icon size like you are
> doing in updateButton.  How is 2x being handled?

In my testing, this.defaults.icon is an object like this:

{16:"moz-extension://04c32d7f-9789-8e45-9910-9547602bf71f/icons/icon-16-v2.svg", 32:"moz-extension://04c32d7f-9789-8e45-9910-9547602bf71f/icons/icon-starred-32-v2.svg"}

i.e., a mapping from size to URI.

In the other patch that Gijs reviewed, BrowserPageActions._updateActionIconURL() calls action.iconURLForSize().  iconURLForSize is in PageActions.jsm, and most of it is actually copy-pasted from ExtensionParent.jsm: it returns the URI of the best icon to use given the passed-in size.  So _updateActionIconURL actually calls iconURLForSize twice, passing it 16 and 32.  The URIs that iconURLForSize returns are then set as `--pageAction-image-${size}px` CSS variables.

Does that answer your question?  Will this.defaults.icon ever be some other type of object?  (iconURLForSize accepts strings, too.)

> ::: browser/components/extensions/test/browser/browser_ext_menus.js:71
> Provide an open bug # in this TODO for a followup.

Will do.

Looks like there are new try failures I'll need to address too.
Depends on: 1412170
Shane, I filed bug 1412170 for the context menus.  The failures I mentioned in my previous comment were due to the WebExtPopupLoaded change: there was one other dispatch of WebExtPopupLoaded that I missed.  I updated it too, but there were still failures.  I didn't want to look into them more given what you said about doing it in a follow-up, so I reverted the WebExtPopupLoaded change altogether.
Comment on attachment 8913566 [details]
Bug 1395387 - Reconcile WebExtension page actions and Photon page actions: WebExtensions changes.

https://reviewboard.mozilla.org/r/184946/#review199074
Attachment #8913566 - Flags: review?(mixedpuppy) → review+
Blocks: 1412377
I noticed that the first patch messes up urlbar button tooltips for some of the built-in actions.  They end up being "sendToDevice-title", "copyURL-title", and "emailLink-title".  That's because these actions don't specify tooltips, so their tooltips fall back to their titles, and their titles are actually the names of attributes on the main panel whose values are the actual titles defined in DTD.  I fixed it by calling action.setTitle() instead of simply setting the label attribute on the nodes for these actions.  So now PageActions will know about the proper titles of these actions.
Comment on attachment 8922553 [details]
Bug 1395387 - Reconcile WebExtension page actions and Photon page actions: Photon page actions changes part 2, purge cache on shutdown.

https://reviewboard.mozilla.org/r/193632/#review199560

Yeah, thanks for the explainer. LGTM!
Attachment #8922553 - Flags: review?(mdeboer) → review+
Pushed by dwillcoxon@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/b5b0ac43e2e8
Reconcile WebExtension page actions and Photon page actions: Photon page actions changes. r=Gijs
https://hg.mozilla.org/integration/autoland/rev/d887685b52ae
Reconcile WebExtension page actions and Photon page actions: Photon page actions changes part 2, purge cache on shutdown. r=mikedeboer
https://hg.mozilla.org/integration/autoland/rev/9f83f552a611
Reconcile WebExtension page actions and Photon page actions: WebExtensions changes. r=mixedpuppy
Depends on: 1413574
Depends on: 1413648
Depends on: 1413676
No longer depends on: 1413676
Depends on: 1417036
Depends on: 1433223
Depends on: 1479721
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: