Add support for browserAction.setTitle and browserAction.getTitle

NEW
Assigned to

Status

()

Toolkit
WebExtensions: Android
P2
normal
2 months ago
18 hours ago

People

(Reporter: mattw, Assigned: mattw)

Tracking

(Blocks: 1 bug)

unspecified
Unspecified
Android
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [browserAction]triaged)

MozReview Requests

()

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

Attachments

(3 attachments)

(Assignee)

Description

2 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

2 months ago
webextensions: --- → ?

Updated

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

Comment 4

2 days 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

a day 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

a day 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

a day 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

a day 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

19 hours 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

19 hours 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

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