1351111 - Add support for browserAction.setTitle and browserAction.getTitle

Status: VERIFIED FIXED

(WebExtensions :: Android, enhancement, P2)

Description

[Matthew Wein [:mattw]]

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.

Comment 1

[Matthew Wein [:mattw]]

Attachment: Bug 1351111 - Add support for updating existing browser actions

Review commit: https://reviewboard.mozilla.org/r/142970/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/142970/

Comment 2

[Matthew Wein [:mattw]]

Attachment: Bug 1351111 - Add support for setTitle and getTitle

Review commit: https://reviewboard.mozilla.org/r/142972/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/142972/

Comment 3

[Matthew Wein [:mattw]]

Attachment: Bug 1351111 - Add unit tests for setTitle and getTitle

Review commit: https://reviewboard.mozilla.org/r/142974/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/142974/

Comment 4

[Sebastian Kaspari (:sebastian; :pocmo)]

Comment on attachment 8871496 [details]
Bug 1351111 - Add support for updating existing browser actions

https://reviewboard.mozilla.org/r/142970/#review146836

Comment 5

[Shane Caraveo (:mixedpuppy)]

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 6

[Shane Caraveo (:mixedpuppy)]

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 7

[Shane Caraveo (:mixedpuppy)]

Comment on attachment 8871497 [details]
Bug 1351111 - Add support for setTitle and getTitle

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

Comment 8

[Shane Caraveo (:mixedpuppy)]

Comment on attachment 8871498 [details]
Bug 1351111 - Add unit tests for setTitle and getTitle

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

Comment 9

[Matthew Wein [:mattw]]

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 10

[Matthew Wein [:mattw]]

Comment on attachment 8871497 [details]
Bug 1351111 - Add support for setTitle and getTitle

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

Comment 11

[Matthew Wein [:mattw]]

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 12

[Matthew Wein [:mattw]]

Comment on attachment 8871496 [details]
Bug 1351111 - Add support for updating existing browser actions

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/142970/diff/1-2/

Comment 13

[Matthew Wein [:mattw]]

Comment on attachment 8871497 [details]
Bug 1351111 - Add support for setTitle and getTitle

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/142972/diff/1-2/

Comment 14

[Matthew Wein [:mattw]]

Comment on attachment 8871498 [details]
Bug 1351111 - Add unit tests for setTitle and getTitle

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/142974/diff/1-2/

Comment 15

[Shane Caraveo (:mixedpuppy)]

Comment on attachment 8871498 [details]
Bug 1351111 - Add unit tests for setTitle and getTitle

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

Comment 16

[Shane Caraveo (:mixedpuppy)]

Comment on attachment 8871497 [details]
Bug 1351111 - Add support for setTitle and getTitle

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

Comment 17

[Matthew Wein [:mattw]]

Comment on attachment 8871496 [details]
Bug 1351111 - Add support for updating existing browser actions

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/142970/diff/2-3/

Comment 18

[Matthew Wein [:mattw]]

Comment on attachment 8871497 [details]
Bug 1351111 - Add support for setTitle and getTitle

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/142972/diff/2-3/

Comment 19

[Matthew Wein [:mattw]]

Comment on attachment 8871498 [details]
Bug 1351111 - Add unit tests for setTitle and getTitle

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/142974/diff/2-3/

Comment 20

[Matthew Wein [:mattw]]

Comment on attachment 8871498 [details]
Bug 1351111 - Add unit tests for setTitle and getTitle

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/142974/diff/3-4/

Comment 21

[Pulsebot]

Pushed by [e-mail]:
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

Comment 22

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/c336400a21d7
https://hg.mozilla.org/mozilla-central/rev/8e9c427717c8
https://hg.mozilla.org/mozilla-central/rev/f066295fa2d3

Comment 23

[Will Bamberg [:wbamberg]]

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

Comment 24

[CosminB]

Attachment: 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 ?

Comment 25

[Matthew Wein [:mattw]]

Yes, that's expected, thanks.

Comment 26

[CosminB]

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!

Comment 27

[lemnis.martens]

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

Comment 28

[Tomislav Jovanovic :zombie]

Hey Luca, can you please check this ^ out?

Comment 29

[Luca Greco [:rpl] [:luca] [: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