Due to the large amount of input, this bug is only for implementation and review of the first phase of tab hidding. Comments are disabled here to keep this focused on the work effort but open on the original bug 1384515. See https://wiki.mozilla.org/WebExtensions/TabHiding for details of the implementation.

Comment on attachment 8935163 [details] Bug 1423725 add show/hide tabs api, only f? right now, thinking about a couple more things.

Comment on attachment 8935163 [details] Bug 1423725 add show/hide tabs api, https://reviewboard.mozilla.org/r/206020/#review211826 ::: browser/base/content/tabbrowser.xml:2497 (Diff revision 1) > aTab.dispatchEvent(evt); > ]]> > </body> > </method> > > - <method name="discardBrowser"> > + <method name="canDiscardBrowser"> Please don't expose this method. Like I said in bug 1384515, a tab not being discardable shouldn't prevent it from getting hidden, as that would be super confusing for users. ::: browser/base/content/tabbrowser.xml:2497 (Diff revision 1) > aTab.dispatchEvent(evt); > ]]> > </body> > </method> > > - <method name="discardBrowser"> > + <method name="canDiscardBrowser"> Please don't expose this method. Like I said in bug 1384515, a tab not being discardable shouldn't prevent it from getting hidden, as that would be super confusing for users.

There are reasons right now that we can't discard a tab (and hence hide it) and surfacing that back to the add-on developer who can then make a meaningful message back to the user seems reasonable. At this time we are in the process of iterating through this feature behind a pref (although I don't think that's been added to this patch) until we feel that all usability, security and privacy reviews have been resolved. This API let's us land something to continue iterating on this functionality until its ready to go. There's been more discussions on this, because bug 1834515 got a lot of noise on and I'd be happy to talk to you about it more if you've got time.

