Closed Bug 1267124 Opened 5 years ago Closed 5 years ago

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

Categories

(WebExtensions :: Untriaged, defect)

Unspecified
Android
defect
Not set
normal

Tracking

(firefox49 fixed)

RESOLVED FIXED
mozilla49
Iteration:
49.1 - May 9
Tracking Status
firefox49 --- fixed

People

(Reporter: mattw, Assigned: mattw)

References

Details

(Whiteboard: [pageAction]triaged)

Attachments

(1 file)

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.
Iteration: 48.3 - Apr 25 → ---
Summary: [PageAction] Add support for chrome.pageAction.show → [PageAction] Add support for chrome.pageAction.show on Android
Blocks: 1267402
Whiteboard: triaged → [pageAction]triaged
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 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/
Attachment #8746738 - Attachment description: MozReview Request: Bug 1267124 - WIP Implement chrome.pageAction.show on android. f?kmag → MozReview Request: Bug 1267124 - Implement chrome.pageAction.show on android. r?kmag
Attachment #8746738 - Flags: review?(kmaglione+bmo)
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/
Assignee: nobody → mwein
Iteration: --- → 49.1 - May 9
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.
Attachment #8746738 - Flags: review?(kmaglione+bmo)
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 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/
Attachment #8746738 - Attachment description: MozReview Request: Bug 1267124 - Implement chrome.pageAction.show on android. r?kmag → MozReview Request: Bug 1267124 - Implement chrome.pageAction.show on android. r?kmag,margaret
Attachment #8746738 - Flags: review?(margaret.leibovic)
Attachment #8746738 - Flags: review?(kmaglione+bmo)
Margaret, do you think you could review the changes I've made to PageActions.jsm and PageActionLayout.java?
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 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'.
Attachment #8746738 - Flags: review?(kmaglione+bmo)
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/
Attachment #8746738 - Flags: review?(kmaglione+bmo)
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 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?
Attachment #8746738 - Flags: review?(margaret.leibovic)
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 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/
Attachment #8746738 - Flags: review?(margaret.leibovic)
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.
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?
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
(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 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!
Attachment #8746738 - Flags: review?(margaret.leibovic) → review+
Attachment #8746738 - Flags: review?(kmaglione+bmo) → review+
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 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 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 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/
Keywords: checkin-needed
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
Flags: needinfo?(mwein)
Keywords: checkin-needed
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/
I fetched and merged the latest changes so hopefully this will work now.
Flags: needinfo?(mwein)
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/7fc6b24beea4
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla49
Product: Toolkit → WebExtensions
You need to log in before you can comment on or make changes to this bug.