[PageAction] Support show/hide on a per tab basis on Android.

RESOLVED FIXED in Firefox 56

Status

()

Toolkit
WebExtensions: Android
P2
normal
RESOLVED FIXED
11 months ago
28 days ago

People

(Reporter: mattw, Assigned: mattw, NeedInfo)

Tracking

(Depends on: 1 bug, Blocks: 1 bug)

unspecified
mozilla56
Unspecified
Android
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox56 fixed)

Details

(Whiteboard: [pageAction]triaged)

MozReview Requests

()

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

Attachments

(6 attachments)

(Assignee)

Description

11 months ago
Currently, show and hide work globally across all tabs, ignoring the tab ID argument passed in.  Once basic support for the Tabs API is added, we can start supporting show and hide on a per tab basis.

Comment 1

7 months ago
Would you be able to take a look Matt now that bug 1260548 has landed?
Assignee: nobody → mwein

Updated

6 months ago
Priority: -- → P2
(Assignee)

Updated

5 months ago
webextensions: --- → ?

Updated

5 months ago
webextensions: ? → +
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)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 13

2 months ago
mozreview-review
Comment on attachment 8875597 [details]
Bug 1300811 - Part 2 - Add support for TabContext to android

https://reviewboard.mozilla.org/r/147018/#review152478
Attachment #8875597 - Flags: review?(mixedpuppy) → review+

Comment 14

2 months ago
mozreview-review
Comment on attachment 8875598 [details]
Bug 1300811 - Part 4 - Support show and hide on a per tab basis

https://reviewboard.mozilla.org/r/147020/#review152480

::: mobile/android/components/extensions/ext-pageAction.js:34
(Diff revisions 2 - 3)
>      this.icons = IconDetails.normalize({path: options.default_icon}, extension);
>  
>      this.popup = options.default_popup;
>  
> -    // Map[TabID -> Object]
> -    this.tabIdToPropertyMap = new DefaultMap(() => ({}));
> +    // Object[TabID -> Object]
> +    this.tabIdToPropertyMap = {};

Couldn't a weakmap be used here?  I'm guessing we don't have TabContext on android.  I wonder if it should be.
(Assignee)

Comment 15

2 months ago
mozreview-review-reply
Comment on attachment 8875598 [details]
Bug 1300811 - Part 4 - Support show and hide on a per tab basis

https://reviewboard.mozilla.org/r/147020/#review152480

> Couldn't a weakmap be used here?  I'm guessing we don't have TabContext on android.  I wonder if it should be.

Yeah, I think now would be a good time to add a TabContext to android.
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 22

2 months ago
mozreview-review
Comment on attachment 8875597 [details]
Bug 1300811 - Part 2 - Add support for TabContext to android

https://reviewboard.mozilla.org/r/147018/#review153026

::: mobile/android/components/extensions/ext-utils.js:438
(Diff revision 3)
> +// Manages tab-specific context data and dispatches tab select and close events.
> +class TabContext {
> +  constructor(getDefaults, extension) {
> +    this.extension = extension;
> +    this.getDefaults = getDefaults;
> +    this.tabData = new Map();

Can weakmap be used here?

Comment 23

2 months ago
mozreview-review
Comment on attachment 8876976 [details]
Bug 1300811 - Part 1 - Convert PageAction to a class

https://reviewboard.mozilla.org/r/148288/#review153028
Attachment #8876976 - Flags: review?(mixedpuppy) → review+

Comment 24

2 months ago
mozreview-review
Comment on attachment 8876977 [details]
Bug 1300811 - Part 3 - Update the browser action API to use TabContext

https://reviewboard.mozilla.org/r/148290/#review153034

::: mobile/android/components/extensions/ext-browserAction.js:121
(Diff revision 1)
>  
>    /**
>     * Unregister the browser action from the BrowserActions module.
>     */
>    shutdown() {
>      BrowserActions.unregister(this.uuid);

Do you need to call tabContext.shutdown here?

::: mobile/android/components/extensions/test/mochitest/test_ext_browserAction_getTitle_setTitle.html:31
(Diff revision 1)
>      });
>  
>      async function createAndTestNewTab(title, url) {
>        // First make sure the default title is correct.
>        let defaultTitle = await browser.browserAction.getTitle({});
> -      browser.test.assertEq("Browser Action", defaultTitle, `Expected the default title to be ${defaultTitle}`);
> +      browser.test.assertEq("Browser Action", defaultTitle, `Expected the default title to be "Browser Action"`);

nit: A failure should output expected and got values, no need to say it again.  Just "Expected default title" is enough.

::: mobile/android/components/extensions/test/mochitest/test_ext_browserAction_getTitle_setTitle.html:115
(Diff revision 1)
>  
>    async function checkTab(tab, name) {
>      extension.sendMessage("select-tab", tab.id);
>      await extension.awaitMessage("tab-selected");
> +    // Give the browser action a chance to update the title.
> +    await new Promise(resolve => setTimeout(resolve, 10));

intermittent waiting to happen. Is there any way to use something like BrowserTestUtils.waitForCondition?
Attachment #8876977 - Flags: review?(mixedpuppy)

Comment 25

2 months ago
mozreview-review
Comment on attachment 8875598 [details]
Bug 1300811 - Part 4 - Support show and hide on a per tab basis

https://reviewboard.mozilla.org/r/147020/#review153046

::: mobile/android/components/extensions/ext-pageAction.js:161
(Diff revision 4)
>        return Promise.resolve();
>      }
>  
>      this.shouldShow = true;
>  
>      // TODO(robwu): Remove dependency on contentWindow from this file. It should

Unrelated, but can you add bugs for these TODO items, and put the bug #'s here?
Attachment #8875598 - Flags: review?(mixedpuppy) → review+

Comment 26

2 months ago
mozreview-review
Comment on attachment 8875599 [details]
Bug 1300811 - Part 5 - test show and hide with multiple tabs

https://reviewboard.mozilla.org/r/147022/#review153048

Try to use ContentTaskUtils here.

::: mobile/android/components/extensions/test/mochitest/test_ext_pageAction_show_hide.html:109
(Diff revision 4)
> +      await extension.awaitMessage("page-action-shown");
> +      ok(!PageActions.isShown(uuid), "The PageAction should still not be shown");
> +      extension.sendMessage("select-tab", tabId);
> +      await extension.awaitMessage("tab-selected");
> +      // Give the page action a chance to be shown.
> +      await new Promise(resolve => setTimeout(resolve, 10));

waitForCondition
Attachment #8875599 - Flags: review?(mixedpuppy) → review+

Comment 27

2 months ago
mozreview-review
Comment on attachment 8875600 [details]
Bug 1300811 - Part 6 - test setPopup and getPopup with multiple tabs

https://reviewboard.mozilla.org/r/147024/#review153052

::: mobile/android/components/extensions/test/mochitest/test_ext_pageAction_getPopup_setPopup.html:181
(Diff revision 4)
> +    PageActions.synthesizeClick(uuid);
> +    let location = await extension.awaitMessage("page-action-from-popup");
> +    ok(location.includes(expectedPopup), "The popup with the correct URL should be shown.");
> +
> +    extension.sendMessage("page-action-close-popup", {location});
> +    location = await tabClosedPromise();

unused location?  remove if not needed.
Attachment #8875600 - Flags: review?(mixedpuppy) → 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)
(Assignee)

Comment 34

2 months ago
mozreview-review-reply
Comment on attachment 8875597 [details]
Bug 1300811 - Part 2 - Add support for TabContext to android

https://reviewboard.mozilla.org/r/147018/#review153026

> Can weakmap be used here?

WeakMaps require the keys to be objects, and I'd like to use the tab Id as the key, since that's all that is provided to "Tab:Selected" and "Tab:Closed" and it's all that is needed for the implementation.
(Assignee)

Comment 35

2 months ago
mozreview-review-reply
Comment on attachment 8875599 [details]
Bug 1300811 - Part 5 - test show and hide with multiple tabs

https://reviewboard.mozilla.org/r/147022/#review153048

Thanks, using ContentTaskUtils worked fine

> waitForCondition

Done.
(Assignee)

Comment 36

2 months ago
mozreview-review-reply
Comment on attachment 8875600 [details]
Bug 1300811 - Part 6 - test setPopup and getPopup with multiple tabs

https://reviewboard.mozilla.org/r/147024/#review153052

> unused location?  remove if not needed.

It's used on the following line to confirm that the correct tab was closed.
(Assignee)

Comment 37

2 months ago
mozreview-review-reply
Comment on attachment 8876977 [details]
Bug 1300811 - Part 3 - Update the browser action API to use TabContext

https://reviewboard.mozilla.org/r/148290/#review153034

> Do you need to call tabContext.shutdown here?

Yes, thanks, same for ext-pageAction.js

> nit: A failure should output expected and got values, no need to say it again.  Just "Expected default title" is enough.

Done.

> intermittent waiting to happen. Is there any way to use something like BrowserTestUtils.waitForCondition?

Updated to use ContentTaskUtils.waitForCondition
(Assignee)

Comment 38

2 months ago
mozreview-review-reply
Comment on attachment 8875598 [details]
Bug 1300811 - Part 4 - Support show and hide on a per tab basis

https://reviewboard.mozilla.org/r/147020/#review153046

> Unrelated, but can you add bugs for these TODO items, and put the bug #'s here?

Sure - I filed bug 1372782 and bug 1372783.
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 42

2 months ago
mozreview-review
Comment on attachment 8876977 [details]
Bug 1300811 - Part 3 - Update the browser action API to use TabContext

https://reviewboard.mozilla.org/r/148290/#review153676
Attachment #8876977 - Flags: review?(mixedpuppy) → review+

Comment 43

2 months ago
Pushed by mwein@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/b1a39016988e
Part 1 - Convert PageAction to a class r=mixedpuppy
https://hg.mozilla.org/integration/autoland/rev/ab64e2df938f
Part 2 - Add support for TabContext to android r=mixedpuppy
https://hg.mozilla.org/integration/autoland/rev/b8834309f85b
Part 3 - Update the browser action API to use TabContext r=mixedpuppy
https://hg.mozilla.org/integration/autoland/rev/76b7cff0e3b2
Part 4 - Support show and hide on a per tab basis r=mixedpuppy
https://hg.mozilla.org/integration/autoland/rev/58da9860201b
Part 5 - test show and hide with multiple tabs r=mixedpuppy
https://hg.mozilla.org/integration/autoland/rev/625f79374b4d
Part 6 - test setPopup and getPopup with multiple tabs r=mixedpuppy
https://hg.mozilla.org/mozilla-central/rev/b1a39016988e
https://hg.mozilla.org/mozilla-central/rev/ab64e2df938f
https://hg.mozilla.org/mozilla-central/rev/b8834309f85b
https://hg.mozilla.org/mozilla-central/rev/76b7cff0e3b2
https://hg.mozilla.org/mozilla-central/rev/58da9860201b
https://hg.mozilla.org/mozilla-central/rev/625f79374b4d
Status: NEW → RESOLVED
Last Resolved: 2 months ago
status-firefox56: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla56

Updated

2 months ago
Depends on: 1373170

Comment 45

28 days ago
Is there necessary any manual testing around this bug? If yes, could you please provide some reliable steps?
Flags: needinfo?(mwein)
You need to log in before you can comment on or make changes to this bug.