The first stage should include basic mapping of Fennec tabs to WebExtension tab objects, tagging of contexts with the correct tab IDs, and basic query support.

Comment on attachment 8831506 [details] Bug 1260548: Part 1 - Factor out the common functionality of the tabs API. https://reviewboard.mozilla.org/r/108052/#review109356 Whew, this is a big one but there are lots of excellent improvements here, thanks! I also understand some of your comments on bug 1307348 a lot better now. My only real feedback (and there's a line-item comment related to this below) is that we could really use some high-level documentation about all the pieces here (the `*Manager` and `*Tracker` classes for instance). Reading their individual implementations is the best option right now, but that is more confusing than it needs to be since they user a bunch of existing components that require methods with specific names (observe/handleEvent/etc) but these methods are not distinguished from public methods and other internal methods. I think we can both make those classes more readable (by clearly annotating individual methods) and also add documentation (though that can be separated from this bug) ::: browser/components/extensions/ext-tabs.js:463 (Diff revision 1) > > - return TabManager.convert(extension, tab); > + return tabManager.convert(tab); > }); > }, > > - remove: function(tabs) { > + async remove(tabs) { Ooh, when did we get the green light to use async? ::: browser/components/extensions/ext-tabs.js:767 (Diff revision 1) > newTab.addEventListener("SSTabRestored", function listener() { > // Once it has been restored, select it and return the promise. > - newTab.removeEventListener("SSTabRestored", listener); > gBrowser.selectedTab = newTab; > - return resolve(TabManager.convert(extension, newTab)); > - }); > + > + return resolve(tabManager.convert(newTab)); i don't think resolve() actually returns anything so the return here is pointless? ::: browser/components/extensions/test/browser/browser_ext_tabs_executeScript_no_create.js:24 (Diff revision 1) > // of removing the listener. This is to minimize any IPC, since the bug that > // is being tested is sensitive to timing. > let ignore = false; > let url; > browser.tabs.onUpdated.addListener((tabId, changeInfo, tab) => { > - if (changeInfo.status === "loading" && tab.url === url && !ignore) { > + if (url && changeInfo.status === "loading" && tab.url === url && !ignore) { What changed that requires this? ::: toolkit/components/extensions/ExtensionTabs.jsm:67 (Diff revision 1) > + if (this.hasTabPermission) { > + return this._url; > + } > + } > + > + get uri() { So we have a uri property that is an nsIURI and a url property that is the string representation of the uri? That feels clunky... ::: toolkit/components/extensions/ExtensionTabs.jsm:86 (Diff revision 1) > + } > + } > + > + get favIconUrl() { > + if (this.hasTabPermission) { > + return this._favIconUrl; Add a comment above that this is meant to be implemented in derived classes? I notice in other places you put an implementation that throws an Error into the base class... Same for other getters that are expected to be implemented in derived classes. ::: toolkit/components/extensions/ExtensionTabs.jsm:131 (Diff revision 1) > + if (this.hasTabPermission) { > + for (let prop of ["url", "title", "favIconUrl"]) { > + let val = this[`_${prop}`]; > + if (val) { > + result[prop] = val; > + } > + } > + } why not just use the uri/title/favIconUrl getters ::: toolkit/components/extensions/ExtensionTabs.jsm:323 (Diff revision 1) > + > + get haveListeners() { > + return this._openListeners.size > 0 || this._closeListeners.size > 0; > + } > + > + addOpenListener(listener) { Just reading from here down, we have a mix of methods that are clearly part of the public API (eg addListener), callbacks that have to be named as they are to implement other interfaces (eg observer, handleEvent, etc) and other methods that are unclear to me (eg this one, addOpenListener). A comment laying this all out or, ideally, grouping the private methods separately from the public ones, would help a lot I think.

