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?

Comment on attachment 8875597 [details] Bug 1300811 - Part 2 - Add support for TabContext to android https://reviewboard.mozilla.org/r/147018/#review152478

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

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?

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?

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

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.

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

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

Is there necessary any manual testing around this bug? If yes, could you please provide some reliable steps?

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.

I've updated the compat data for: https://developer.mozilla.org/en-US/Add-ons/WebExtensions/API/pageAction/show#Browser_compatibility https://developer.mozilla.org/en-US/Add-ons/WebExtensions/API/pageAction/hide#Browser_compatibility Setting this dev-doc-complete, but let me know if we need anything else.