1267124 - [PageAction] Add support for chrome.pageAction.show on Android

Status: RESOLVED FIXED

(WebExtensions :: Untriaged, defect)

Description

[Matthew Wein [:mattw]]

This bug is meant to track the implementation and testing of Chrome.pageAction.show on android.

Firefox docs: https://developer.mozilla.org/en-US/Add-ons/WebExtensions/API/PageAction/show
Chrome docs: https://developer.chrome.com/extensions/pageAction#method-show

Notes: We should be able to implement this method by wrapping `PageActions.add` in the existing PageActions.jsm module.

Comment 1

[Matthew Wein [:mattw]]

Attachment: MozReview Request: Bug 1267124 - Implement chrome.pageAction.show on android. r?kmag,margaret

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

Comment 2

[Matthew Wein [:mattw]]

Comment on attachment 8746738 [details]
MozReview Request: Bug 1267124 - Implement chrome.pageAction.show on android. r?kmag,margaret

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

Comment 3

[Matthew Wein [:mattw]]

Comment on attachment 8746738 [details]
MozReview Request: Bug 1267124 - Implement chrome.pageAction.show on android. r?kmag,margaret

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

Comment 4

[Matthew Wein [:mattw]]

Comment on attachment 8746738 [details]
MozReview Request: Bug 1267124 - Implement chrome.pageAction.show on android. r?kmag,margaret

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

Comment 5

[Kris Maglione [:kmag]]

Comment on attachment 8746738 [details]
MozReview Request: Bug 1267124 - Implement chrome.pageAction.show on android. r?kmag,margaret

https://reviewboard.mozilla.org/r/49559/#review46833

The WebExtension parts look pretty good to me, but I'm not a Fennec peer, so please ask someone on the Android team to review those parts.

::: mobile/android/components/extensions/ext-pageAction.js:1
(Diff revision 4)
> +/* -*- Mode: indent-tabs-mode: nil; js-indent-level: 2 -*- */

We need a .eslintrc here that extends the one from our toolkit components directory.

::: mobile/android/components/extensions/ext-pageAction.js:6
(Diff revision 4)
> +/* -*- Mode: indent-tabs-mode: nil; js-indent-level: 2 -*- */
> +/* vim: set sts=2 sw=2 et tw=80: */
> +"use strict";
> +
> +Cu.import("resource://gre/modules/Task.jsm");
> +Cu.import("resource://gre/modules/ExtensionUtils.jsm");

These imports aren't used.

::: mobile/android/components/extensions/ext-pageAction.js:9
(Diff revision 4)
> +
> +Cu.import("resource://gre/modules/Task.jsm");
> +Cu.import("resource://gre/modules/ExtensionUtils.jsm");
> +
> +// Import the android PageActions module.
> +Cu.import("resource://gre/modules/PageActions.jsm");

Please use `defineLazyModuleGetter` here.