Comment on attachment 8831507 [details] Bug 1260548: Part 2 - Factor out the excuteScript/insertCSS logic. https://reviewboard.mozilla.org/r/108054/#review109402 ::: browser/components/extensions/ext-tabs.js:125 (Diff revision 2) > awaitTabReady(tab) { > let deferred = this.tabReadyPromises.get(tab); > if (!deferred) { > deferred = PromiseUtils.defer(); > - if (!this.initializingTabs.has(tab) && tab.linkedBrowser.innerWindowID) { > + if (!this.initializingTabs.has(tab) && (tab.linkedBrowser.innerWindowID || > + tab.linkedBrowser.currentURI.spec === "about:blank")) { how is this related to the rest of this patch? ::: browser/components/extensions/ext-tabs.js:148 (Diff revision 2) > return tabTracker.getTab(tabId); > } > return tabTracker.activeTab; > } > > + async function promiseTabOrActive(tabId) { This name isn't very clear, add Ready to the end? ::: browser/components/extensions/ext-tabs.js:461 (Diff revision 2) > > if (createProperties.pinned) { > window.gBrowser.pinTab(tab); > } > > - if (createProperties.url && !createProperties.url.startsWith("about:")) { > + if (createProperties.url && createProperties.url !== window.BROWSER_NEW_TAB_URL) { Again, I don't understand how this is related to the rest of this patch ::: browser/components/extensions/ext-tabs.js:623 (Diff revision 2) > > - executeScript: function(tabId, details) { > - return self.tabs._execute(tabId, details, "js", "executeScript"); > + async insertCSS(tabId, details) { > + let tab = await promiseTabOrActive(tabId); > - }, > > - insertCSS: function(tabId, details) { > + return tab.insertCSS(context, details); I suspect I'm misunderstanding something here, but why isn't this `return await tab.insertCSS...`?

Comment on attachment 8831508 [details] Bug 1260548: Part 3 - Factor out the extension popup code into its own module. https://reviewboard.mozilla.org/r/108056/#review109412

Comment on attachment 8831506 [details] Bug 1260548: Part 1 - Factor out the common functionality of the tabs API. https://reviewboard.mozilla.org/r/108052/#review109356 Yeah, I was planning to go through and add doc comments to everything. I just wanted to get the patches out as soon as possible to avoid as much rebase hell as I could. > Ooh, when did we get the green light to use async? I've always been aiming for 54. If any issues come up or someone objects, I'll fix them or take the fall. In any case, there's already a fair amount of usage in the tree. > i don't think resolve() actually returns anything so the return here is pointless? Oops. Yeah, I have no idea why that was there. > What changed that requires this? Nothing. The test was briefly failing because a bug was causing `tab.url` to be undefined, and the initial undefined value caused the handler to be executed early. The result was confusing, and I just wanted to avoid letting it happen again. > So we have a uri property that is an nsIURI and a url property that is the string representation of the uri? That feels clunky... Yeah, I'm not especially happy about it either. The `url` property is there because that's the property name on the tab object we expose to content, and I want all of those properties to be consistent. The `uri` property is just kind of handy, but it also kind of bugs me. > Add a comment above that this is meant to be implemented in derived classes? > I notice in other places you put an implementation that throws an Error into the base class... > Same for other getters that are expected to be implemented in derived classes. Yeah, I should have added comments about these. The properties that are gated on permissions have the underscored values implemented in the derived class, and the gated getters implemented in the base class. > why not just use the uri/title/favIconUrl getters Just for the sake of performance. Those getters all check for tab permissions, and while that's not especially expensive with the current implementation, I'd still rather avoid the overhead. I'll add a comment. > Just reading from here down, we have a mix of methods that are clearly part of the public API (eg addListener), callbacks that have to be named as they are to implement other interfaces (eg observer, handleEvent, etc) and other methods that are unclear to me (eg this one, addOpenListener). > A comment laying this all out or, ideally, grouping the private methods separately from the public ones, would help a lot I think. Yeah, I was actually thinking about adding underscores to all of the private method names. I decided to stop waffling over it and just push, but it's probably a good idea.

Comment on attachment 8831509 [details] Bug 1260548: Part 4 - Factor out tab status listener logic into shared module. https://reviewboard.mozilla.org/r/108058/#review109416

Comment on attachment 8831507 [details] Bug 1260548: Part 2 - Factor out the excuteScript/insertCSS logic. https://reviewboard.mozilla.org/r/108054/#review109402 > how is this related to the rest of this patch? When I changed the `executeScript` calls to wait for the tab to load before validating arguments, one of the tests that checked invalid arguments failed, because the promise never resolved for the tab it opened with an explicit "about:blank" URL. > Again, I don't understand how this is related to the rest of this patch Same issue as the other change. This code path is only necessary for the new tab URL. Taking it for about:blank breaks things, but the tests were previously missing the issue. > I suspect I'm misunderstanding something here, but why isn't this `return await tab.insertCSS...`? Because `return await` is Considered Harmful. Returning a promise has exactly the same effect as returning its resolution value.

