Closed Bug 1351111 Opened 7 years ago Closed 7 years ago

Add support for browserAction.setTitle and browserAction.getTitle

Categories

(WebExtensions :: Android, enhancement, P2)

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

Tracking

(firefox55 verified, firefox56 verified)

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

People

(Reporter: mattw, Assigned: mattw)

References

Details

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

Attachments

(4 files)

Bug 1351111 - Add support for updating existing browser actions
59 bytes, text/x-review-board-request
sebastian
: review+
Details
Bug 1351111 - Add support for setTitle and getTitle
59 bytes, text/x-review-board-request
mixedpuppy
: review+
Details
Bug 1351111 - Add unit tests for setTitle and getTitle
59 bytes, text/x-review-board-request
mixedpuppy
: review+
Details
VideoSetGetTitle.gif
2.24 MB, image/gif
Details 
MDN documentation: 
https://developer.mozilla.org/en-US/Add-ons/WebExtensions/API/browserAction/getTitle
https://developer.mozilla.org/en-US/Add-ons/WebExtensions/API/browserAction/setTitle

Note - since BrowserActions in Fennec are displayed as menu items in the main menu, the title attribute is used as the menu item's label.
webextensions: --- → ?
webextensions: ? → +
Comment on attachment 8871496 [details]
Bug 1351111 - Add support for updating existing browser actions

https://reviewboard.mozilla.org/r/142970/#review146836
Attachment #8871496 - Flags: review?(s.kaspari) → review+
Comment on attachment 8871497 [details]
Bug 1351111 - Add support for setTitle and getTitle

https://reviewboard.mozilla.org/r/142972/#review147200

