Closed Bug 1300811 Opened 3 years ago Closed 3 years ago

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

Categories

(WebExtensions :: Android, defect, P2)

Unspecified
Android
defect

Tracking

(firefox56 verified, firefox57 verified)

VERIFIED FIXED
mozilla56
Tracking Status
firefox56 --- verified
firefox57 --- verified
Blocking Flags:
webextensions +

People

(Reporter: mattw, Assigned: mattw)

References

(Blocks 1 open bug)

Details

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

Attachments

(7 files)

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.
Would you be able to take a look Matt now that bug 1260548 has landed?
Assignee: nobody → mwein
Priority: -- → P2
webextensions: --- → ?
webextensions: ? → +
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 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.
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 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 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 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 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 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 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 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.
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.
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.
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
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 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+
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
Depends on: 1373170
Is there necessary any manual testing around this bug? If yes, could you please provide some reliable steps?
Flags: needinfo?(mwein)
Attached image ShowHide.gif
This issue is verified as fixed on Fennec 56.0b3 and Fennec 57.0a1(2017-08-17) under Android 6.0.1

The show and hide is supported per tab basis, as presented in the video.
Flags: needinfo?(matthewjwein)
Status: RESOLVED → VERIFIED
Product: Toolkit → WebExtensions
You need to log in before you can comment on or make changes to this bug.