Comment on attachment 8831510 [details] Bug 1260548: Part 5 - Factor out <browser> data logic into shared modules. https://reviewboard.mozilla.org/r/108060/#review109422 ::: browser/components/extensions/ext-utils.js:336 (Diff revision 2) > }, Ci.nsIThread.DISPATCH_NORMAL); > } > > - getBrowserId(browser) { > + getBrowserData(browser) { > + if (!browser.ownerGlobal.location.href === "about:addons") { > + // When we're loaded into a <browser> inside about:addons, we need to go up I know this is just moving existing code but what's the scenario that this code applies to? inline options pages? ::: browser/components/extensions/ext-utils.js:341 (Diff revision 2) > + // When we're loaded into a <browser> inside about:addons, we need to go up > + // one more level. > + browser = browser.ownerGlobal.QueryInterface(Ci.nsIInterfaceRequestor) > + .getInterface(Ci.nsIDocShell) > + .chromeEventHandler; > + } the original code bailed out here if this returned null, that's no longer necessary?

Comment on attachment 8831510 [details] Bug 1260548: Part 5 - Factor out <browser> data logic into shared modules. https://reviewboard.mozilla.org/r/108060/#review109422 > I know this is just moving existing code but what's the scenario that this code applies to? inline options pages? Yeah, just inline options pages. > the original code bailed out here if this returned null, that's no longer necessary? Right, it's no longer necessary now that we explicitly check that the parent document is about:addons. That will always have a parent <browser> element.

Comment on attachment 8831507 [details] Bug 1260548: Part 2 - Factor out the excuteScript/insertCSS logic. https://reviewboard.mozilla.org/r/108054/#review109430 The handling of about:blank and about:newtab here feels really awkward but you know the details here much better than I do so I trust if there was a practical way to do this more neatly that you would have found it...

Comment on attachment 8831507 [details] Bug 1260548: Part 2 - Factor out the excuteScript/insertCSS logic. https://reviewboard.mozilla.org/r/108054/#review109430 Basically, the problem is bug 1315553. Modifying top-level `about:blank` currently isn't supported because of the way that it's loaded. Fixing that bug should make this workaround unnecessary. But, in the mean time, the workaround prevents these calls from simply stalling until the first time another URL is loaded into the tab.

Comment on attachment 8831511 [details] Bug 1260548: Part 6 - Add basic tabs API support for Android. https://reviewboard.mozilla.org/r/108062/#review109438 Wow, this is impressive. My only criticism here is the inconsistent use of async and directly returning Promises in ext-tabs.js but I'd be fine leaving cleanup of that for a follow-up. ::: mobile/android/components/extensions/ext-tabs.js:341 (Diff revision 2) > for (let tabId of tabs) { > let tab = tabTracker.getTab(tabId); > - tab.ownerGlobal.gBrowser.removeTab(tab); > + tab.browser.ownerGlobal.BrowserApp.closeTab(tab); > } > + > + return Promise.resolve(); why not just make this async? ::: mobile/android/components/extensions/ext-tabs.js:366 (Diff revision 2) > - tabbrowser.selectedTab = tab; > + BrowserApp.selectTab(tab); > } else { > // Not sure what to do here? Which tab should we select? > } > } > - if (updateProperties.muted !== null) { > + // FIXME: highlighted/selected, muted, pinned, openerTabId follow-up bug? (unless these are in one of the subsequent patches that i haven't seen yet) ::: mobile/android/components/extensions/ext-tabs.js:368 (Diff revision 2) > - tabbrowser.unpinTab(tab); > - } > - } > - // FIXME: highlighted/selected, openerTabId > > - return tabManager.convert(tab); > + return Promise.resolve(tabManager.convert(tab)); Why the `Promise.resolve` ? ::: toolkit/components/extensions/ExtensionParent.jsm:213 (Diff revision 2) > // tabs.sendMessage / tabs.connect > if (tabId) { > // `tabId` being set implies that the tabs API is supported, so we don't > // need to check whether `tabTracker` exists. > let tab = apiManager.global.tabTracker.getTab(tabId, null); > - return tab && tab.linkedBrowser.messageManager; > + return tab && (tab.linkedBrowser || tab.browser).messageManager; Ugh. how about something like `const BROWSER_PROPERTY = (desktop) ? "linkedBrowser" : "browser";` at the top level and then just `tab[BROWSER_PROPERTY]` here?

