Closed Bug 1260548 Opened 8 years ago Closed 8 years ago

Basic tabs API support for Android WebExtensions

Categories

(WebExtensions :: Android, defect, P3)

Product:
WebExtensions
Component:
Android
Platform:
Unspecified
Android
Type:
defect

Tracking

(Not tracked)

Status:
RESOLVED FIXED
Milestone:
mozilla54
Blocking Flags:
webextensions +

People

(Reporter: kmag, Assigned: kmag)

References

(Depends on 1 open bug)

Details

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

Attachments

(10 files)

Bug 1260548: Part 1 - Factor out the common functionality of the tabs API.
59 bytes, text/x-review-board-request
aswan
: review+
Details
Bug 1260548: Part 2 - Factor out the excuteScript/insertCSS logic.
59 bytes, text/x-review-board-request
aswan
: review+
Details
Bug 1260548: Part 3 - Factor out the extension popup code into its own module.
59 bytes, text/x-review-board-request
aswan
: review+
Details
Bug 1260548: Part 4 - Factor out tab status listener logic into shared module.
59 bytes, text/x-review-board-request
aswan
: review+
Details
Bug 1260548: Part 5 - Factor out <browser> data logic into shared modules.
59 bytes, text/x-review-board-request
aswan
: review+
Details
Bug 1260548: Part 6 - Add basic tabs API support for Android.
59 bytes, text/x-review-board-request
aswan
: review+
Details
Bug 1260548: Part 7 - Add mochitests for the Android tabs API.
59 bytes, text/x-review-board-request
aswan
: review+
Details
Bug 1260548: Part 8 - Cut down on unnecessary console warnings during test runs.
59 bytes, text/x-review-board-request
aswan
: review+
Details
Errors from console during testing
5.45 KB, application/x-zip-compressed
Details
Viewsource for TypeError can't access dead object.txt
2.80 KB, text/plain
Details 
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.
Blocks: 1260550
OS: Unspecified → Android
Whiteboard: [tabs]
Blocks: 1260250
Whiteboard: [tabs] → [tabs]triaged
Blocks: 1294390
Blocks: 1263005
Blocks: abp
Blocks: 1300811
No longer blocks: 1263005
Component: WebExtensions: Untriaged → WebExtensions: Android
Priority: -- → P3
Blocks: webext-port-abp
No longer blocks: abp
webextensions: --- → +
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.
Attachment #8831506 - Flags: review?(aswan) → review+
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
Attachment #8831508 - Flags: review?(aswan) → review+
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
Attachment #8831509 - Flags: review?(aswan) → review+
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?
Attachment #8831510 - Flags: review?(aswan) → review+
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...
Attachment #8831507 - Flags: review?(aswan) → review+
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?
Attachment #8831511 - Flags: review?(aswan) → review+
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?
Attachment #8831512 - Flags: review?(aswan) → review+
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.
Attachment #8831513 - Flags: review?(aswan) → review+
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
Keywords: dev-doc-needed
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/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...
Blocks: 1336308
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.
Flags: needinfo?(kmaglione+bmo)
Depends on: 1358415
See Also: → 1372178
Depends on: 1389381
Flags: needinfo?(kmaglione+bmo)
Product: Toolkit → WebExtensions
status-firefox54: fixed → ---
See Also: → 1397667
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: