Closed Bug 1331742 Opened 7 years ago Closed 7 years ago

Add support for browserAction.onClicked

Categories

(WebExtensions :: Android, defect, P2)

Product:
WebExtensions
Component:
Android
Platform:
Unspecified
Android
Type:
defect

Tracking

(firefox55 fixed, firefox56 verified)

Status:
VERIFIED FIXED
Milestone:
mozilla55
Tracking Flags:
Tracking Status
firefox55 --- fixed
firefox56 --- verified
Blocking Flags:
webextensions +

People

(Reporter: mattw, Assigned: mattw)

References

Details

(Keywords: dev-doc-complete, Whiteboard: [browserAction]triaged)

Attachments

(7 files)

Bug 1331742 - Part 1 - Create and register browser_action.json
59 bytes, text/x-review-board-request
mixedpuppy
: review+
Details
Bug 1331742 - Part 2 - Create a module for managing browser actions similar to PageActions.jsm
59 bytes, text/x-review-board-request
sebastian
: review+
Details
Bug 1331742 - Part 3 - Create and register ext-browserAction.js
59 bytes, text/x-review-board-request
mixedpuppy
: review+
Details
Bug 1331742 - Part 4 - Add support for adding and removing browser actions from the main menu
59 bytes, text/x-review-board-request
sebastian
: review+
Details
Bug 1331742 - Part 5 - Add unit tests for browserAction.onClicked
59 bytes, text/x-review-board-request
mixedpuppy
: review+
Details
bug-1331742-browseraction-onclicked-0.1.xpi
4.94 KB, application/x-xpinstall
Details
browserAction-onClick.gif
1.26 MB, image/gif
Details 
MDN documentation: https://developer.mozilla.org/en-US/Add-ons/WebExtensions/API/BrowserAction/onClicked

This bug will involve adding icons to the UI.
Assignee: nobody → mwein
Blocks: 1330159
No longer blocks: 1331126
Actually, I'm going to clear review until I have tests written, because I think there are some edge cases I'm missing to support multiple browser actions.
Attachment #8848217 - Flags: review?(mixedpuppy)
Attachment #8848218 - Flags: review?(gbrown)
Attachment #8848219 - Flags: review?(mixedpuppy)
Attachment #8848220 - Flags: review?(lgreco)
Comment on attachment 8848217 [details]
Bug 1331742 - Part 1 - Create and register browser_action.json

https://reviewboard.mozilla.org/r/121154/#review123162

::: mobile/android/components/extensions/schemas/browser_action.json:4
(Diff revision 1)
> +// Copyright (c) 2012 The Chromium Authors. All rights reserved.
> +// Use of this source code is governed by a BSD-style license that can be
> +// found in the LICENSE file.
> +

this old dog learned a new trick today.

hg cp from browser/components/extensions/schemas/browser_action.json

then make any changes necessary
Attachment #8848217 - Flags: review?(mixedpuppy)
Comment on attachment 8848217 [details]
Bug 1331742 - Part 1 - Create and register browser_action.json

https://reviewboard.mozilla.org/r/121154/#review123162

> this old dog learned a new trick today.
> 
> hg cp from browser/components/extensions/schemas/browser_action.json
> 
> then make any changes necessary

+1
Attachment #8848217 - Flags: review?(mixedpuppy)
Comment on attachment 8848218 [details]
Bug 1331742 - Part 2 - Create a module for managing browser actions similar to PageActions.jsm

https://reviewboard.mozilla.org/r/121156/#review126356

Sorry, I'm not comfortable reviewing these patches. Perhaps try :sebastian?

(I usually work on test infrastructure and test code these days.)
Attachment #8848218 - Flags: review?(gbrown)
Comment on attachment 8848220 [details]
Bug 1331742 - Part 4 - Add support for adding and removing browser actions from the main menu