::: mobile/android/components/extensions/schemas/page_action.json:18
(Diff revision 4)
> +            "type": "object",
> +            "additionalProperties": { "$ref": "UnrecognizedProperty" },
> +            "properties": {
> +              "default_title": {
> +                "type": "string",
> +                "unsupported": true,

Your code uses this property, so it shouldn't be marked unsupported.

::: mobile/android/components/extensions/test/mochitest/.eslintrc:2
(Diff revision 4)
> +{
> +  "extends": "../../../../../../testing/mochitest/mochitest.eslintrc",

We'd be better off extending the toolkit webextensions mochitest RC here.

::: mobile/android/components/extensions/test/mochitest/test_ext_pageAction.html:20
(Diff revision 4)
> +function backgroundScript() {
> +  browser.test.assertTrue("pageAction" in browser, "pageAction exists in browser");
> +  browser.test.assertTrue("show" in browser.pageAction, "show exists in browser.pageAction");
> +
> +  // TODO: Use the Tabs API to obtain the tab ids for showing pageActions.
> +  browser.pageAction.show(1);

We need to check that this is actually shown, and also that it's removed when the extension unloads.

Comment 6

[Kris Maglione [:kmag]]

https://reviewboard.mozilla.org/r/49559/#review46839

::: mobile/android/components/extensions/moz.build:11
(Diff revision 4)
> +
> +JAR_MANIFESTS += ['jar.mn']
> +
> +DIRS += ['schemas']
> +
> +MOCHITEST_MANIFESTS += ['test/mochitest/mochitest.ini']

Oh, and we should really probably be using chrome mochitests here.

Comment 7

[Matthew Wein [:mattw]]

Comment on attachment 8746738 [details]
MozReview Request: Bug 1267124 - Implement chrome.pageAction.show on android. r?kmag,margaret

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

Comment 8

[Matthew Wein [:mattw]]

Margaret, do you think you could review the changes I've made to PageActions.jsm and PageActionLayout.java?

Comment 9

[Matthew Wein [:mattw]]

https://reviewboard.mozilla.org/r/49559/#review47091

::: mobile/android/components/extensions/test/mochitest/.eslintrc:8
(Diff revision 5)
> +
> +  "env": {
> +    "webextensions": true,
> +  },
> +
> +  "globals": {

I forgot to add `isPageActionShown` to the list of globals here.  I'll add that in with my next revision.

::: mobile/android/modules/PageActions.jsm:81
(Diff revision 5)
> +        item.isShownCallback(false);
> +      }
> +    }
> +  },
> +
> +  isShown: function(id, callback) {

Margaret, I'm not sure if the changes to PageActionLayout.java are necessary. If you don't think so, I could remove them and simplify `isShown` to just:

function(id) {
  return !!this._items[id];
}

What do you think?

Comment 10

[Kris Maglione [:kmag]]

Comment on attachment 8746738 [details]
MozReview Request: Bug 1267124 - Implement chrome.pageAction.show on android. r?kmag,margaret

https://reviewboard.mozilla.org/r/49559/#review47145

::: mobile/android/components/extensions/.eslintrc:2
(Diff revision 5)
> +{
> +  "extends": "../../../../toolkit/components/extensions/.eslintrc"

Missing comma.

::: mobile/android/components/extensions/.eslintrc:5
(Diff revision 5)
> +{
> +  "extends": "../../../../toolkit/components/extensions/.eslintrc"
> +
> +  "env": {
> +    "webextensions": true,

I'm pretty sure we don't want this here.

::: mobile/android/components/extensions/ext-pageAction.js:18
(Diff revision 5)
> +  this.options = {
> +    title: options.default_title || extension.name,
> +    // TODO: handle setting the icon when one isn't provided.
> +    icon: options.default_icon,
> +    id: extension.id,
> +  };

Please set `this.id` to `null`.

::: mobile/android/components/extensions/ext-pageAction.js:24
(Diff revision 5)
> +}
> +
> +PageAction.prototype = {
> +  show(tabId) {
> +    // TODO: Only show the PageAction for the tab with the provided tabId.
> +    this.id = PageActions.add(this.options);

Is this going to cause problems if the PageAction is already shown at this point?

::: mobile/android/components/extensions/ext-pageAction.js:29
(Diff revision 5)
> +    this.id = PageActions.add(this.options);
> +  },
> +
> +  shutdown() {
> +    if (this.id) {
> +      PageActions.remove(this.id);

Should null out the ID here.

::: mobile/android/components/extensions/schemas/page_action.json:1
(Diff revision 5)
> +// Copyright (c) 2012 The Chromium Authors. All rights reserved.

Can you use `hg cp` to create this file from the one in `browser/` so we get a shared history, and better diff?

::: mobile/android/components/extensions/test/mochitest/.eslintrc:2
(Diff revision 5)
> +{
> +  "extends": "../../../../../../testing/mochitest/mochitest.eslintrc",

This needs to extend the .eslintrc from `toolkit/components/extensions/test/mochitest`, not the one from `testing/`.

::: mobile/android/components/extensions/test/mochitest/head.js:6
(Diff revision 5)
> +"use strict";
> +
> +/* exported isPageActionShown */
> +
> +const {
> +  utils: Cu,

Please define `Cc`, `Ci`, and `Cr` here too. And we generally keep these declarations on one line.

::: mobile/android/components/extensions/test/mochitest/test_ext_pageAction.html:30
(Diff revision 5)
> +
> +  browser.test.notifyPass("page-action");
> +}
> +
> +let extensionData = {
> +  background: "(" + backgroundScript.toString() + ")()",

You can just use `background: backgroundScript` here. The stringification is only necessary for plain mochitests.

::: mobile/android/components/extensions/test/mochitest/test_ext_pageAction.html:34
(Diff revision 5)
> +let extensionData = {
> +  background: "(" + backgroundScript.toString() + ")()",
> +  manifest: {
> +    "name": "PageAction Extension",
> +    "page_action": {
> +      "default_icon": "foo/bar.png",

We should probably include a valid image file for this. I'm not we should rely on handling invalid images gracefully.

::: mobile/android/components/extensions/test/mochitest/test_ext_pageAction.html:47
(Diff revision 5)
> +
> +  yield extension.startup();
> +  yield extension.awaitMessage("page-action-shown");
> +
> +  let isShown = false;
> +  isShown = yield isPageActionShown(extension.id);

`let isShown = yield ...`

::: mobile/android/components/extensions/test/mochitest/test_ext_pageAction.html:48
(Diff revision 5)
> +  yield extension.startup();
> +  yield extension.awaitMessage("page-action-shown");
> +
> +  let isShown = false;
> +  isShown = yield isPageActionShown(extension.id);
> +  is(true, isShown, "The PageAction should be shown");

The expected value should be the second argument. Same for the check below.

::: mobile/android/components/moz.build:44
(Diff revision 5)
>  EXTRA_PP_COMPONENTS += [
>      'MobileComponents.manifest',
>  ]
>  
> +DIRS += [
> +    'extensions'

Please add a trailing comma.

::: mobile/android/components/moz.build:47
(Diff revision 5)
>  
> +DIRS += [
> +    'extensions'
> +]
> +
>  DIRS += ['build']

This should go in the same list where you add 'extensions'.

Comment 11

[Matthew Wein [:mattw]]

Comment on attachment 8746738 [details]
MozReview Request: Bug 1267124 - Implement chrome.pageAction.show on android. r?kmag,margaret

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

Comment 12

[Matthew Wein [:mattw]]

https://reviewboard.mozilla.org/r/49559/#review47145

> Is this going to cause problems if the PageAction is already shown at this point?

Good catch, thanks.

> Can you use `hg cp` to create this file from the one in `browser/` so we get a shared history, and better diff?

Sure, will do.

> You can just use `background: backgroundScript` here. The stringification is only necessary for plain mochitests.

I don't know, for some reason the tests wouldn't run unless I stringified the background script. I looked a few of the chrome tests in toolkit and they also stringify the background script.

> We should probably include a valid image file for this. I'm not we should rely on handling invalid images gracefully.

I'm not exactly sure what the right way to support `default_icon` is yet with the way android handles icons. I think it makes sense to work on this on it's own, so I created a separate bug for tracking this (Bug 1270742) and for now just set the icon to the default extension SVG icon.

Comment 13

[:Margaret Leibovic]

Comment on attachment 8746738 [details]
MozReview Request: Bug 1267124 - Implement chrome.pageAction.show on android. r?kmag,margaret

https://reviewboard.mozilla.org/r/49559/#review47753

This generally looks fine to me, but I think we should delay intialization before landing.

::: mobile/android/chrome/content/browser.js:395
(Diff revision 6)
>  
> +    // Register extension source files.
> +    ExtensionManagement.registerScript("chrome://browser/content/ext-pageAction.js");
> +
> +    // Register extension schemas.
> +    ExtensionManagement.registerSchema("chrome://browser/content/schemas/page_action.json");

Can we be lazier about registering these? Ideally, it would be great to avoid doing this at all at startup, but it would be better if we could even do this down below in our delayed startup function.

::: mobile/android/components/extensions/ext-pageAction.js:15
(Diff revision 6)
> +var pageActionMap = new WeakMap();
> +
> +function PageAction(options, extension) {
> +  this.id = null;
> +
> +  let DEFAULT_ICON = "chrome://browser/content/extension.svg";

Do svg icons actually work in our native UI?

Comment 14

[Matthew Wein [:mattw]]

https://reviewboard.mozilla.org/r/49559/#review47753

Thanks for the review!

> Can we be lazier about registering these? Ideally, it would be great to avoid doing this at all at startup, but it would be better if we could even do this down below in our delayed startup function.

Sounds good. I moved it into the `BrowserApp_delayedStartup` function and then called it like so:

      InitLater(() => {
        Cu.import("resource://gre/modules/ExtensionManagement.jsm");

        // Register extension source files.
        ExtensionManagement.registerScript("chrome://browser/content/ext-pageAction.js");

        // Register extension schemas.
        ExtensionManagement.registerSchema("chrome://browser/content/schemas/page_action.json");
      });
      
Will this do the trick?

> Do svg icons actually work in our native UI?

I'm sorry, I shouldn't have assumed it would work. I only tested that the icon loaded when I visited chrome://browser/content/extension.svg. You're right, the icon doesn't display where the PageAction should be.

I'm not really sure then what to use for a default icon since this is the only one I could find. Do you have any ideas?

Comment 15

[Matthew Wein [:mattw]]

Comment on attachment 8746738 [details]
MozReview Request: Bug 1267124 - Implement chrome.pageAction.show on android. r?kmag,margaret

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

Comment 16

[Kris Maglione [:kmag]]

https://reviewboard.mozilla.org/r/49559/#review47753

> Sounds good. I moved it into the `BrowserApp_delayedStartup` function and then called it like so:
> 
>       InitLater(() => {
>         Cu.import("resource://gre/modules/ExtensionManagement.jsm");
> 
>         // Register extension source files.
>         ExtensionManagement.registerScript("chrome://browser/content/ext-pageAction.js");
> 
>         // Register extension schemas.
>         ExtensionManagement.registerSchema("chrome://browser/content/schemas/page_action.json");
>       });
>       
> Will this do the trick?

Registering the scripts doesn't actually cause them to be loaded, it just adds them to the list of scripts to loaded when we have to start up an extension. Delaying registering them won't cause them to be loaded any later, but it may lead to a race which causes them to not be loaded at all.

> I'm sorry, I shouldn't have assumed it would work. I only tested that the icon loaded when I visited chrome://browser/content/extension.svg. You're right, the icon doesn't display where the PageAction should be.
> 
> I'm not really sure then what to use for a default icon since this is the only one I could find. Do you have any ideas?

We need to support SVGs either way, since that's what we've been pushing people to use in order to support all the different icon sizes we (and other browsers) require. If they don't work, we can always rasterize them and replace them with a data: URL when they're needed.

Comment 17

[Matthew Wein [:mattw]]

https://reviewboard.mozilla.org/r/49559/#review47753

> Registering the scripts doesn't actually cause them to be loaded, it just adds them to the list of scripts to loaded when we have to start up an extension. Delaying registering them won't cause them to be loaded any later, but it may lead to a race which causes them to not be loaded at all.

Ah, okay, so this doesn't seem like it's necessary nor a good idea.  And since the `ExtensionManagement` module was already lazy loaded, everyone should be fine where it was?

> We need to support SVGs either way, since that's what we've been pushing people to use in order to support all the different icon sizes we (and other browsers) require. If they don't work, we can always rasterize them and replace them with a data: URL when they're needed.

That sounds good to me, but I'd like to do all of the icon support in Bug 1270742. Do you know what I could use as a placeholder?

Comment 18

[Matthew Wein [:mattw]]

https://reviewboard.mozilla.org/r/49559/#review47753

> Ah, okay, so this doesn't seem like it's necessary nor a good idea.  And since the `ExtensionManagement` module was already lazy loaded, everyone should be fine where it was?

everyone -> everything

Comment 19

[:Margaret Leibovic]

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

> > I'm sorry, I shouldn't have assumed it would work. I only tested that the icon loaded when I visited chrome://browser/content/extension.svg. You're right, the icon doesn't display where the PageAction should be.
> > 
> > I'm not really sure then what to use for a default icon since this is the only one I could find. Do you have any ideas?
> 
> We need to support SVGs either way, since that's what we've been pushing
> people to use in order to support all the different icon sizes we (and other
> browsers) require. If they don't work, we can always rasterize them and
> replace them with a data: URL when they're needed.

You will need to create a data: URL in this case, since SVG is not supported in native Android views. We filed bug 1158250 about this a while back, but it hasn't been a priority to address it.

Comment 20

[:Margaret Leibovic]

Comment on attachment 8746738 [details]
MozReview Request: Bug 1267124 - Implement chrome.pageAction.show on android. r?kmag,margaret

https://reviewboard.mozilla.org/r/49559/#review48145

::: mobile/android/chrome/content/browser.js:528
(Diff revision 7)
> +
> +        // Register extension source files.
> +        ExtensionManagement.registerScript("chrome://browser/content/ext-pageAction.js");
> +
> +        // Register extension schemas.
> +        ExtensionManagement.registerSchema("chrome://browser/content/schemas/page_action.json");

You should follow Kris's advice to register these earlier. I'm happy to hear this doesn't actually load anything until there's an add-on!

Comment 21

[Kris Maglione [:kmag]]

Comment on attachment 8746738 [details]
MozReview Request: Bug 1267124 - Implement chrome.pageAction.show on android. r?kmag,margaret

https://reviewboard.mozilla.org/r/49559/#review48201

Looks good to me, once you address Margaret's comments.

For now, you should probably just use a static data: URL for the default icon. Here's a 36×36 version:

    data:image/png;base64,iVBORw0KGgoAAAANSUhEUgAAACQAAAAkCAYAAADhAJiYAAAC4klEQVRYhdWXLWzbQBSADQtDAwsHC1tUhUxqfL67lk2tdn+OJg0ODU0rLByqgqINBY6tmlbn7LMTJ5FaFVVBk1G0oUGjG2jT2Y7jxmmcbU/6iJ+f36fz+e5sGP9riCGm9hB37RG+scd4Yo/wsDXCZyIE2xuXsce4bY+wXkAsQtzYmExrfFgvkJkRbkzo1ehoxx5iXcgI/9iYUGt8WH9MqDXEcmNChmEYrRCf2SHWeYgQx3x0tLNRIeKQLTtEFyJEep4NTuhk8BC+yMrwEE3+iozo42d8gK7FAOkMsRiiN8QhW2ttSK5QTfRRV4QoymVeJMvPvDp7gCZigD613MN6yRFA3SWarow9QB9LCfG+NeF9qCtjAKOSQjCqVKhfVsiHEQ+grgx/lRGqUihAc1uL8EFD+KCRO+GrF4J61phcoRoPoEzkYhZYpykh5sMb7kOdIeY+jHKur4QI4Feh4AFX1nVeLxrAvQchGsBz5ls6wa2QdwcvIcE2863bTH79KOvsz/uUYJsp+J0pSzNlDckVqqVGUAF+n6uS7txcOl6wot4JVy70ufDLy4pWLUQVPE81pRI0mGe9oxLMHSeohHvMs/STUNaUK6vDPCvOyxMFDx4achehRDJmHnydnkPww5OFfLxrGIZBFDyYl4LpMzlTQFIP6AQx86w2UeYBccFpJrcKv5L9eGDtUAU6RIELqsB74uynjy/UBRF1gS5BTFxwQT1wTiXoUg9MH7m/3NZRRoi5IJytUbMgzv4Wc832+oQkiKgEehmyMkkpKsFkQV11QsRJL5rJYBLItQgRaUZEmnoZXsomz3vGiWw+I9KMF9SVFOqZEemZekli1jN3U/UOqhHHvC6oWWGElhfSpGdOk6+O9prdwvtLj5BjRsQxdRnot+Zeifpy/2/0stktKTRNLmbk0mwXyl8253fyojj+8rxOHNAhjjm5n0/5OOCGOKBzkrMO0Z75lvSAzKlrF32Z/3z8BqLAn+yMV7VhAAAAAElFTkSuQmCC

Comment 22

[Matthew Wein [:mattw]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=ecb75bffbf82

Comment 23

[Matthew Wein [:mattw]]

Comment on attachment 8746738 [details]
MozReview Request: Bug 1267124 - Implement chrome.pageAction.show on android. r?kmag,margaret

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

Comment 24

[Matthew Wein [:mattw]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=7c280aae6be7

Comment 25

[Matthew Wein [:mattw]]

Comment on attachment 8746738 [details]
MozReview Request: Bug 1267124 - Implement chrome.pageAction.show on android. r?kmag,margaret

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

Comment 26

[Matthew Wein [:mattw]]

Comment on attachment 8746738 [details]
MozReview Request: Bug 1267124 - Implement chrome.pageAction.show on android. r?kmag,margaret

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

Comment 27

[Carsten Book [:Tomcat]]

has problems to apply:

applying b0887f529bfb
patching file mobile/android/chrome/content/browser.js
Hunk #2 FAILED at 382
1 out of 2 hunks FAILED -- saving rejects to file mobile/android/chrome/content/browser.js.rej
patch failed to apply
abort: fix up the working directory and run hg transplant --continue

Comment 28

[Matthew Wein [:mattw]]

Comment on attachment 8746738 [details]
MozReview Request: Bug 1267124 - Implement chrome.pageAction.show on android. r?kmag,margaret

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

Comment 29

[Matthew Wein [:mattw]]

I fetched and merged the latest changes so hopefully this will work now.

Comment 30

[Pulsebot]

https://hg.mozilla.org/integration/fx-team/rev/7fc6b24beea4

Comment 31

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/7fc6b24beea4