(In reply to Andy McKay [:andym] from comment #5) > There are reasons right now that we can't discard a tab (and hence hide it) As we need to prevent dataloss and can't perfectly restore browser states, this isn't just a current limitation but will stay like this for the foreseeable future. Furthermore, and this is the point I already made in my previous comments, there's no direct connection between being able to discard a browser and hiding the tab. In fact we used to hide tabs with Panorama / Tab Groups before we were even able to discard browsers.

I looked through this and I'm concerned about race conditions. Suppose an extension wants to hide all of the current tabs and show some new ones. First it gets the list of currently shown tabs, then it calls hide() and passes the list in. If between those two steps one of the tabs is closed, getTab() will throw and the remaining tabs won't be hidden. This leaves things in an inconsistent state where multiple groups could be mixed together. Am I reading this code wrong and/or is there some other reason this couldn't happen? An extension could work around this by catching the exception and retrying, assuming it keeps an internal list of what needs to be hidden and shown.

Comment on attachment 8935162 [details] Bug 1423725 add event, query and details for hidden status, https://reviewboard.mozilla.org/r/206018/#review212516 The WebExtension side of this looks fine. I know approximately nothing about the tabbrowser side of this, its probably worth having Dao or somebody look over this too.

(In reply to Andrew Swan [:aswan] from comment #8) > Comment on attachment 8935162 [details] > Bug 1423725 add event, query and details for hidden status, > > https://reviewboard.mozilla.org/r/206018/#review212516 > > The WebExtension side of this looks fine. I know approximately nothing > about the tabbrowser side of this, its probably worth having Dao or somebody > look over this too. I find that confusing, the patch reviewed has no tabbrowser changes. Are you talking about the second patch that I requested feedback on? (and I have more changes on the second patch anyway)

Comment on attachment 8935163 [details] Bug 1423725 add show/hide tabs api, https://reviewboard.mozilla.org/r/206020/#review212518 ::: browser/components/extensions/ext-tabs.js:107 (Diff revision 1) > await tabListener.awaitTabReady(tab.nativeTab); > > return tab; > } > > + if (extension.hasPermission("tabHide")) { `getAPI()` does not seem like the right place to do this. What is the behavior you're trying to get? ::: browser/components/extensions/ext-tabs.js:115 (Diff revision 1) > + // Show all hidden tabs if a tab managing extension is uninstalled or > + // disabled. If a user has more than one, the extensions will need to > + // self-manage re-hiding tabs. Really? I realize this is meant to be the first of multiple iterations but what's the long-term idea here? ::: browser/components/extensions/ext-tabs.js:1024 (Diff revision 1) > + if (!extension.hasPermission("tabHide")) { > + throw new ExtensionError("tabHide permission required."); > + } this should be done from the schema ::: browser/components/extensions/ext-tabs.js:1033 (Diff revision 1) > + tabIds = [tabIds]; > + } > + // TODO Bug 1384515 followup. We additionally discard hidden tabs > + // which places the tab back into a suspended lazy state. This will > + // become an option later. > + let discard = !options || options.discard !== false; The schema will normalize this, you don't need to check for `!options`. You can also put a default value for `discard` into the schema and then just rea the value here. ::: browser/components/extensions/ext-tabs.js:1042 (Diff revision 1) > + } > + > + let hidden = []; > + for (let tabId of tabIds) { > + let tab = tabTracker.getTab(tabId); > + if (!discard || tab.ownerGlobal.gBrowser.canDiscardBrowser(tab.linkedBrowser, true)) { I don't understand the logic here, if discard is false then discard the browser regardless of whether it can be discarded? ::: browser/components/extensions/schemas/tabs.json:16 (Diff revision 1) > "choices": [{ > "type": "string", > "enum": [ > "activeTab", > - "tabs" > + "tabs", > + "tabHide" An optional permission without a permission prompt is effectively a no-op... ::: browser/components/extensions/schemas/tabs.json:1223 (Diff revision 1) > ] > } > ] > + }, > + { > + "name": "show", this doesn't require a permission? ::: browser/components/extensions/test/browser/browser_ext_tabs_hide.js:39 (Diff revision 1) > + browser.tabs.hide(ids).then(() => { > + browser.test.notifyFail("pref-test"); > + }).catch(e => { > + browser.test.assertTrue(e.message.startsWith("tabs.hide is currently experimental"), "enabled message received"); > + browser.test.notifyPass("pref-test"); > + }); Please just use `try {...} catch () {...}` ::: browser/components/extensions/test/browser/browser_ext_tabs_hide.js:42 (Diff revision 1) > + let tabs = await browser.tabs.query({hidden: false}); > + let ids = tabs.map(tab => tab.id); > + browser.tabs.hide(ids).then(() => { > + browser.test.notifyFail("pref-test"); > + }).catch(e => { > + browser.test.assertTrue(e.message.startsWith("tabs.hide is currently experimental"), "enabled message received"); Fix the message ::: browser/components/extensions/test/browser/browser_ext_tabs_hide.js:72 (Diff revision 1) > + let tabs = await browser.tabs.query({hidden: false}); > + for (let tab of tabs) { > + browser.test.assertFalse(tab.hidden, "tab is visible"); > + } > + let ids = tabs.map(tab => tab.id); > + browser.tabs.hide(ids); This seems dangerous, if you hide and discard the tab from which the mochitest framework is running, bad things will happen. I assume you've run this and its passing so that's not happening for some reason, but I think the safer thing would be to explicitly create some test tabs and only mess with those.

Comment on attachment 8935163 [details] Bug 1423725 add show/hide tabs api, Even as a first iteration, there are some issues from the WebExtensions side. And this should also be reviewed by somebody more familiar with tabbrowser.

(In reply to Shane Caraveo (:mixedpuppy) from comment #9) > I find that confusing, the patch reviewed has no tabbrowser changes. Nothing is changed, but you're using features from tabbrowser that I don't know enough about to identify possible issues with the way they're being used. > Are > you talking about the second patch that I requested feedback on? (and I > have more changes on the second patch anyway) Well that would have been good to know before reviewing the patch...

(In reply to Andrew Swan [:aswan] from comment #10) > This seems dangerous, if you hide and discard the tab from which the > mochitest framework is running, bad things will happen. The browser chrome test framework doesn't run in a tab.

(In reply to Dão Gottwald [::dao] from comment #13) > (In reply to Andrew Swan [:aswan] from comment #10) > > This seems dangerous, if you hide and discard the tab from which the > > mochitest framework is running, bad things will happen. > > The browser chrome test framework doesn't run in a tab. Okay, good to know. Nevertheless, I think its a good practice for tests to not unnecessarily mess with other things running in the browser. This test could quite easily just use browser.window.create() and browser.tabs.create() and then manipulate only the tabs it "owns".

(In reply to Andrew Swan [:aswan] from comment #14) > (In reply to Dão Gottwald [::dao] from comment #13) > > (In reply to Andrew Swan [:aswan] from comment #10) > > > This seems dangerous, if you hide and discard the tab from which the > > > mochitest framework is running, bad things will happen. > > > > The browser chrome test framework doesn't run in a tab. > > Okay, good to know. Nevertheless, I think its a good practice for tests to > not unnecessarily mess with other things running in the browser. This test > could quite easily just use browser.window.create() and > browser.tabs.create() and then manipulate only the tabs it "owns". Using sessionstore this way is reasonable to use here and simplifies the process of setting up the test.

(In reply to Shane Caraveo (:mixedpuppy) from comment #15) > (In reply to Andrew Swan [:aswan] from comment #14) > > (In reply to Dão Gottwald [::dao] from comment #13) > > > (In reply to Andrew Swan [:aswan] from comment #10) > > > > This seems dangerous, if you hide and discard the tab from which the > > > > mochitest framework is running, bad things will happen. > > > > > > The browser chrome test framework doesn't run in a tab. > > > > Okay, good to know. Nevertheless, I think its a good practice for tests to > > not unnecessarily mess with other things running in the browser. This test > > could quite easily just use browser.window.create() and > > browser.tabs.create() and then manipulate only the tabs it "owns". > > Using sessionstore this way is reasonable to use here and simplifies the > process of setting up the test. The important point wasn't about the method you use to create tabs, it was about the fact that the test hides (which discards) tabs that it didn't create. That works right now but I think it is sloppy.

Given bug 1384515 comment 137 (yep) the latest iteration on the patches remove all the discard requirements. I'll be working over them a bit more so if you want to give feedback be aware they may change more.

Comment on attachment 8935163 [details] Bug 1423725 add show/hide tabs api, Clearing r? until the finished product is ready

Comment on attachment 8935162 [details] Bug 1423725 add event, query and details for hidden status, I added properties for webrtc sharing modes to the webext Tab object. I also prevent hiding if webrtc sharing is occuring. I probably need to write up some test for this (will have to research doing that for webrtc). Gijs: can you look at my use of the sharing state on the tab (see tabbrowser first) let me know if you're ok withthat.

Comment on attachment 8935162 [details] Bug 1423725 add event, query and details for hidden status, I think the use of _sharingState in tabbrowser.xml is definitely OK. For the use of the property directly from the webextensions code, I think it'd be neater to have a getter of sorts in the tabbrowser binding. Florian has worked a lot with the webrtc sharing stuff, so perhaps he has other suggestions, but that's what I would recommend over relying on the "internal" underscore property. Either that or moving it to a public property, but read-only access to it seems like the better way to go here.

Comment on attachment 8935162 [details] Bug 1423725 add event, query and details for hidden status, Added a getter on tabbrowser. Test is added in the other patch.

Comment on attachment 8935162 [details] Bug 1423725 add event, query and details for hidden status, https://reviewboard.mozilla.org/r/206018/#review217046 r=me with the below addressed. ::: browser/base/content/tabbrowser.xml:1580 (Diff revision 4) > </method> > > + <method name="getTabSharingState"> > + <parameter name="aTab"/> > + <body><![CDATA[ > + return aTab._sharingState; I think it would make sense for this to use Object.assign() or similar constructs to avoid the consumers being able to modify this state (obviously only if it's non-null).

(to be clear, I only reviewed the tabbrowser changes :-) )

Comment on attachment 8935162 [details] Bug 1423725 add event, query and details for hidden status, https://reviewboard.mozilla.org/r/206018/#review217144 ::: browser/components/extensions/ext-browser.js:625 (Diff revision 4) > + return this.nativeTab.hidden; > + } > + > + get screen() { > + let state = this.window.gBrowser.getTabSharingState(this.nativeTab); > + return state && state.screen; This is a string ("Screen", "Window", "Application" or "Browser"), not a boolean. I think you'll either want to document this, or add a !! This state object is created by https://searchfox.org/mozilla-central/rev/cf149b7b63ff97023e28723167725e38cf5df757/browser/modules/ContentWebRTC.jsm#352 ::: browser/components/extensions/ext-browser.js:625 (Diff revision 4) > + return this.nativeTab.hidden; > + } > + > + get screen() { > + let state = this.window.gBrowser.getTabSharingState(this.nativeTab); > + return state && state.screen; nit: If I was writing this code, I would put a helper like: get _sharingState() { return this.window.gBrowser.getTabSharingState(this.nativeTab) || {}; } This would reduce the duplication and avoid the nullncheck in each of the 3 getters. ::: toolkit/components/extensions/ext-tabs-base.js:593 (Diff revision 4) > mutedInfo: this.mutedInfo, > isArticle: this.isArticle, > isInReaderMode: this.isInReaderMode, > + screen: this.screen, > + camera: this.camera, > + microphone: this.microphone, It's a bit unfortunate that this will call gBrowser.getTabSharingState 3 times in a row. The _sharingState getter I was proposing earlier may help here too.

Comment on attachment 8935163 [details] Bug 1423725 add show/hide tabs api, https://reviewboard.mozilla.org/r/206020/#review212518 > Really? I realize this is meant to be the first of multiple iterations but what's the long-term idea here? For now we'll go with restoring hidden tabs on shutdown

(In reply to Florian Quèze [:florian] from comment #31) > Comment on attachment 8935162 [details] > ::: toolkit/components/extensions/ext-tabs-base.js:593 > (Diff revision 4) > > mutedInfo: this.mutedInfo, > > isArticle: this.isArticle, > > isInReaderMode: this.isInReaderMode, > > + screen: this.screen, > > + camera: this.camera, > > + microphone: this.microphone, > > It's a bit unfortunate that this will call gBrowser.getTabSharingState 3 > times in a row. The _sharingState getter I was proposing earlier may help > here too. I changed the api to have a sharingstate object, rather then the individual properties, and otherwise avoided multiple gets where possible.

(In reply to Shane Caraveo (:mixedpuppy) from comment #37) > (In reply to Florian Quèze [:florian] from comment #31) > > Comment on attachment 8935162 [details] > > > ::: toolkit/components/extensions/ext-tabs-base.js:593 > > (Diff revision 4) > > > mutedInfo: this.mutedInfo, > > > isArticle: this.isArticle, > > > isInReaderMode: this.isInReaderMode, > > > + screen: this.screen, > > > + camera: this.camera, > > > + microphone: this.microphone, > > > > It's a bit unfortunate that this will call gBrowser.getTabSharingState 3 > > times in a row. The _sharingState getter I was proposing earlier may help > > here too. > > I changed the api to have a sharingstate object, rather then the individual > properties, and otherwise avoided multiple gets where possible. Which API stability guarantees are we giving to add-on authors here? Will we need to provide a backward compatibility wrapper to keep returning the current sharingState object if somehow the internal format changes in webrtcUI.jsm? Or is it find to do slight API changes here in the future?

(In reply to Florian Quèze [:florian] from comment #38) > (In reply to Shane Caraveo (:mixedpuppy) from comment #37) > > (In reply to Florian Quèze [:florian] from comment #31) > > > Comment on attachment 8935162 [details] > > I changed the api to have a sharingstate object, rather then the individual > > properties, and otherwise avoided multiple gets where possible. > > Which API stability guarantees are we giving to add-on authors here? Will we > need to provide a backward compatibility wrapper to keep returning the > current sharingState object if somehow the internal format changes in > webrtcUI.jsm? Or is it find to do slight API changes here in the future? If you're anticipating changes, or there are planned changes, I should see what those are and tighten this up now. If it's just a general thought...preferably we don't change those items in incompatible ways.

(In reply to Shane Caraveo (:mixedpuppy) from comment #39) > > Which API stability guarantees are we giving to add-on authors here? Will we > > need to provide a backward compatibility wrapper to keep returning the > > current sharingState object if somehow the internal format changes in > > webrtcUI.jsm? Or is it find to do slight API changes here in the future? > > If you're anticipating changes, or there are planned changes, I should see > what those are and tighten this up now. If it's just a general > thought...preferably we don't change those items in incompatible ways. I'm not anticipating any change right now (except maybe that the "Browser" value is almost completely dead at this point), but bug 1333468 may make some changes to add the "paused" (muted) state. In a distant future (or whenever someone makes the needed platform changes... but this hasn't happened in the last couple years), I would like to have a way to know which screen or which camera is being shared to the page.

We could sidestep the whole issue by not doing this: (In reply to Shane Caraveo (:mixedpuppy) from comment #23) > I added properties for webrtc sharing modes to the webext Tab object. I > also prevent hiding if webrtc sharing is occuring. It's not clear to me what the envisioned UX is here. If this API is supposed to be used to implement something like tab groups, then refusing to hide tabs that happen to be using webrtc seems problematic. What motivated this restriction in the first place? We do have a global sharing indicator that should take you to the sharing tab (and unhide it if needed; not sure if this currently works).

(In reply to Dão Gottwald [::dao] from comment #41) > We could sidestep the whole issue by not doing this: Not really. We still want the properties regardless of what the restrictions on hiding end up as. Tab manager extensions should also be able to provide ui based on these if they choose to. That is what bug 1408059 was for, though I dup'd to this since I needed it here anyway. > (In reply to Shane Caraveo (:mixedpuppy) from comment #23) > > I added properties for webrtc sharing modes to the webext Tab object. I > > also prevent hiding if webrtc sharing is occuring. > > It's not clear to me what the envisioned UX is here. If this API is supposed > to be used to implement something like tab groups, then refusing to hide > tabs that happen to be using webrtc seems problematic. > > What motivated this restriction in the first place? We do have a global > sharing indicator that should take you to the sharing tab (and unhide it if > needed; not sure if this currently works). UX is happening separately which is one reason why this is pref'd off for now. The initial api will be available (after users prefing on) in order to allow us to have time to further refine based on any issues that come up. The specific concern here is having camera/mic/screen sharing turned on for a tab that is not visible. Adding the sharing properties to the extension api allows extensions to determine what can be done up front. An extension could first discard the tab (which should turn off that sharing) then hide it.

Comment on attachment 8935162 [details] Bug 1423725 add event, query and details for hidden status, https://reviewboard.mozilla.org/r/206018/#review217552 What's the situation on Android? You added some properties to the TabBase class that is only implemented in the desktop Tab class. Those properties need at least stubs for Android. This also still needs: 1. a better commit message (that mentions the new media device properties) 2. tests for the new media device properties ::: browser/components/extensions/schemas/tabs.json:62 (Diff revision 6) > + "type": "object", > + "description": "Tab sharing state for screen, microphone and camera.", > + "properties": { > + "screen": { > + "type": "string", > + "optional": true, what is the difference between Window, Application, and Browser? ::: browser/components/extensions/schemas/tabs.json:67 (Diff revision 6) > + "optional": true, > + "description": "If the tab is sharing the screen the value will be one of \"Screen\", \"Window\", \"Application\" or \"Browser\"." > + }, > + "camera": { > + "type": "boolean", > + "optional": true, Why is this optional? Its a boolean, seems like it should always be present. ::: browser/components/extensions/schemas/tabs.json:673 (Diff revision 6) > "type": "integer", > "minimum": 0, > "optional": true, > "description": "The ID of the tab that opened this tab. If specified, the opener tab must be in the same window as this tab." > + }, > + "screen": { Having these be nested in the Tab object but flattened here is confusing. If they need to be top-level properties, lets call them sharingScreen, sharingCamera, etc.

Comment on attachment 8935162 [details] Bug 1423725 add event, query and details for hidden status, https://reviewboard.mozilla.org/r/206018/#review217552 Tests are in the second patch. > what is the difference between Window, Application, and Browser? These are values defined by webrtc. My bet is that they represent what level of screen sharing is occuring. The whole app, a single window, or a specific browser (tab). florian can probably expand on that. > Why is this optional? Its a boolean, seems like it should always be present. Because they are not always present in the source data. We can set them, but it doesn't make much of a difference. > Having these be nested in the Tab object but flattened here is confusing. If they need to be top-level properties, lets call them sharingScreen, sharingCamera, etc. This is exactly as MutedInfo works. Query is a flat set of options to query on, not dependent on the structure of the Tab object.

Florian, are the screen sharing values documented somewhere? See comment 44

Comment on attachment 8935162 [details] Bug 1423725 add event, query and details for hidden status, https://reviewboard.mozilla.org/r/206018/#review217552 > Because they are not always present in the source data. We can set them, but it doesn't make much of a difference. I think it does to users of the API. If the property is a boolean but the two most common values are true and undefined, that's just an unnecessary nuisance. > This is exactly as MutedInfo works. Query is a flat set of options to query on, not dependent on the structure of the Tab object. My point was more that the names don't make sense. You don't query for a tab that is `camera`. You query for a tab that is `usingCamera` or `sharingCamera` or something.

Comment on attachment 8935163 [details] Bug 1423725 add show/hide tabs api, https://reviewboard.mozilla.org/r/206020/#review217584 ::: browser/components/extensions/ext-tabs.js:83 (Diff revision 6) > }, > }; > > this.tabs = class extends ExtensionAPI { > + onShutdown(reason) { > + if (!Services.prefs.getBoolPref(TABHIDE_PREFNAME, false)) { Why check this here? Its probably an uncommon case but if a user has the pref enabled and a tab hiding extension installed then they flip the pref and then disable/uninstall the extension, their tabs remain hidden. ::: browser/components/extensions/ext-tabs.js:1008 (Diff revision 6) > tab.linkedBrowser.messageManager.sendAsyncMessage("Reader:ToggleReaderMode"); > }, > + > + show(tabIds) { > + if (!Services.prefs.getBoolPref(TABHIDE_PREFNAME, false)) { > + throw new ExtensionError(`tabs.hide is currently experimental and must be enabled with the ${TABHIDE_PREFNAME} preference.`); hide -> show ::: browser/components/extensions/ext-tabs.js:1015 (Diff revision 6) > + > + if (!Array.isArray(tabIds)) { > + tabIds = [tabIds]; > + } > + > + let tabs = tabIds.map(tabId => tabTracker.getTab(tabId)); nit: why build a new array here instead of just calling `getTab()` inside the loop below ::: browser/components/extensions/ext-tabs.js:1034 (Diff revision 6) > + > + let tabs = tabIds.map(tabId => tabTracker.getTab(tabId)); > + for (let tab of tabs) { > + tab.ownerGlobal.gBrowser.hideTab(tab); > + } > + return tabs.map(tab => tab.hidden); Did you mean to use `filter()` instead of `map()`? ::: browser/components/extensions/test/browser/browser_ext_tabs_hide.js:25 (Diff revision 6) > + } > + > + return Promise.all(windows.map(BrowserTestUtils.closeWindow)); > +} > + > +function promiseNotification(topic) { just use `TestUtils.topicObserved()` ::: browser/components/extensions/test/browser/browser_ext_tabs_hide.js:59 (Diff revision 6) > +add_task(async function test_tabs_mediaIndicators() { > + await SpecialPowers.pushPrefEnv({ > + set: [["webextensions.tabhide.enabled", true]], > + }); There are too many things being tested at once here, the sharingState properties should have their own test, in a different test file. ::: browser/components/extensions/test/browser/browser_ext_tabs_hide.js:134 (Diff revision 6) > + for (let tab of tabs) { > + browser.test.assertFalse(tab.hidden, "tab is visible"); > + } > + let ids = tabs.map(tab => tab.id); > + let hidden = await browser.tabs.hide(ids); > + browser.test.assertEq(hidden.length, tabs.length, "all tabs hidden"); You currently have `hide()` returning an array of booleans of the same length as the arguments. This should actually validate the contents of the returned array since I don't think that array of booleans is what was intended. ::: browser/components/extensions/test/browser/browser_ext_tabs_hide.js:178 (Diff revision 6) > + let oldState = SessionStore.getBrowserState(); > + let restored = promiseNotification("sessionstore-browser-state-restored"); > + SessionStore.setBrowserState(JSON.stringify(sessData)); > + await restored; > + > + extension.sendMessage("hideall"); These needs more detail. Comments describing what's happening would help, though code that looks at the contents of the onChanged events would serve the same purpose. ::: browser/components/extensions/test/browser/browser_ext_tabs_hide.js:186 (Diff revision 6) > + await extension.awaitMessage("changeInfo"); > + > + let otherwin; > + for (let win of BrowserWindowIterator()) { > + if (win != window) { > + otherwin = win; This is similarly obscure. What is otherwin here, aren't there at least 2 windows other than the main mochitest window?

Comment on attachment 8935162 [details] Bug 1423725 add event, query and details for hidden status, https://reviewboard.mozilla.org/r/206018/#review217552 > My point was more that the names don't make sense. You don't query for a tab that is `camera`. You query for a tab that is `usingCamera` or `sharingCamera` or something. All other query properties matche the name (if not structure) of what is in Tab. I prefer to maintain the consistency in that regard.

(In reply to Shane Caraveo (:mixedpuppy) from comment #45) > Florian, are the screen sharing values documented somewhere? See comment 44 The exact values are an internal detail of the WebRTC UI implementation, but it roughly matches the mediaSource constraints provided when requesting the video stream with getUserMedia. "Screen" is a video stream showing all of the user's screen. "Application" is a video stream of the size of the screen, but showing only the windows of one application. The rest of the screen is blacked out. "Window" is a video stream sharing a single window. "Browser" is a video stream of a tab. This feature has never been exposed to web pages, it was used by Firefox Hello. Given bug 742832 comment 29, I don't expect this to become available to web pages soon.

Comment on attachment 8935162 [details] Bug 1423725 add event, query and details for hidden status, https://reviewboard.mozilla.org/r/206018/#review217552 > All other query properties matche the name (if not structure) of what is in Tab. I prefer to maintain the consistency in that regard. Well then lets make the names have the same part of speech as other existing tab properties.

Comment on attachment 8935163 [details] Bug 1423725 add show/hide tabs api, https://reviewboard.mozilla.org/r/206020/#review217584 > This is similarly obscure. What is otherwin here, aren't there at least 2 windows other than the main mochitest window? There are only two navigator:browser windows in the session. I added a comment.

Andrew is out, switching r? to rpl so we can still try to get in for 59.

Comment on attachment 8935162 [details] Bug 1423725 add event, query and details for hidden status, https://reviewboard.mozilla.org/r/206018/#review219276 Hi Shane, here is a first round of review comments, mostly nits (e.g. related to part of this patch that probably belongs to the other patch and some optional proposed change related to the readability) and one related to Firefox for Android (mostly double-checking that the change applied to the mobile file are not going to break anything on the Firefox for Android tabs API). Besides that I feel that the changes to tabbrowser.xml could use an explicit review and sign-off from someone that has worked on this file, especially if it has worked or reviewed the pieces related to the tab sharing state. I'm setting it as r- for now (mainly to be notified automatically once the patch is updated again, most of my comments on this patch are currently related to optional nits) ::: browser/base/content/tabbrowser.xml:3902 (Diff revision 7) > <method name="hideTab"> > <parameter name="aTab"/> > <body> > <![CDATA[ > if (!aTab.hidden && !aTab.pinned && !aTab.selected && > - !aTab.closing) { > + !aTab.closing && !aTab._sharingState) { This part seems to make sense to me, but I'm wondering if instead of accessing `aTab._sharingState` here and defining a new `getTabSharingState(aTab)` method on gBrowser, it could be a property getter of the tabbrowser-tab, as for `aTab.closing`, `aTab.pinned` etc. (https://searchfox.org/mozilla-central/rev/48cbb200aa027a0a379b6004b6196a167344b865/browser/base/content/tabbrowser.xml#7851-7865) Do you mind to add an additional reviewer which may double-check the changes applied to tabbrowser.xml? (e.g. looking at the tabbrowser.xml hg logs, johannh and Gijs seem to have reviewed changes to this file related to the sharing state of the tabs). ::: browser/components/extensions/test/browser/browser_ext_tabs_sharingState.js:4 (Diff revision 7) > + await SpecialPowers.pushPrefEnv({ > + set: [["webextensions.tabhide.enabled", true]], > + }); This preference doesn't seem to be needed yet in this test (I guess that the other patch adds more assertion in this file that are going to need it) Also, it looks that the preference itself is going to be actually introduced in the other patch from this mozreview request (it is no probably preventing this test to pass even without the other patch, but it is worth a mention). ::: browser/components/extensions/test/browser/browser_ext_tabs_sharingState.js:18 (Diff revision 7) > + let state = testTab.sharingState; > + browser.test.assertTrue(state.camera, "sharing camera was turned on"); > + browser.test.assertTrue(state.microphone, "sharing mic was turned on"); > + browser.test.assertEq(state.screen, "Window", "sharing screen is window"); Nit, it looks that we could merge these assertions into a single `Assert.deepEqual` assertion it could potentially fail if the sharing state object content is changed, but if we change it to use `testTab.gBrowser.getTabSharingState`, it could be a good thing, because it could help to catch regressions related to new properties added to `getTabSharingState` that we didn't expect (and be able to change the schema to reflect that). ::: browser/components/extensions/test/browser/browser_ext_tabs_sharingState.js:40 (Diff revision 7) > + } > + let state = tab.sharingState; > + browser.test.assertFalse(state.camera, "sharing camera was turned off"); > + browser.test.assertFalse(state.microphone, "sharing mic was turned off"); > + browser.test.assertFalse(state.screen, "sharing screen was turned off"); > + browser.test.sendMessage("done"); Any reason for not using `browser.test.notifyPass` here and `extension.awaitFinish` at line 57? ::: browser/components/extensions/test/browser/browser_ext_tabs_sharingState.js:46 (Diff revision 7) > + }); > + browser.test.sendMessage("ready"); > + } > + > + let extdata = { > + manifest: {permissions: ["tabs", "tabHide"]}, "tabHide" doesn't seem to be defined yet (or needed by this version of this test, I guess that it will be needed because of changes introduced in this file by the other patch in this mozreview request). ::: browser/components/extensions/test/browser/browser_ext_tabs_sharingState.js:53 (Diff revision 7) > + background, > + }; > + let extension = ExtensionTestUtils.loadExtension(extdata); > + await extension.startup(); > + > + // test onUpdated Nit, how about a more descriptive comment? e.g. // Wait the extension to be ready and then // update the sharing state of the tab to ensure // that a tabs.onUpdated event is fired. ::: mobile/android/components/extensions/ext-utils.js:580 (Diff revision 7) > + get hidden() { > + return false; > + } > + > + get sharingState() { > + return { > + screen: undefined, > + microphone: false, > + camera: false, > + }; > + } It looks that these additions are not going to be tested on Android in this patch, as a first step it could be reasonable to just be sure that the existent Android WE tests are going to trigger this code and not going to break with this changes (e.g. iirc when our tests are running in debug, the values resolved by our APIs are checked against the schema and they may raise exception that doesn't happen in non-debug mode, and it looks that we are not applied any change to the Android API schema files in this patch, e.g. defining the presence of the sharingState) ::: toolkit/components/extensions/ext-tabs-base.js:530 (Diff revision 7) > - if (queryInfo.muted !== null) { > - if (queryInfo.muted !== this.mutedInfo.muted) { > + if (queryInfo.muted !== null && queryInfo.muted !== this.mutedInfo.muted) { > + return false; > + } > + > + let state = this.sharingState; > + if (["camera", "microphone"].some(prop => queryInfo[prop] != null && queryInfo[prop] !== state[prop])) { Nit, the check itself here seems good to me, but (at least in my opinion) it is a bit big as an expression to have inside the if condition itself, how about moving the arrow function out of it and giving it a descriptive name? e.g. ``` let state = this.sharingState; let checkSharingState = (prop) => { // return true if the property is part of // the query and it doesn't match the // current sharing state of the tab. return queryInfo[prop] != null && queryInfo[prop] !== state[prop]; } if (["camera","microphone"].some(checkSharingState)) { ... } ``` ::: toolkit/components/extensions/ext-tabs-base.js:535 (Diff revision 7) > + if (["camera", "microphone"].some(prop => queryInfo[prop] != null && queryInfo[prop] !== state[prop])) { > + return false; > + } > + // query for screen can be boolean (ie. any) or string (ie. specific). > + if (queryInfo.screen !== null) { > + let match = typeof queryInfo.screen == "boolean" ? queryInfo.screen === !!state.screen : queryInfo.screen === state.screen; Nit, this could be probably more readable if it spans on more then one line, I mean something like: ``` let match = typeof(queryInfo.screen) === "boolean" ? (queryInfo.screen === !!state.screen) : (queryInfo.screen === state.screen); ```

Comment on attachment 8935163 [details] Bug 1423725 add show/hide tabs api, https://reviewboard.mozilla.org/r/206020/#review219098 Here is the first round of review comments for this second patch, some are related to test failures that require some further change, and the other ones related to questions/doubts on some of the implementation details. ::: browser/components/extensions/ext-tabs.js:89 (Diff revision 7) > + return; > + } > + if (reason == "ADDON_DISABLE" || > + reason == "ADDON_UNINSTALL") { > + // Show all hidden tabs if a tab managing extension is uninstalled or > + // disabled. If a user has more than one, the extensions will need to What if, while an extension is being disabled/uninstalled, another extension is in the progress of hiding a number of tabs? Could we keep track of the tabs hidden by a particular extension by using a WeakSet? This way we could just filter out any tab that wasn't hidden by that extension by checking if `extensionTabsHiddenWeakSet.has(tab)` (without keeping a strong reference to them, so that they can be garbage collected, and without the need to manually clean the WeakSet if the tab is being closed), and then remove the tab from the WeakSet when we receive a TabShow event. ::: browser/components/extensions/ext-tabs.js:91 (Diff revision 7) > + if (reason == "ADDON_DISABLE" || > + reason == "ADDON_UNINSTALL") { > + // Show all hidden tabs if a tab managing extension is uninstalled or > + // disabled. If a user has more than one, the extensions will need to > + // self-manage re-hiding tabs. > + let tabs = Array.from(tabTracker._tabIds.values()); `tabTracker._tabIds` looks like an implementation detail of `tabTracker` itself (I mean something that was not supposed to be used externally and that may change), it could be safer if we define an iterator on the tabTracker, so that it is much more clear that it is something that it is going to be used from code external to the tabTracker's own code. Also, `TabTracker` has two implementation: one implemention for Firefox Desktop and another one implemented for Fennec, and (at least iirc) the one from Fennec doesn't have a `_tabIds` Map and so this line is likely to raise an exception on Android. ::: browser/components/extensions/ext-tabs.js:93 (Diff revision 7) > + // Show all hidden tabs if a tab managing extension is uninstalled or > + // disabled. If a user has more than one, the extensions will need to > + // self-manage re-hiding tabs. > + let tabs = Array.from(tabTracker._tabIds.values()); > + for (let tab of tabs) { > + tab.ownerGlobal.gBrowser.showTab(tab); Nit, I'm wondering if it would make sense to also show the hidden tabs when the preference that locks the tabHide permission is turned to false (e.g. I'm thinking about an extension that may want to use the tabs hiding API if enabled, and the user then set the about:config preference to false without hiding the tab, maybe because he still wants to use that extension, as an example "tab center redux" may use this API only if available) (but maybe it is not worth it because the API is still experimental and it is not supposed to be used by regular users, and so I'm not marking it as an issue). ::: browser/components/extensions/ext-tabs.js:93 (Diff revision 7) > + // Show all hidden tabs if a tab managing extension is uninstalled or > + // disabled. If a user has more than one, the extensions will need to > + // self-manage re-hiding tabs. > + let tabs = Array.from(tabTracker._tabIds.values()); > + for (let tab of tabs) { > + tab.ownerGlobal.gBrowser.showTab(tab); We could also avoid to call gBrowser.showTab if the tab is not currently hidden. ::: browser/components/extensions/ext-tabs.js:93 (Diff revision 7) > + // Show all hidden tabs if a tab managing extension is uninstalled or > + // disabled. If a user has more than one, the extensions will need to > + // self-manage re-hiding tabs. > + let tabs = Array.from(tabTracker._tabIds.values()); > + for (let tab of tabs) { > + tab.ownerGlobal.gBrowser.showTab(tab); It seems that the tests are currently raising exceptions from this line, e.g.: - https://treeherder.mozilla.org/logviewer.html#?job_id=156871100&repo=try&lineNumber=2920 apparently tab.ownerGlobal is undefined when this code is executing (I did't investigate the exception too deeply, but it may be because the tab is already closing/closed and it has been removed from the DOM, but it is still tracked by the `tabTracker._tabIds` map). Not all the tests are currently failing because of this exception, but it is happening after every one of the tests from this file (and it is failing explicitly in the test where we check that the tabs are not hidden anymore after uninstalling the test extension: https://treeherder.mozilla.org/logviewer.html#?job_id=156871100&repo=try&lineNumber=3043). ::: browser/components/extensions/ext-tabs.js:1004 (Diff revision 7) > > tab.linkedBrowser.messageManager.sendAsyncMessage("Reader:ToggleReaderMode"); > }, > + > + show(tabIds) { > + if (!Services.prefs.getBoolPref(TABHIDE_PREFNAME, false)) { Even if we are defaulting this preference to false here when missing, I think that it would be good to also add it explicitly (and set to false) also in here: - https://searchfox.org/mozilla-central/rev/48cbb200aa027a0a379b6004b6196a167344b865/modules/libpref/init/all.js#5008-5020 I'm also wondering if we should also consider to allow the preference to be used to enable the API only when running in nightly (and maybe beta), so that the API cannot be enabled on release until we decide that it can be turned on by default. ::: browser/components/extensions/ext-tabs.js:1014 (Diff revision 7) > + tabIds = [tabIds]; > + } > + > + for (let tabId of tabIds) { > + let tab = tabTracker.getTab(tabId); > + tab.ownerGlobal.gBrowser.showTab(tab); Given that the tests are failing on similar code executed from `onShutdown`, I think that we should also be careful with accessing `tab.ownerGlobal.gBrowser` here without having checked that `ownerGlobal` is actually defined (e.g. if `ownerGlobal` becomes undefined because the tab has been already removed from the DOM, it could also happen here if the user or another extension removed the tab in the time between when the extension has got the array of the tabs that it wants to remove and when this is code is actually executed), and in the hide method as well. ::: browser/components/extensions/ext-tabs.js:1018 (Diff revision 7) > + let tab = tabTracker.getTab(tabId); > + tab.ownerGlobal.gBrowser.showTab(tab); > + } > + }, > + > + hide(tabIds, options) { `options` seems to be a leftover (it seems to be both unused in the code and not defined in the schema) ::: browser/components/extensions/ext-tabs.js:1031 (Diff revision 7) > + > + let tabs = tabIds.map(tabId => tabTracker.getTab(tabId)); > + for (let tab of tabs) { > + tab.ownerGlobal.gBrowser.hideTab(tab); > + } > + return tabs.filter(tab => tab.hidden).map(tab => tabTracker.getId(tab)); If I'm not reading this wrong, it seems that we are going to return all the tabs hidden instead of just the ones hidden by the single API call, is it on purpose? Otherwise we should probably filter the result using the `[tabIds]` array (and check this behavior with an additional assertion or test case in the new tabHide tests). ::: browser/components/extensions/test/browser/browser_ext_tabs_hide.js:3 (Diff revision 7) > +"use strict"; > + > +function* BrowserWindowIterator() { it looks that we have a number of this kind of helpers around, maybe we can move this one in our head.js, at least we can at least avoid to have more than one of it in our own test files :-) ::: browser/components/extensions/test/browser/browser_ext_tabs_hide.js:14 (Diff revision 7) > + } > + } > +} > + > +// Ensure the pref prevents API use when the extension has the tabHide permission. > +add_task(async function test_tabs_enabled() { Nit, how about renaming it into something like `test_pref_disabled`? ::: browser/components/extensions/test/browser/browser_ext_tabs_hide.js:22 (Diff revision 7) > + let ids = tabs.map(tab => tab.id); > + try { > + await browser.tabs.hide(ids); > + browser.test.notifyFail("pref-test"); > + } catch (e) { > + browser.test.assertTrue(e.message.includes("tabs.hide is currently experimental"), "pref not enabled"); It looks that we are not going to know which was the actual error if this assertion fails, and the test would also get stuck until it fails for timeout. How about using `browser.test.assertRejects` here, e.g. something like: ``` await browser.test.assertRejects( browser.tabs.hide(ids), /tabs.hide is currently experimental/, "Got the expected error when pref not enabled" ).catch(err => { browser.test.notifyFail("pref-test"); throw err; }); browser.test.notifyPass("pref-test"); ``` ::: browser/components/extensions/test/browser/browser_ext_tabs_hide.js:29 (Diff revision 7) > + } > + } > + > + let extdata = { > + manifest: {permissions: ["tabs", "tabHide"]}, > + useAddonManager: "temporary", is this necessary? ::: browser/components/extensions/test/browser/browser_ext_tabs_hide.js:78 (Diff revision 7) > + }); > + } > + > + let extdata = { > + manifest: {permissions: ["tabs", "tabHide"]}, > + useAddonManager: "temporary", is this necessary? ::: browser/components/extensions/test/browser/browser_ext_tabs_hide.js:87 (Diff revision 7) > + await extension.startup(); > + > + let sessData = { > + windows: [{ > + tabs: [ > + {entries: [{url: "about:blank"}]}, It looks that this session data is going to need a valid triggeringPrincipal_base64 property, otherwise the test is going to crash in debug mode (because of Bug 1345433, "Ensure tests load pass valid triggeringPrincipal") e.g. this looks one of this kind of failure from a try push of the patches in this mozreview request: - https://treeherder.mozilla.org/logviewer.html#?job_id=156870172&repo=try&lineNumber=13362 The following snippet could be helpful to fix this crash: https://searchfox.org/mozilla-central/rev/48cbb200aa027a0a379b6004b6196a167344b865/browser/components/extensions/test/browser/browser_ext_tabs_lazy.js#3-13 ::: browser/components/extensions/test/browser/browser_ext_tabs_hide.js:131 (Diff revision 7) > + > + // Test closing the last visible tab, the next tab which is hidden should become > + // the selectedTab and will be visible. > + ok(!otherwin.gBrowser.selectedTab.hidden, "selected tab is not hidden"); > + await BrowserTestUtils.removeTab(otherwin.gBrowser.selectedTab); > + ok(!otherwin.gBrowser.selectedTab.hidden, "tab was unhidden"); I guess that this one is the next hidden tab that we expected to become visible because we removed the last visible tab, do you mind to also assert that is the expected one? (e.g. checking the tab id or the tab url).

Comment on attachment 8935162 [details] Bug 1423725 add event, query and details for hidden status, https://reviewboard.mozilla.org/r/206018/#review219276 > This part seems to make sense to me, but I'm wondering if instead of accessing `aTab._sharingState` here and defining a new `getTabSharingState(aTab)` method on gBrowser, it could be a property getter of the tabbrowser-tab, as for `aTab.closing`, `aTab.pinned` etc. (https://searchfox.org/mozilla-central/rev/48cbb200aa027a0a379b6004b6196a167344b865/browser/base/content/tabbrowser.xml#7851-7865) > > Do you mind to add an additional reviewer which may double-check the changes applied to tabbrowser.xml? > > (e.g. looking at the tabbrowser.xml hg logs, johannh and Gijs seem to have reviewed changes to this file related to the sharing state of the tabs). I'm clearing this issue myself, I just noticed that this file has been already reviewed by :Gijs in Comment 29.

Comment on attachment 8935162 [details] Bug 1423725 add event, query and details for hidden status, https://reviewboard.mozilla.org/r/206018/#review219382 I noticed that the tabbrowser.xml pieces have been already r+ before, there should not be any further big changes needed on this patch, and so I'm setting this patch to r+ (possibly with some of the nits fixed by moving the pieces that doesn't belong to this patch to the other patch) Looking at the try push, it seems that the android tabs API tests are running successfully with this changes applied, and so that part seems also to be ok.

Comment on attachment 8935162 [details] Bug 1423725 add event, query and details for hidden status, https://reviewboard.mozilla.org/r/206018/#review219276 > Nit, it looks that we could merge these assertions into a single `Assert.deepEqual` assertion > > it could potentially fail if the sharing state object content is changed, but if we change it to use `testTab.gBrowser.getTabSharingState`, it could be a good thing, because it could help to catch regressions related to new properties added to `getTabSharingState` that we didn't expect (and be able to change the schema to reflect that). The code is in a background script, we dont have access to gBrowser or Assert. > Nit, the check itself here seems good to me, but (at least in my opinion) it is a bit big as an expression to have inside the if condition itself, how about moving the arrow function out of it and giving it a descriptive name? > e.g. > > ``` > let state = this.sharingState; > let checkSharingState = (prop) => { > // return true if the property is part of > // the query and it doesn't match the > // current sharing state of the tab. > return queryInfo[prop] != null && > queryInfo[prop] !== state[prop]; > } > > if (["camera","microphone"].some(checkSharingState)) { > ... > } > ``` I've made a change here I'll want you to take a look at.

Comment on attachment 8935162 [details] Bug 1423725 add event, query and details for hidden status, https://reviewboard.mozilla.org/r/206018/#review219276 > The code is in a background script, we dont have access to gBrowser or Assert. yeah, that's true, and there is no browser.test.assertDeepEqual unfortunately. On the part related to checking the getTabSharingState, how about doing (clearly out of the extension code) something like the following to keep an eye on new properties that may be added on it? ``` const expectedSharingState = {screen: "Window", microphone: true, camera: true}; gBrowser.setBrowserSharing(tab.linkedBrowser, expectedSharingState); assert.deepEqual(gBrowser.getTabSharingState(tab), expectedSharingState, "Got the expected sharing state object from gBrowser.getTabSharingState"); ```

Comment on attachment 8935163 [details] Bug 1423725 add show/hide tabs api, https://reviewboard.mozilla.org/r/206020/#review219098 > We could also avoid to call gBrowser.showTab if the tab is not currently hidden. showTab itself makes that check. I have added checks where it makes sense. > I guess that this one is the next hidden tab that we expected to become visible because we removed the last visible tab, do you mind to also assert that is the expected one? (e.g. checking the tab id or the tab url). I'm not really concerned *which* tab is selected, that is a behavior of tabbrowser. I am wanting to make sure there is an unhidden selected tab.

Comment on attachment 8935162 [details] Bug 1423725 add event, query and details for hidden status, https://reviewboard.mozilla.org/r/206018/#review219800 I took another look to this patch (mostly to the changes related to the new `checkProperty` function) and follows two more comments (just one more nit in the test, and a note on a small difference in the code that checks the queryInfo between the old and the new version of the internal `tab.matches(queryInfo)` method). ::: toolkit/components/extensions/ext-tabs-base.js:522 (Diff revisions 7 - 8) > matches(queryInfo) { > const PROPS = ["active", "audible", "cookieStoreId", "discarded", "hidden", > "highlighted", "index", "openerTabId", "pinned", "status"]; > > - if (PROPS.some(prop => queryInfo[prop] != null && queryInfo[prop] !== this[prop])) { > + function checkProperty(prop, obj) { > + return queryInfo[prop] !== null && queryInfo[prop] !== obj[prop]; Extracting the shared property checks into the `checkProperty` function looks like a good idea, it makes the function much more readable. There is just a small difference between the previous and the new implementation that I noticed (but it should not be a problem because `queryInfo` should have been normalized by the schema wrappers before they reach this function, and so the `queryInfo` properties should never be `undefined` even if explicitly set to `undefined` in the tabs.query API call): - in the previous version `queryInfo[prop]` was checked using `queryInfo[prop] != null` which is going to return `false` for both `null` and `undefined`, while in the new `checkProperty` function is always checked using `queryInfo[prop] !== null` (besides queryInfo.muted which was already checked using `!== null` in the original version) This difference could become a problem only if there are going to be callers of this function which are not getting the queryInfo from an API call (and right now the only caller is tabs.query: https://searchfox.org/mozilla-central/search?q=symbol:tab%23matches&redirect=false). ::: browser/components/extensions/test/browser/browser_ext_tabs_sharingState.js:43 (Diff revision 8) > + browser.test.sendMessage("ready"); > + } > + > + let extdata = { > + manifest: {permissions: ["tabs"]}, > + useAddonManager: "temporary", Nit, this is probably not needed as well.

Comment on attachment 8935162 [details] Bug 1423725 add event, query and details for hidden status, https://reviewboard.mozilla.org/r/206018/#review219800 > Extracting the shared property checks into the `checkProperty` function looks like a good idea, it makes the function much more readable. > > There is just a small difference between the previous and the new implementation that I noticed (but it should not be a problem because `queryInfo` should have been normalized by the schema wrappers before they reach this function, and so the `queryInfo` properties should never be `undefined` even if explicitly set to `undefined` in the tabs.query API call): > > - in the previous version `queryInfo[prop]` was checked using `queryInfo[prop] != null` which is going to return `false` for both `null` and `undefined`, while in the new `checkProperty` function is always checked using `queryInfo[prop] !== null` (besides queryInfo.muted which was already checked using `!== null` in the original version) > > This difference could become a problem only if there are going to be callers of this function which are not getting the queryInfo from an API call (and right now the only caller is tabs.query: https://searchfox.org/mozilla-central/search?q=symbol:tab%23matches&redirect=false). I'm going to fix that. > Nit, this is probably not needed as well. It is necessary in this test, it is testing onShutdown functionality.

Comment on attachment 8935163 [details] Bug 1423725 add show/hide tabs api, https://reviewboard.mozilla.org/r/206020/#review219768 Hi Shane, I see that the last round of changes have fixed the test failure related to the closing tabs and the session restore. Follows another round of review comments on this patch (some additional nit and one last round of questions related to the code that is handling hiding and un-hiding of the tabs) ::: browser/components/extensions/ext-tabs.js:26 (Diff revisions 7 - 8) > } = ExtensionUtils; > > -const TABHIDE_PREFNAME = "webextensions.tabhide.enabled"; > +const TABHIDE_PREFNAME = "extensions.webextensions.tabhide.enabled"; > + > +// WeakMap[Tab -> Extension] > +let hiddenTabs = new WeakMap(); How about `WeakMap[Tab -> extensionId]`? so that, by using the extensionId string, we exclude any risk of leaking the extension object because of race conditions between the code that clean up the hiddenTabs when the extension is being disable/uninstalled and other extensions that may try to hide the same tab. ::: browser/components/extensions/ext-tabs.js:96 (Diff revisions 7 - 8) > // Show all hidden tabs if a tab managing extension is uninstalled or > // disabled. If a user has more than one, the extensions will need to > // self-manage re-hiding tabs. > - let tabs = Array.from(tabTracker._tabIds.values()); > - for (let tab of tabs) { > - tab.ownerGlobal.gBrowser.showTab(tab); > + for (let tab of this.extension.tabManager.query()) { > + let nativeTab = tabTracker.getTab(tab.id); > + if (nativeTab.hidden && hiddenTabs.get(nativeTab) === this.extension) { is the nativeTab.hidden check needed? an inline comment that explain why we need to check `nativeTab.hidden` before looking into the `hiddenTabs` WeakMap may be helpful. ::: browser/components/extensions/ext-tabs.js:98 (Diff revisions 7 - 8) > // self-manage re-hiding tabs. > - let tabs = Array.from(tabTracker._tabIds.values()); > - for (let tab of tabs) { > - tab.ownerGlobal.gBrowser.showTab(tab); > + for (let tab of this.extension.tabManager.query()) { > + let nativeTab = tabTracker.getTab(tab.id); > + if (nativeTab.hidden && hiddenTabs.get(nativeTab) === this.extension) { > + hiddenTabs.delete(nativeTab); > + if (nativeTab.ownerGlobal) { Nit, how about doing the opposite? ``` for (...) { ... if (!nativeTab.ownerGlobal) { continue; } ... } ``` (also for the same check in the show and hide methods). ::: browser/components/extensions/ext-tabs.js:304 (Diff revisions 7 - 8) > needed.push("discarded"); > } else if (event.type == "TabBrowserDiscarded") { > needed.push("discarded"); > } else if (event.type == "TabShow") { > needed.push("hidden"); > + // Always remove the tab from the hiddenTabs map Nit, close the comment with a '.' ::: browser/components/extensions/ext-tabs.js:1025 (Diff revisions 7 - 8) > } > > for (let tabId of tabIds) { > let tab = tabTracker.getTab(tabId); > + if (tab.ownerGlobal) { > - tab.ownerGlobal.gBrowser.showTab(tab); > + tab.ownerGlobal.gBrowser.showTab(tab); I'm wondering if it would make sense to remove the tab from the `hiddenTabs` WeakMap here, given that we already know that it is going to be unhidden by the API method. ::: modules/libpref/init/all.js:5024 (Diff revision 8) > // Whether or not the moz-extension resource loads are remoted. For debugging > // purposes only. Setting this to false will break moz-extension URI loading > // unless other process sandboxing and extension remoting prefs are changed. > pref("extensions.webextensions.protocol.remote", true); > > +// disable tab hiding API Nit, capitalize the first letter and close the comment with a '.'. (Nit Nit, How about `// Disable experimental tab hiding API.`?)

Clean try except android debug. https://treeherder.mozilla.org/#/jobs?repo=try&revision=ed60e624fb48&selectedJob=157252470 Last rb push adds changes to the schema for android to deal with that. Next try will be android only.

Pushed by mixedpuppy@gmail.com: https://hg.mozilla.org/integration/autoland/rev/c7cc4bef1e90 add event, query and details for hidden status, r=Gijs,rpl https://hg.mozilla.org/integration/autoland/rev/ea7705cc940f add show/hide tabs api, r=rpl

https://hg.mozilla.org/mozilla-central/rev/c7cc4bef1e90 https://hg.mozilla.org/mozilla-central/rev/ea7705cc940f

Gingerbread commented https://addons.mozilla.org/firefox/addon/hide-tab/ at https://bugzilla.mozilla.org/show_bug.cgi?id=1128316#c4 > This add-on requires a newer version of Firefox (at least version 59.0a1). You are using Firefox 57.0.

Even though this is behind a pref right now, I'd like to get this documented on MDN as an *experimental* API so that developers can start trying it and the team can get feedback on its usefulness, limitations and potential security issues.

William, see comment 77

Sure, it's fine to document things that are still behind a pref. We do this all the time :). There's a flag in the compat data to represent this state: https://github.com/mdn/browser-compat-data/blob/master/compat-data-schema.md#flags.

This issue is verified as fixed on Firefox 60.0a1 (20180204220351) and Firefox 59.0b6 (20180201171410) under Wind 7 64-bit and Mac OS X 10.13.2 The preference “extensions.webextensions.tabhide.enabled” (set to false by default) is displayed in about:config. The extension from https://bugzilla.mozilla.org/show_bug.cgi?id=1423725#c76 was used for the testing. Please see the attached video and screenshot.

Hi guys, I tried it and it's great, but can I ask for a little feature? I'd like to really hide tabs in a sense that if I have something private I don't want tabs to pop out when last visible was closed. It kills the point of hiding them in the first place. My suggestion would be to open a new empty tab if last visible tab was closed, or activate a pinned one if available instead of unhiding invisible tabs. Is it possible to implement?

Oh ok, can't edit. If there is a pinned tab it will go to it, which is fine. But if there are no pinned tabs it will unhide hidden tab.

Some docs: https://developer.mozilla.org/en-US/Add-ons/WebExtensions/API/tabs/hide https://developer.mozilla.org/en-US/Add-ons/WebExtensions/API/tabs/show Please let me know if we need anything else. I've added a bit on experimentalness and it being behind a pref, but will remove that once the browser compat data is updated. I wasn't sure whether this is available on Firefox for Android. I think not but would appreciate confirmation.

Is there any way to really highlight at the top that this is currently experimental and subject to change? I see the note lower in the docs, but I'd like to see a blazing warning. Otherwise, LGTM. Android does not support this.

I think that's probably a good idea -> https://developer.mozilla.org/en-US/Add-ons/WebExtensions/API/tabs/hide