Note: There are a few cases of duplicates in user autocompletion which are being worked on.

Add support for browserAction.setTitle and browserAction.getTitle

VERIFIED FIXED in Firefox 55

Status

()

Toolkit
WebExtensions: Android
P2
normal
VERIFIED FIXED
4 months ago
4 days ago

People

(Reporter: mattw, Assigned: mattw)

Tracking

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

unspecified
mozilla55
Unspecified
Android
dev-doc-complete
Points:
---

Firefox Tracking Flags

(firefox55 verified, firefox56 verified)

Details

(Whiteboard: [browserAction]triaged)

MozReview Requests

()

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

Attachments

(4 attachments)

(Assignee)

Description

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

Updated

4 months ago
webextensions: --- → ?

Updated

4 months ago
webextensions: ? → +
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 4

2 months ago
mozreview-review
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 5

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

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

2 months ago
mozreview-review
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 8

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

Comment 9

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

Comment 10

2 months ago
mozreview-review
Comment on attachment 8871497 [details]
Bug 1351111 - Add support for setTitle and getTitle

https://reviewboard.mozilla.org/r/142972/#review147262
(Assignee)

Comment 11

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

Comment 15

2 months ago
mozreview-review
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 16

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

Comment 21

2 months ago
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
https://hg.mozilla.org/mozilla-central/rev/c336400a21d7
https://hg.mozilla.org/mozilla-central/rev/8e9c427717c8
https://hg.mozilla.org/mozilla-central/rev/f066295fa2d3
Status: NEW → RESOLVED
Last Resolved: 2 months ago
status-firefox55: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
(Assignee)

Updated

2 months ago
Keywords: dev-doc-needed
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
Keywords: dev-doc-needed → dev-doc-complete

Comment 24

a month ago
Created attachment 8879567 [details]
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)
(Assignee)

Comment 25

a month ago
Yes, that's expected, thanks.
Flags: needinfo?(mwein)

Comment 26

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

Comment 27

4 days ago
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
You need to log in before you can comment on or make changes to this bug.