Comment on attachment 8831511 [details] Bug 1260548: Part 6 - Add basic tabs API support for Android. https://reviewboard.mozilla.org/r/108062/#review109438 > why not just make this async? Rebase botch. > follow-up bug? (unless these are in one of the subsequent patches that i haven't seen yet) Well, muted and pinned are not currently supported on Android. `openerTabId` already has a bug for desktop, and I was planning to handle both at once. I still need to file follow-ups for the other missing functionality, including some of these properties. > Why the `Promise.resolve` ? Also merge botch. > Ugh. > how about something like `const BROWSER_PROPERTY = (desktop) ? "linkedBrowser" : "browser";` at the top level and then just `tab[BROWSER_PROPERTY]` here? I was considering cleaner ways to handle this, and was leaning toward either adding some method to tabTracker to handle this, or figuring out some way to use a Tab instance here. I'd rather do it in a follow-up, though, since this is an unusual use case.

Comment on attachment 8831512 [details] Bug 1260548: Part 7 - Add mochitests for the Android tabs API. https://reviewboard.mozilla.org/r/108064/#review109452 This looks good. I assume you've given though to possibly sharing some of this code and didn't do it since it wasn't practical to do now? Is it worth a follow-up bug to do something at some point to avoid having so much copied test code? ::: mobile/android/components/extensions/test/mochitest/test_ext_tabs_executeScript.html:186 (Diff revision 2) > browser.test.assertEq("http://example.com/", result[0], "Script executed correctly in new tab"); > > await browser.tabs.remove(tab.id); > }), > > + // This currently does not work on Android. either remove it or have a follow-up bug?

Comment on attachment 8831513 [details] Bug 1260548: Part 8 - Cut down on unnecessary console warnings during test runs. https://reviewboard.mozilla.org/r/108066/#review109464 Nice, thanks.

Comment on attachment 8831512 [details] Bug 1260548: Part 7 - Add mochitests for the Android tabs API. https://reviewboard.mozilla.org/r/108064/#review109452 Yeah, I was planning to move as much as possible to toolkit, and then delete the corresponding browser tests, once Android tab support is more complete. I'll file a bug for that, once I've filed its dependencies.

Currently missing from the Android tabs API: The following methods and events: - detectLanguage - getZoom - getZoomSettings - move - onMoved - onZoomChange - setZoom - setZoomSettings In tabs.create: - cookieStoreId - pinned - ownerTabId In tabs.update: - muted - pinned

https://hg.mozilla.org/integration/mozilla-inbound/rev/9e70a396920869369a36eebbcf307835ce806ee5 Bug 1260548: Part 1 - Factor out the common functionality of the tabs API. r=aswan https://hg.mozilla.org/integration/mozilla-inbound/rev/70f6eb26604a29da53fc8fd64d098d885aeac366 Bug 1260548: Part 2 - Factor out the excuteScript/insertCSS logic. r=aswan https://hg.mozilla.org/integration/mozilla-inbound/rev/628d13328225f0f004a8ade3d3a3d266df5b5351 Bug 1260548: Part 3 - Factor out the extension popup code into its own module. r=aswan https://hg.mozilla.org/integration/mozilla-inbound/rev/7ac255bc85d767e26424eb7182b0eb7bbe16ca51 Bug 1260548: Part 4 - Factor out tab status listener logic into shared module. r=aswan https://hg.mozilla.org/integration/mozilla-inbound/rev/980266a5316b3bb73844aa17a50a7121ffd9de1d Bug 1260548: Part 5 - Factor out <browser> data logic into shared modules. r=aswan https://hg.mozilla.org/integration/mozilla-inbound/rev/683a0eb722b663c43b90cd83aa23e99e82288efa Bug 1260548: Part 6 - Add basic tabs API support for Android. r=aswan https://hg.mozilla.org/integration/mozilla-inbound/rev/1c8f0e831975e3f24c0b2c518134a8bc4e93e1e4 Bug 1260548: Part 7 - Add mochitests for the Android tabs API. r=aswan https://hg.mozilla.org/integration/mozilla-inbound/rev/8e67a3e9a5bfd7eff6327963a9c66fa13f3d7e80 Bug 1260548: Part 8 - Cut down on unnecessary console warnings during test runs. r=aswan https://hg.mozilla.org/integration/mozilla-inbound/rev/bc59eff957ebf8d522b91f275ec3838e50df3392 Bug 1260548: Part 9 - Make sure Android mochitests do not leave extra tabs open. r=aswan

https://hg.mozilla.org/integration/mozilla-inbound/rev/1ca8ceb7b7fabca08691e54930acf604d3aee960 Bug 1260548: Follow-up: Add missing support file to chrome.ini. r=me

https://hg.mozilla.org/integration/mozilla-inbound/rev/e250509dc1d3267e22e694383e19d881b09adc9c Bug 1260548: Follow-up: Fix inadequate skip-if. r=me

https://hg.mozilla.org/integration/mozilla-inbound/rev/40833d595677539789acc9bc55e45f18b2c614e3 Bug 1260548: Follow-up: Fix skip-if typo that was the real problem. r=me CLOSED TREE

https://hg.mozilla.org/integration/mozilla-inbound/rev/04d1a9455d3cdee5668117c3ffea9345568192c1 Bug 1260548: Follow-up: Add another missing dependency to mochitest.ini. r=me

One thing I have learned about Android mochitests is that no matter how many try runs and local test runs I do, I will never win...

https://hg.mozilla.org/mozilla-central/rev/9e70a3969208 https://hg.mozilla.org/mozilla-central/rev/70f6eb26604a https://hg.mozilla.org/mozilla-central/rev/628d13328225 https://hg.mozilla.org/mozilla-central/rev/7ac255bc85d7 https://hg.mozilla.org/mozilla-central/rev/980266a5316b https://hg.mozilla.org/mozilla-central/rev/683a0eb722b6 https://hg.mozilla.org/mozilla-central/rev/1c8f0e831975 https://hg.mozilla.org/mozilla-central/rev/8e67a3e9a5bf https://hg.mozilla.org/mozilla-central/rev/bc59eff957eb https://hg.mozilla.org/mozilla-central/rev/1ca8ceb7b7fa https://hg.mozilla.org/mozilla-central/rev/e250509dc1d3 https://hg.mozilla.org/mozilla-central/rev/40833d595677 https://hg.mozilla.org/mozilla-central/rev/04d1a9455d3c

We have tested this bug with “every event 1.0” add-on recommended by andym and Krupa. Purpose of test: see that the implemented APIs are fired and works correctly. Results: 1.No issues were observed on the mobile device regarding Firefox browser 2.During the tests, the console recorded multiple issues that involved the implemented APIs (tabs, webRequest , webNavigation) and they were extracted in separate txt files for further investigation by the dev side. 3.Two clear patterns were identified which led to issues observed in the console: - google search fires “browser.webRequest.onResponseStarted” API and returns “NS_BINDING_ABORTED: Component returned failure code: 0x804b0002” - closing a tab fires “browser.tabs.onHighlighted” and returns "TypeError: this.browser.webProgress is undefined” 4.For the other issues we don’t have a clear pattern (yet). Basically they appear when navigating. Please let us know your thoughts and how can we be of help anymore. We will attach the txt files with errors.

Sorry for the delay. I started responding to this and forgot to finish. (In reply to Victor Carciu from comment #45) > - google search fires “browser.webRequest.onResponseStarted” API and returns > “NS_BINDING_ABORTED: Component returned failure code: 0x804b0002” This is normal. It's a side-effect of the way error reporting works in certain corner cases, but it is expected. > - closing a tab fires “browser.tabs.onHighlighted” and returns "TypeError: > this.browser.webProgress is undefined” It looks like this is happening because we're firing an event after the tab's <browser> element has been removed. Can you please file a bug with steps to reproduce? > 4.For the other issues we don’t have a clear pattern (yet). Basically they > appear when navigating. > TypeError: this._recipeManager is null[Learn More] This is an issue with the login manager. I've seen it before, and it probably merits a bug, but it's unrelated to these changes. > RangeError: too many arguments provided for a function call SessionStore.js:1154:17 This may be related, but it's hard to say without a complete stack, and knowing what's on that line in the build that this came from. > NS_ERROR_UNEXPECTED: Component returned failure code: 0x8000ffff > (NS_ERROR_UNEXPECTED) [nsIPrefBranch.getBoolPref] browser.js:4106 This is probably unrelated, but probably deserves its own Android bug with steps to reproduce. > TypeError: can't access dead object tab.js:1656:1 This looks like it may be a devtools bug, but is probably unrelated.

For "> TypeError: can't access dead object" I have opened the source and from what I see, it's indeed related to devtools (attached txt file "viewsource for...")

I've updated the compat data according to comment 37, I think: https://developer.mozilla.org/en-US/Add-ons/WebExtensions/API/tabs#Browser_compatibility. But please let me know if I need anything else here.