https://reviewboard.mozilla.org/r/121160/#review126360
Attachment #8848220 - Flags: review?(gbrown)
No worries, thanks for the quick response. Sebastian, would you be comfortable reviewing Part 2 and Part 4?
Flags: needinfo?(s.kaspari)
webextensions: --- → ?
webextensions: ? → +
(In reply to Matthew Wein [:mattw] from comment #21)
> No worries, thanks for the quick response. Sebastian, would you be
> comfortable reviewing Part 2 and Part 4?

Yeah, I can do that. I assign me in mozreview.

I just glimpsed at the patch - is it correct that we are going to add those actions to the main menu (instead of the toolbar like on desktop)?
Flags: needinfo?(s.kaspari)
Attachment #8848218 - Flags: review?(s.kaspari)
Attachment #8848220 - Flags: review?(s.kaspari)
Comment on attachment 8848217 [details]
Bug 1331742 - Part 1 - Create and register browser_action.json

https://reviewboard.mozilla.org/r/121154/#review128722

::: mobile/android/components/extensions/schemas/browser_action.json:23
(Diff revision 2)
>                  "optional": true,
>                  "preprocess": "localize"
>                },
>                "default_icon": {
>                  "$ref": "IconPath",
> +                "unsupported": true,

IIRC unsupported causes an error during manifest parsing and the addon wont load.  This will prevent the same addon from working in both desktop and android.  Using "deprecated" with a message might be better:

"deprecated": "this option is not supported on android"

Another alternative might be to remove unsupported properties and ignore unknown properties.

::: mobile/android/components/extensions/schemas/browser_action.json:80
(Diff revision 2)
>        }
>      ],
>      "functions": [
>        {
>          "name": "setTitle",
> +        "unsupported": true,

What is the reason for not supporting set/getTitle?  Same for set/getPanel.
Attachment #8848217 - Flags: review?(mixedpuppy)
Comment on attachment 8851329 [details]
Bug 1331742 - Part 5 - Add unit tests for browserAction.onClicked

https://reviewboard.mozilla.org/r/123650/#review128728

r+ with comments addressed

::: mobile/android/components/extensions/test/mochitest/head.js:27
(Diff revision 1)
>        ok(false, `Test left extra windows or tabs: ${JSON.stringify(results)}\n`);
>      }
>    });
>  }
>  
> -function isPageActionShown(uuid) {
> +function clickBrowserAction(uuid) {

These helper functions seem very redundant, can we just call them directly in the tests?

::: mobile/android/components/extensions/test/mochitest/test_ext_browserAction.html:28
(Diff revision 1)
> +    browser.test.sendMessage("browser-action-clicked", tab);
> +  });
> +
> +  const extensionInfo = {
> +    // Extract the assigned uuid from the background page url.
> +    uuid: `{${window.location.hostname}}`,

You don't need to pass uuid here, get it off extension in the tests.

::: mobile/android/components/extensions/test/mochitest/test_ext_browserAction.html:48
(Diff revision 1)
> +    },
> +  });
> +}
> +
> +function* checkBrowserAction(extension, {uuid, tab}) {
> +  ok(isBrowserActionShown(uuid), "The BrowerAction should be shown");

extension.uuid
Attachment #8851329 - Flags: review?(mixedpuppy) → review+
Comment on attachment 8848219 [details]
Bug 1331742 - Part 3 - Create and register ext-browserAction.js

https://reviewboard.mozilla.org/r/121158/#review128732

This generally looks fine, but I'm wondering about adding/removing active tab permission which is done on the desktop.
Comment on attachment 8848219 [details]
Bug 1331742 - Part 3 - Create and register ext-browserAction.js

https://reviewboard.mozilla.org/r/121158/#review128732

I believe the active tab permission on desktop is only used for popups - http://searchfox.org/mozilla-central/source/browser/components/extensions/ext-browserAction.js#233

I think we'll need to do the same on Android when we add support for popups, but this bug doesn't cover that.
Comment on attachment 8848217 [details]
Bug 1331742 - Part 1 - Create and register browser_action.json

https://reviewboard.mozilla.org/r/121154/#review128722

> IIRC unsupported causes an error during manifest parsing and the addon wont load.  This will prevent the same addon from working in both desktop and android.  Using "deprecated" with a message might be better:
> 
> "deprecated": "this option is not supported on android"
> 
> Another alternative might be to remove unsupported properties and ignore unknown properties.

That's a good point. I think adding `onError: "warn"` to the unsupported properties would be good for this - http://searchfox.org/mozilla-central/source/toolkit/components/extensions/Schemas.jsm#1516

> What is the reason for not supporting set/getTitle?  Same for set/getPanel.

Popups won't be supported in the first version so we don't need set/getPopup yet, and bug 1351111 is for tracking adding support for set/getTitle.
Comment on attachment 8848218 [details]
Bug 1331742 - Part 2 - Create a module for managing browser actions similar to PageActions.jsm

https://reviewboard.mozilla.org/r/121156/#review129422
Attachment #8848218 - Flags: review?(s.kaspari) → review+
Comment on attachment 8848220 [details]
Bug 1331742 - Part 4 - Add support for adding and removing browser actions from the main menu

https://reviewboard.mozilla.org/r/121160/#review129424
Attachment #8848220 - Flags: review?(s.kaspari) → review+
Comment on attachment 8851329 [details]
Bug 1331742 - Part 5 - Add unit tests for browserAction.onClicked

https://reviewboard.mozilla.org/r/123650/#review128728

> These helper functions seem very redundant, can we just call them directly in the tests?

Yeah, I think that's a good idea.

> You don't need to pass uuid here, get it off extension in the tests.

Interesting,I think I tried this a while ago and had issues but maybe it will work now. I'll try this and do another try run.
Comment on attachment 8848217 [details]
Bug 1331742 - Part 1 - Create and register browser_action.json

https://reviewboard.mozilla.org/r/121154/#review130082

Sorry, second guessing myself on the unsupported issue.  Lets chat about that before I give r+

::: mobile/android/components/extensions/schemas/browser_action.json:31
(Diff revisions 2 - 4)
>                },
>                "default_popup": {
>                  "type": "string",
>                  "format": "relativeUrl",
>                  "unsupported": true,
> +                "onError": "warn",

I know I said to do this before, but now I wonder.  If only onclicked is supported, this may need to be an error anyway.

::: mobile/android/components/extensions/schemas/browser_action.json:196
(Diff revisions 2 - 4)
>          ]
>        },
>        {
>          "name": "setPopup",
>          "unsupported": true,
> +        "onError": "warn",

Again here, maybe we shouldn't use onError (getPopup as well).  The other issue, with all the functions, is what happens with onError=warn but the function is not implemented.

::: mobile/android/components/extensions/schemas/browser_action.json:431
(Diff revisions 2 - 4)
>            }
>          ]
>        },
>        {
>          "name": "openPopup",
>          "unsupported": true,

I'm not sure what to do here.  openPopup will be implemented (I have a patch for it) but if it's not supported on android, should it be an error, or should it be a warning?  I'll address it with that patch though.
Attachment #8848217 - Flags: review?(mixedpuppy)
Comment on attachment 8848219 [details]
Bug 1331742 - Part 3 - Create and register ext-browserAction.js

https://reviewboard.mozilla.org/r/121158/#review130084

r+ given we address the schema issues and we dont need further changes here.
Attachment #8848219 - Flags: review?(mixedpuppy) → review+
Comment on attachment 8848217 [details]
Bug 1331742 - Part 1 - Create and register browser_action.json

https://reviewboard.mozilla.org/r/121154/#review130090

After discussion this is what we'll do.  hard error on anything that requires popup (can change in the bug for implementing popup if it happens).  warn on anything that can be used on combination with onclicked browser actions but is not supported on android.  r+ with those changes.
Attachment #8848217 - Flags: review+
> After discussion this is what we'll do.  hard error on anything that
> requires popup (can change in the bug for implementing popup if it happens).
> warn on anything that can be used on combination with onclicked browser
> actions but is not supported on android.  r+ with those changes.

I'm not sure if we can easily warn for functions that are marked as unsupported on Android, because I don't believe these functions are injected into the namespace - I think we would need to inject some sort of method stub for these functions which would log a warning whenever they are called. What do you think?
Sebastian, I forgot to update my local commit messages so I lost your r+'s when I uploaded a new patch. Could you re-r+ them please? I haven't changed the code you reviewed. Also, I updated my local commit messages to "r?sebastian" but that doesn't seem to be triggering you for review - is there a different keyword I should use?
Flags: needinfo?(s.kaspari)
Comment on attachment 8848218 [details]
Bug 1331742 - Part 2 - Create a module for managing browser actions similar to PageActions.jsm

https://reviewboard.mozilla.org/r/121156/#review131286
Attachment #8848218 - Flags: review+
Comment on attachment 8848220 [details]
Bug 1331742 - Part 4 - Add support for adding and removing browser actions from the main menu

https://reviewboard.mozilla.org/r/121160/#review131288
Attachment #8848220 - Flags: review+
(In reply to Matthew Wein [:mattw] from comment #56)
> Sebastian, I forgot to update my local commit messages so I lost your r+'s
> when I uploaded a new patch. Could you re-r+ them please?

Done!

(In reply to Matthew Wein [:mattw] from comment #56)
> I haven't changed the code you reviewed. Also, I updated my local commit messages to
> "r?sebastian" but that doesn't seem to be triggering you for review - is
> there a different keyword I should use?

No, that's correct. Might be worth filing a mozreview bug for that.
Flags: needinfo?(s.kaspari)
Keywords: checkin-needed
Pushed by ihsiao@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/32ab1a3d736f
Part 1 - Create and register browser_action.json r=mixedpuppy
https://hg.mozilla.org/integration/autoland/rev/c91b14d9ad2a
Part 2 - Create a module for managing browser actions similar to PageActions.jsm r=sebastian
https://hg.mozilla.org/integration/autoland/rev/31f4919d1f6c
Part 3 - Create and register ext-browserAction.js r=mixedpuppy
https://hg.mozilla.org/integration/autoland/rev/753e71053540
Part 4 - Add a position property to keep track of the menu item's position instead of using the ID r=sebastian
https://hg.mozilla.org/integration/autoland/rev/79579aab8851
Part 5 - Add unit tests for browserAction.onClicked r=mixedpuppy
Keywords: checkin-needed
Blocks: 1358888
backed out for causing bug 1358888 from m-c and the integration trees
Status: RESOLVED → REOPENED
status-firefox55: fixed → affected
Flags: needinfo?(mwein)
Resolution: FIXED → ---
It looks like this bug caused a startup crash on Android for addons that create a submenu and then populate that submenu, see Bug 1358888. I had a quick fixup patch in Bug 1358888 (no longer needed due to the backout) which should be helpful in making submenu items work with UUID menu ID's ( https://reviewboard.mozilla.org/r/132768/diff/1#index_header ). I'm not sure if it's an optimal approach, unfortunately Android menus operate purely on numerical IDs so we need some other way of mapping UUID to Android menu item ID (maybe the menu cache works for that, I haven't studied the code in detail).

(Https Everywhere creates a submenu, and hence is useful for testing purposes.)

It might be worth adding some tests to make sure that adding a submenu works as expected to avoid such breakage in future! (I.e. test that the ID returned from menu.add() can be used as a valid "parent" for any further items that are added. And we should maybe test the tools menu due to its use of a hardcoded ID if we don't have any tests for it yet.)
Backout by cbook@mozilla.com:
https://hg.mozilla.org/mozilla-central/rev/b38afd8b5109
Backed out changeset 79579aab8851 
https://hg.mozilla.org/mozilla-central/rev/ecd8c74214ff
Backed out changeset 753e71053540 
https://hg.mozilla.org/mozilla-central/rev/d54bcfaac2e2
Backed out changeset 31f4919d1f6c 
https://hg.mozilla.org/mozilla-central/rev/8a84c97e9e2f
Backed out changeset c91b14d9ad2a 
https://hg.mozilla.org/mozilla-central/rev/e17cbb839dd2
Backed out changeset 32ab1a3d736f for causing Bug 1358888 and help fixing a topcrash
I think this also caused bug 1349612 to reappear, so please test the "Report site issue" menu item before relanding.
I've updated the patch to only use UUIDs for the BrowserAction menu items without changing how the existing code works for legacy addons. When legacy addons are no longer supported this code can be simplified.

I've confirmed manually that "Https Everywhere" works now with the updated patch and that the "Report site issue" menu item is still enabled.
Flags: needinfo?(mwein)
Sebastian, do you think you could take another look at part 4? I needed to change it quite a bit in order to prevent legacy addons from breaking. I ended up creating a completely separate code path for adding and removing browser actions. Let me know if this looks okay.

https://reviewboard.mozilla.org/r/121160/diff/10#index_header
Flags: needinfo?(s.kaspari)
Comment on attachment 8848220 [details]
Bug 1331742 - Part 4 - Add support for adding and removing browser actions from the main menu

https://reviewboard.mozilla.org/r/121160/#review141852

::: mobile/android/base/java/org/mozilla/gecko/BrowserApp.java:1890
(Diff revision 10)
>                  break;
>  
> +            case "Menu:AddBrowserAction":
> +                final BrowserActionItemInfo browserAction = new BrowserActionItemInfo();
> +                browserAction.label = message.getString("name");
> +                if (browserAction.label == null || browserAction.label.equals("")) {

TextUtils.isEmpty() checks for null and empty strings.

::: mobile/android/base/java/org/mozilla/gecko/BrowserApp.java:3289
(Diff revision 10)
> +        item.setVisible(info.visible);
> +    }
> +
> +    private void addBrowserActionMenuItem(final BrowserActionItemInfo info) {
> +        if (mBrowserActionItemsCache == null) {
> +            mBrowserActionItemsCache = new Vector<BrowserActionItemInfo>();

Vector is quite an unusual choice in our code base. Usually we pick something based on the List interface (ArrayList or LinkedList).

::: mobile/android/base/java/org/mozilla/gecko/BrowserApp.java:3319
(Diff revision 10)
> +        if (mMenu == null || position == -1)
> +            return;

Please always use {} even for one line statements.

::: mobile/android/base/java/org/mozilla/gecko/BrowserApp.java:3324
(Diff revision 10)
> +        if (mMenu == null || position == -1)
> +            return;
> +
> +        final MenuItem menuItem = mMenu.findItem(position);
> +        if (menuItem != null)
> +            mMenu.removeItem(position);

Here too.
Attachment #8848220 - Flags: review?(walkingice0204)
I did a quick scan and added walkingice as a second reviewer. He's been working on the toolbar/menu lately.
Flags: needinfo?(s.kaspari)
Comment on attachment 8848220 [details]
Bug 1331742 - Part 4 - Add support for adding and removing browser actions from the main menu

https://reviewboard.mozilla.org/r/121160/#review141852

> TextUtils.isEmpty() checks for null and empty strings.

Thanks - updated.

> Vector is quite an unusual choice in our code base. Usually we pick something based on the List interface (ArrayList or LinkedList).

Okay, I switched to ArrayList.

> Please always use {} even for one line statements.

Done.

> Here too.

Done.
Attachment #8848220 - Flags: review?(walkingice0204)
Comment on attachment 8848220 [details]
Bug 1331742 - Part 4 - Add support for adding and removing browser actions from the main menu

https://reviewboard.mozilla.org/r/121160/#review142932

::: mobile/android/base/java/org/mozilla/gecko/BrowserApp.java:3293
(Diff revision 11)
> +
> +        addAddonMenuItemToMenu(mMenu, info);
> +    }
> +
> +    private void removeBrowserActionMenuItem(String uuid) {
> +        int position = -1;

since Menu.findItem and Menu.removeItem regards its parameter as ID, I think `mMenu.removeItem(id)` has different meaning from `mMenu.removeItem(position)`. Would it be ok if we change the naming to align Android doc?

::: mobile/android/base/java/org/mozilla/gecko/BrowserApp.java:3295
(Diff revision 11)
> +    }
> +
> +    private void removeBrowserActionMenuItem(String uuid) {
> +        int position = -1;
> +
> +        // Remove add-on menu item from cache, if available.

I am not quite sure about the naming. Should it still be 'add-on'?
Attachment #8848220 - Flags: review?(walkingice0204) → review+
Comment on attachment 8848220 [details]
Bug 1331742 - Part 4 - Add support for adding and removing browser actions from the main menu

https://reviewboard.mozilla.org/r/121160/#review142932

> since Menu.findItem and Menu.removeItem regards its parameter as ID, I think `mMenu.removeItem(id)` has different meaning from `mMenu.removeItem(position)`. Would it be ok if we change the naming to align Android doc?

Renamed position to id.

> I am not quite sure about the naming. Should it still be 'add-on'?

This comment should say browser action, thanks.
Pushed by mwein@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/6175b270de8d
Part 1 - Create and register browser_action.json r=mixedpuppy
https://hg.mozilla.org/integration/autoland/rev/8b5ac28fb391
Part 2 - Create a module for managing browser actions similar to PageActions.jsm r=sebastian
https://hg.mozilla.org/integration/autoland/rev/507eb09bc63f
Part 3 - Create and register ext-browserAction.js r=mixedpuppy
https://hg.mozilla.org/integration/autoland/rev/5b3f1f520ee3
Part 4 - Add support for adding and removing browser actions from the main menu r=sebastian,walkingice
https://hg.mozilla.org/integration/autoland/rev/ba79216d1f10
Part 5 - Add unit tests for browserAction.onClicked r=mixedpuppy
This looks good, thanks. 

Support for browserAction.setTitle and getTitle was added in 1351111. I just added dev-doc-needed to the bug.
Flags: needinfo?(mwein)
Oh, yes, thanks Matt. I did update those too, but didn't refresh the pages. So I;ve marked that one dev-doc-complete, but as always, let me know if we need anything else.
Keywords: dev-doc-needed → dev-doc-complete
Thanks!
Is manual testing necessary here? If yes, could you provide the proper WebExtension?
Flags: needinfo?(mwein)
Hi Cosmin,
As briefly discussed over IRC, manual testing this feature would be very helpful.

I'm attaching a small example addon which adds a browserAction and logs a message in the console when its browserAction has been clicked.

STR:
- install the attached extension xpi
- connect the Firefox for Android instance using the WebIDE (to be able to check that the expected console message has been logged when the browserAction has been clicked)
- click the browserAction added by the extension
- Expected behavior: the message "bug-1331742-browserAction-onClicked - browserAction clicked" should have been logged in the console panel of the WebIDE window connected to the Firefox for Android instance.
Flags: needinfo?(matthewjwein) → needinfo?(cosmin.badescu)
Thanks Luca!

This issue is verified as fixed on Fennec 56.0a1 (2017-07-27), the message "bug-1331742-browserAction-onClicked - browserAction clicked" is displayed in the WebIDE console panel, as described by Luca.

This issue could not be verified on the previous builds, because I am blocked by the Bug 1367494.
Flags: needinfo?(cosmin.badescu)
Status: RESOLVED → VERIFIED
status-firefox56: --- → verified
Blocks: 1389911
Product: Toolkit → WebExtensions
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: