Add support for browserAction.onClicked

RESOLVED FIXED in Firefox 55

Status

()

Toolkit
WebExtensions: Android
P2
normal
RESOLVED FIXED
5 months ago
16 days ago

People

(Reporter: mattw, Assigned: mattw)

Tracking

(Blocks: 1 bug, {dev-doc-complete})

unspecified
mozilla55
Unspecified
Android
dev-doc-complete
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox55 fixed)

Details

(Whiteboard: [browserAction]triaged)

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(5 attachments)

(Assignee)

Description

5 months ago
MDN documentation: https://developer.mozilla.org/en-US/Add-ons/WebExtensions/API/BrowserAction/onClicked

This bug will involve adding icons to the UI.
(Assignee)

Updated

3 months ago
Assignee: nobody → mwein
(Assignee)

Updated

3 months ago
Blocks: 1330159
(Assignee)

Updated

3 months ago
No longer blocks: 1331126
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Comment 5

3 months ago
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.
(Assignee)

Updated

3 months ago
Attachment #8848217 - Flags: review?(mixedpuppy)
(Assignee)

Updated

3 months ago
Attachment #8848218 - Flags: review?(gbrown)
(Assignee)

Updated

3 months ago
Attachment #8848219 - Flags: review?(mixedpuppy)
(Assignee)

Updated

3 months ago
Attachment #8848220 - Flags: review?(lgreco)

Comment 6

3 months ago
mozreview-review
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)
(Assignee)

Comment 7

3 months ago
mozreview-review-reply
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)
(Assignee)

Comment 8

3 months ago
https://treeherder.mozilla.org/#/jobs?repo=try&revision=4ce15e8e6c75825093bca247ab1bd579aba05dc0
(Assignee)

Comment 9

3 months ago
https://treeherder.mozilla.org/#/jobs?repo=try&revision=197df833f743716e5f3ff3bfd6e94dcc8dc26f88
(Assignee)

Comment 10

3 months ago
https://treeherder.mozilla.org/#/jobs?repo=try&revision=e2279f174441c75c1e95493276e7919210190298
(Assignee)

Comment 11

3 months ago
https://treeherder.mozilla.org/#/jobs?repo=try&revision=12a03bac95a70f786ba000d1681a127d05398946
(Assignee)

Comment 12

3 months ago
https://treeherder.mozilla.org/#/jobs?repo=try&revision=e0063125250cb10e511559b8134f986bcb4a2ce6
(Assignee)

Comment 13

3 months ago
https://treeherder.mozilla.org/#/jobs?repo=try&revision=4f41572ff5d9f9937384bc9d8658e0d7f2afc8cf
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 19

3 months ago
mozreview-review
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 20

3 months ago
mozreview-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/#review126360
Attachment #8848220 - Flags: review?(gbrown)
(Assignee)

Comment 21

3 months ago
No worries, thanks for the quick response. Sebastian, would you be comfortable reviewing Part 2 and Part 4?
Flags: needinfo?(s.kaspari)
(Assignee)

Updated

3 months ago
webextensions: --- → ?

Updated

3 months ago
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 23

3 months ago
mozreview-review
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 24

3 months ago
mozreview-review
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 25

3 months ago
mozreview-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.
(Assignee)

Comment 26

3 months ago
mozreview-review-reply
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.
(Assignee)

Comment 27

3 months ago
mozreview-review-reply
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+
(Assignee)

Comment 30

3 months ago
mozreview-review-reply
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 hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Comment 36

3 months ago
https://treeherder.mozilla.org/#/jobs?repo=try&revision=4d2b2cb41ed83e6218481d0fd6733e3a112e2901
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Comment 42

3 months ago
https://treeherder.mozilla.org/#/jobs?repo=try&revision=de3fbcd2f9c3096e33c25b5418f0db6d24a6a0dd

Comment 43

3 months ago
mozreview-review
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 44

3 months ago
mozreview-review
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 45

3 months ago
mozreview-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+
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Comment 54

2 months ago
> 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?
(Assignee)

Comment 55

2 months ago
https://treeherder.mozilla.org/#/jobs?repo=try&revision=a6b47a51764bfc7809330b8387081859a445625d
(Assignee)

Comment 56

2 months ago
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 hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Comment 62

2 months ago
https://treeherder.mozilla.org/#/jobs?repo=try&revision=ba554c0caa0343c6dbab4c47fe9d359475752291
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)
(Assignee)

Comment 66

2 months ago
https://treeherder.mozilla.org/#/jobs?repo=try&revision=376a017140b26d12f157ff4ad0434c1f5ffc1097
(Assignee)

Comment 67

2 months ago
https://treeherder.mozilla.org/#/jobs?repo=try&revision=b0cba29d6cf92f96eca97df4f6b56b4ea4e152ae
(Assignee)

Comment 68

2 months ago
https://treeherder.mozilla.org/#/jobs?repo=try&revision=749f7b95da02708a5490efd68eda7d9f45751a7d
(Assignee)

Comment 69

2 months ago
https://treeherder.mozilla.org/#/jobs?repo=try&revision=4a763765e24d4383611d7bd767f2ade492519d4f
(Assignee)

Comment 70

2 months ago
https://treeherder.mozilla.org/#/jobs?repo=try&revision=d350af8f252d0dec7063f85dd374ddf39fc4286b
(Assignee)

Comment 71

2 months ago
https://treeherder.mozilla.org/#/jobs?repo=try&revision=2e7edc41aab2e00cc8f27939723147d2eb525341
(Assignee)

Comment 72

2 months ago
https://treeherder.mozilla.org/#/jobs?repo=try&revision=cc0a6010d1f673149c31198548ee455beb038dbe
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Updated

2 months ago
Keywords: checkin-needed

Comment 78

2 months ago
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

Comment 79

2 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/32ab1a3d736f
https://hg.mozilla.org/mozilla-central/rev/c91b14d9ad2a
https://hg.mozilla.org/mozilla-central/rev/31f4919d1f6c
https://hg.mozilla.org/mozilla-central/rev/753e71053540
https://hg.mozilla.org/mozilla-central/rev/79579aab8851
Status: NEW → RESOLVED
Last Resolved: 2 months ago
status-firefox55: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla55

Updated

2 months ago
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 → ---

Comment 81

2 months ago
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.)

Comment 82

2 months ago
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.
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Comment 89

a month ago
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)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Comment 95

a month ago
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)
(Assignee)

Comment 98

a month ago
mozreview-review-reply
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.
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Comment 104

a month ago
https://treeherder.mozilla.org/#/jobs?repo=try&revision=d1ee1fd4b3f927d4db66f91349eb9f0f72a60783
Keywords: dev-doc-needed
(Assignee)

Updated

a month ago
Attachment #8848220 - Flags: review?(walkingice0204)

Comment 105

a month ago
mozreview-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

::: 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+
(Assignee)

Comment 106

a month ago
mozreview-review-reply
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.
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Comment 112

a month ago
https://treeherder.mozilla.org/#/jobs?repo=try&revision=4efb6cb3132d99b4e3eaee60e3cdb332690706fa
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 118

a month ago
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

Comment 119

a month ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/6175b270de8d
https://hg.mozilla.org/mozilla-central/rev/8b5ac28fb391
https://hg.mozilla.org/mozilla-central/rev/507eb09bc63f
https://hg.mozilla.org/mozilla-central/rev/5b3f1f520ee3
https://hg.mozilla.org/mozilla-central/rev/ba79216d1f10
Status: REOPENED → RESOLVED
Last Resolved: 2 months agoa month ago
status-firefox55: affected → fixed
Resolution: --- → FIXED
Updated compat data for browser_action:
https://developer.mozilla.org/en-US/Add-ons/WebExtensions/manifest.json/browser_action#Browser_compatibility

and browserAction:
https://developer.mozilla.org/en-US/Add-ons/WebExtensions/API/browserAction/onClicked#Browser_compatibility

Please let me know if we need anything else.
Flags: needinfo?(mwein)
(Assignee)

Comment 121

16 days ago
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)
(Assignee)

Comment 122

16 days ago
https://bugzilla.mozilla.org/show_bug.cgi?id=1351111
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
(Assignee)

Comment 124

16 days ago
Thanks!
You need to log in before you can comment on or make changes to this bug.