Closed
Bug 1331742
Opened 8 years ago
Closed 8 years ago
Add support for browserAction.onClicked
Categories
(WebExtensions :: Android, defect, P2)
Tracking
(firefox55 fixed, firefox56 verified)
VERIFIED
FIXED
mozilla55
webextensions | + |
People
(Reporter: mattw, Assigned: mattw)
References
Details
(Keywords: dev-doc-complete, Whiteboard: [browserAction]triaged)
Attachments
(7 files)
59 bytes,
text/x-review-board-request
|
mixedpuppy
:
review+
|
Details |
59 bytes,
text/x-review-board-request
|
sebastian
:
review+
|
Details |
59 bytes,
text/x-review-board-request
|
mixedpuppy
:
review+
|
Details |
59 bytes,
text/x-review-board-request
|
sebastian
:
review+
|
Details |
59 bytes,
text/x-review-board-request
|
mixedpuppy
:
review+
|
Details |
4.94 KB,
application/x-xpinstall
|
Details | |
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 | ||
Updated•8 years ago
|
Assignee: nobody → mwein
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Assignee | ||
Comment 5•8 years 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•8 years ago
|
Attachment #8848217 -
Flags: review?(mixedpuppy)
Assignee | ||
Updated•8 years ago
|
Attachment #8848218 -
Flags: review?(gbrown)
Assignee | ||
Updated•8 years ago
|
Attachment #8848219 -
Flags: review?(mixedpuppy)
Assignee | ||
Updated•8 years ago
|
Attachment #8848220 -
Flags: review?(lgreco)
Comment 6•8 years 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•8 years 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
Updated•8 years ago
|
Attachment #8848217 -
Flags: review?(mixedpuppy)
Assignee | ||
Comment 8•8 years ago
|
||
Assignee | ||
Comment 9•8 years ago
|
||
Assignee | ||
Comment 10•8 years ago
|
||
Assignee | ||
Comment 11•8 years ago
|
||
Assignee | ||
Comment 12•8 years ago
|
||
Assignee | ||
Comment 13•8 years ago
|
||
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment 19•8 years 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•8 years 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•8 years 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•8 years ago
|
webextensions: --- → ?
Updated•8 years ago
|
webextensions: ? → +
Comment 22•8 years ago
|
||
(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)
Updated•8 years ago
|
Attachment #8848218 -
Flags: review?(s.kaspari)
Attachment #8848220 -
Flags: review?(s.kaspari)
Comment 23•8 years 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•8 years 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•8 years 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•8 years 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•8 years 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 28•8 years 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/#review129422
Attachment #8848218 -
Flags: review?(s.kaspari) → review+
Comment 29•8 years 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/#review129424
Attachment #8848220 -
Flags: review?(s.kaspari) → review+
Assignee | ||
Comment 30•8 years 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•8 years ago
|
||
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Assignee | ||
Comment 42•8 years ago
|
||
Comment 43•8 years 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•8 years 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•8 years 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•8 years 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•8 years ago
|
||
Assignee | ||
Comment 56•8 years 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•8 years ago
|
||
Comment 63•8 years 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/#review131286
Attachment #8848218 -
Flags: review+
Comment 64•8 years 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/#review131288
Attachment #8848220 -
Flags: review+
Comment 65•8 years ago
|
||
(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•8 years ago
|
||
Assignee | ||
Comment 67•8 years ago
|
||
Assignee | ||
Comment 68•8 years ago
|
||
Assignee | ||
Comment 69•8 years ago
|
||
Assignee | ||
Comment 70•8 years ago
|
||
Assignee | ||
Comment 71•8 years ago
|
||
Assignee | ||
Comment 72•8 years ago
|
||
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Assignee | ||
Updated•8 years ago
|
Keywords: checkin-needed
Comment 78•8 years 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•8 years 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
Closed: 8 years ago
status-firefox55:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
Comment 80•8 years ago
|
||
backed out for causing bug 1358888 from m-c and the integration trees
Status: RESOLVED → REOPENED
Flags: needinfo?(mwein)
Resolution: FIXED → ---
Comment 81•8 years 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•8 years 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
Comment 83•8 years ago
|
||
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•8 years 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•8 years 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 96•8 years 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/#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.
Updated•8 years ago
|
Attachment #8848220 -
Flags: review?(walkingice0204)
Comment 97•8 years ago
|
||
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•8 years 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•8 years ago
|
||
Updated•8 years ago
|
Keywords: dev-doc-needed
Assignee | ||
Updated•8 years ago
|
Attachment #8848220 -
Flags: review?(walkingice0204)
Comment 105•8 years 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•8 years 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•8 years ago
|
||
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment 118•8 years 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•8 years 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
Closed: 8 years ago → 8 years ago
Resolution: --- → FIXED
Comment 120•7 years ago
|
||
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•7 years 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•7 years ago
|
||
Comment 123•7 years ago
|
||
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•7 years ago
|
||
Thanks!
Comment 125•7 years ago
|
||
Is manual testing necessary here? If yes, could you provide the proper WebExtension?
Flags: needinfo?(mwein)
Comment 126•7 years ago
|
||
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)
Comment 127•7 years ago
|
||
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
Updated•6 years ago
|
Product: Toolkit → WebExtensions
You need to log in
before you can comment on or make changes to this bug.
Description
•