::: mobile/android/components/extensions/ext-browserAction.js:93
(Diff revision 1)
> +    if (tab == null && value) {
> +      this[prop] = value;
> +    } else if (value) {
> +      this.tabIdToPropertyMap.get(tab.id)[prop] = value;
> +    } else {
> +      delete this.tabIdToPropertyMap.get(tab.id)[prop];

seems like tab can be null here if value is also falsey.  Maybe just do:

if (tab ==null) {
  if (value) this[prop]=value
} else ...

::: mobile/android/modules/BrowserActions.jsm:76
(Diff revision 1)
>        uuid: browserAction.uuid,
>        name: browserAction.name,
>      });
>  
> -    this._browserActions[browserAction.uuid] = browserAction;
> +    this._browserActions[browserAction.uuid] = {
> +      name: browserAction.name,

done this change break browserAction.onClicked in onEvent above?
Comment on attachment 8871498 [details]
Bug 1351111 - Add unit tests for setTitle and getTitle

https://reviewboard.mozilla.org/r/142974/#review147206

::: mobile/android/modules/BrowserActions.jsm:99
(Diff revision 1)
>        });
>      }
>    },
>  
>    /**
> +   * Retrieves the name for the browser action with the specified UUID.

why more changes in a test patch?

::: mobile/android/modules/BrowserActions.jsm:126
(Diff revision 1)
> -  synthesizeClick: function(uuid) {
> +  synthesizeClick(uuid) {
>      let browserAction = this._browserActions[uuid];
>      if (!browserAction) {
>        throw new Error(`No BrowserAction with UUID ${uuid} was found`);
>      }
>      browserAction.onClicked();

seems like this would also be broken by changes in the earlier patch
Comment on attachment 8871497 [details]
Bug 1351111 - Add support for setTitle and getTitle

https://reviewboard.mozilla.org/r/142972/#review147208
Attachment #8871497 - Flags: review?(mixedpuppy)
Comment on attachment 8871498 [details]
Bug 1351111 - Add unit tests for setTitle and getTitle

https://reviewboard.mozilla.org/r/142974/#review147210
Attachment #8871498 - Flags: review?(mixedpuppy)
Comment on attachment 8871497 [details]
Bug 1351111 - Add support for setTitle and getTitle

https://reviewboard.mozilla.org/r/142972/#review147200

> seems like tab can be null here if value is also falsey.  Maybe just do:
> 
> if (tab ==null) {
>   if (value) this[prop]=value
> } else ...

Good catch, thanks. I'll also add a test for this case.

> done this change break browserAction.onClicked in onEvent above?

You're right, it does - I think it was better the way it was before. What I really need is a way to keep track of the name that's currently active for the selected tab, so I think adding a new member variable to track this would work well.
Comment on attachment 8871497 [details]
Bug 1351111 - Add support for setTitle and getTitle

https://reviewboard.mozilla.org/r/142972/#review147262
Comment on attachment 8871498 [details]
Bug 1351111 - Add unit tests for setTitle and getTitle

https://reviewboard.mozilla.org/r/142974/#review147206

> why more changes in a test patch?

I had these changes here because they are only used for testing purposes, but I moved them to the other patch to simplify the diffs.

> seems like this would also be broken by changes in the earlier patch

Good catch - this will be fixed in the updated patch.
Comment on attachment 8871498 [details]
Bug 1351111 - Add unit tests for setTitle and getTitle

https://reviewboard.mozilla.org/r/142974/#review147932
Attachment #8871498 - Flags: review?(mixedpuppy) → review+
Comment on attachment 8871497 [details]
Bug 1351111 - Add support for setTitle and getTitle

https://reviewboard.mozilla.org/r/142972/#review147934
Attachment #8871497 - Flags: review?(mixedpuppy) → review+
Pushed by mwein@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/c336400a21d7
Add support for updating existing browser actions r=sebastian
https://hg.mozilla.org/integration/autoland/rev/8e9c427717c8
Add support for setTitle and getTitle r=mixedpuppy
https://hg.mozilla.org/integration/autoland/rev/f066295fa2d3
Add unit tests for setTitle and getTitle r=mixedpuppy
Keywords: dev-doc-needed
Attached image VideoSetGetTitle.gif — 
After installation of the WebExtension https://addons-dev.allizom.org/en-US/android/addon/bookmark-it/ , the title is displayed in the drop-down list from the main menu.
 
Verified on Fennec 56.0a1 (2017-06-20) under Android 7.1.2
 
Can you confirm that this is the expected result for this bug Matthew ?
Flags: needinfo?(mwein)
Yes, that's expected, thanks.
Flags: needinfo?(mwein)
Based on comment 24 and comment 25 I will mark this bug as verified fixed.
 
Verified on Fennec 56.0a1 (2017-06-20) and Fennec 55.0a1 (2017-06-02) under Android 7.1.2
 
Thanks Matthew!
Status: RESOLVED → VERIFIED
status-firefox55: fixed → verified
status-firefox56: --- → verified
I was testing this feature, unfortunately, I got an unexpected result.

manifest.json
> ...
> "browser_action": {
>   "default_title": "Truuske"
> }
> ...

background.js
> chrome.browserAction.getTitle({}, (result) => {
>   console.log(result); // returns 'Truuske'
> });
> chrome.browserAction.setTitle({title: "Foo"});
> chrome.browserAction.setTitle({tabId: 1, title: "Bar"});
> chrome.browserAction.getTitle({}, (result) => {
>   console.log(result); // returns 'Foo'
> });
> chrome.browserAction.getTitle({tabId: 1}, (result) => {
>   console.log(result); // returns 'Bar'
> });

Results:
- At startup 'Truuske' is shown in the menu, while 'Foo' is immediately set. After a switch to another tab (or maybe a couple), the correct text is shown.
- 'Bar' is never shown in the menu
Hey Luca, can you please check this ^ out?
Flags: needinfo?(lgreco)
Thanks Tom,
I took a deeper look and, besides Bug 1375857, there are still two issues that needs to be fixed into the Android browserAction:

- Bug 1386786 - Fix Android browserAction.setTitle/getTitle behavior on switching and closing tabs
- Bug 1387026 - Fix Android browserAction corrupting legacy Addon menu items label on overlapping menu item id
Flags: needinfo?(lgreco)
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: