1273146 - WebExtensions: chrome.windows.create - callback parameter (window) does not have window.tabs property

Status: RESOLVED FIXED

(WebExtensions :: Untriaged, defect, P2)

Description

[dw-dev]

User Agent: Mozilla/5.0 (Windows NT 10.0; WOW64; rv:46.0) Gecko/20100101 Firefox/46.0
Build ID: 20160502172042

Steps to reproduce:

I tried to execute the following code:

chrome.windows.create({ type: "normal", tabId: tabId },
function(newwin)
{
    var win_id, tab_id;
    
    win_id = newwin.id;

    tab_id = newwin.tabs[0].id;
});



Actual results:

The code fails with the following error message: "newwin.tabs is undefined".



Expected results:

tab_id should be assigned the id of the first tab in the new window.

Comment 1

[:shell escalante]

need to check Chrome code to see what should happen / when.  normally there with query with populate.

Comment 2

[Bob Silverberg [:bsilverberg]]

I have confirmed that Chrome does return an array of tabs for the window when calling `windows.create()`. The docs [1] are non-specific about this.

We should fix this, and this bug can be used to track that. We should also update our docs for the API [2] to indicate that tabs can be expected for a window that was just created.


[1] https://developer.chrome.com/extensions/windows#method-create
[2] https://developer.mozilla.org/en-US/Add-ons/WebExtensions/API/windows/create

Comment 3

[Andy McKay]

Attachment: second-patch.txt

Comment 4

[Bob Silverberg [:bsilverberg]]

Comment on attachment 8754497 [details] [diff] [review]
second-patch.txt

Review of attachment 8754497 [details] [diff] [review]:
-----------------------------------------------------------------

This looks good Andy, nice work! I'm wondering if we should also add some checks into the tests in `browser_ext_windows_create.js` which create windows that do not inherit tabs? Looking at the code change there is no reason to expect that call to `windows.create()` to behave any differently from the one being tested here, but we might want that extra coverage just in case.

Comment 5

[Andy McKay]

That seems like a good idea, I'll look into it.

Comment 6

[Andy McKay]

Turns out, if you send multiple tabs to create, it loops through each tab creating a tab in order. But you only get one tab in the window.tabs response. I think this is because it creates the tab in order and sends a document-shown event when that first tab has loaded. As soon as the first document-shown is completed it sends the callback, and then the next tab loads. I'm assuming we should wait for all the documents to be shown for each tab before sending the callback.

Comment 7

[Andy McKay]

Minor correction, its just listening for load on the window and then returning once that has happened.

Comment 8

[Andy McKay]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=f2cd438ede24

Comment 9

[Andy McKay]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=576b0bf5d167

Comment 10

[Brian Kieffer]

Other methods have the same issue. For example, chrome.windows.get gives a windows.Window object without a tabs property.

Comment 11

[Brian Kieffer]

(In reply to Brian Kieffer from comment #10)
> Other methods have the same issue. For example, chrome.windows.get gives a
> windows.Window object without a tabs property.

Oops. I am stupid. I need to specify populate.

Comment 12

[Andy McKay]

(In reply to Brian Kieffer from comment #11)
If you think there's a problem there, could you file a separate bug please?

Comment 13

[Andy McKay]

Attachment: second-patch.txt

Unassigning and throwing back into the pool, I'm unlikely to get any further on this for a while. 

This is the last patch I had, it uses the browser-delayed-startup-finished event to catch the scenario when you create multiple tabs in a window. Got stuck on the Linux test failures on the try server which I haven't had time to look at.

Comment 14

[Tomislav Jovanovic :zombie]

Attachment: bug 1273146 - populate tabs in windows.create() return value

Review commit: https://reviewboard.mozilla.org/r/84488/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/84488/

Comment 15

[Tomislav Jovanovic :zombie]

Comment on attachment 8799189 [details]
bug 1273146 - populate tabs in windows.create() return value

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/84488/diff/1-2/

Comment 16

[Tomislav Jovanovic :zombie]

Comment on attachment 8799189 [details]
bug 1273146 - populate tabs in windows.create() return value

https://reviewboard.mozilla.org/r/84488/#review83030

::: browser/components/extensions/ext-windows.js
(Diff revision 2)
> -          window.addEventListener("load", function listener() {
> -            window.removeEventListener("load", listener);
> +          .then(() => {
> +            if (createData.state !== null) {
> -
> -            if (createData.state == "maximized" || createData.state == "normal" ||
> -                (createData.state == "fullscreen" && AppConstants.platform != "macosx")) {
> -              window.document.documentElement.setAttribute("sizemode", createData.state);

Most states don't work here, and setting `fullscreen` before `delayed-startup-finished` even breaks browser's `delayedStartup` (it throws, never sends the notification), so I simplified and used `setState()` for everything.  Since we now always need to wait for `delayed-startup-finished`, I don't think there is any benefit in trying to set some states earlier.

Comment 17

[Tomislav Jovanovic :zombie]

Comment on attachment 8799189 [details]
bug 1273146 - populate tabs in windows.create() return value

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/84488/diff/2-3/

Comment 18

[Tomislav Jovanovic :zombie]

Comment on attachment 8799189 [details]
bug 1273146 - populate tabs in windows.create() return value

https://reviewboard.mozilla.org/r/84488/#review83030

> Most states don't work here, and setting `fullscreen` before `delayed-startup-finished` even breaks browser's `delayedStartup` (it throws, never sends the notification), so I simplified and used `setState()` for everything.  Since we now always need to wait for `delayed-startup-finished`, I don't think there is any benefit in trying to set some states earlier.

reverted per irc conversation with Kris.

Comment 19

[Kris Maglione [:kmag]]

Comment on attachment 8799189 [details]
bug 1273146 - populate tabs in windows.create() return value

https://reviewboard.mozilla.org/r/84488/#review84272

::: browser/components/extensions/test/browser/browser_ext_windows_create_tabId.js:118
(Diff revision 3)
> +      browser.test.assertEq(2, window.tabs.length, "2 tabs were opened in new window");
> +      browser.test.assertEq("about:blank", window.tabs[0].url, "about:blank, page not loaded yet");
> +      browser.test.assertEq("about:blank", window.tabs[1].url, "about:blank, page not loaded yet");
> +
>        return Promise.all([
>          promiseTabUpdated("http://example.com/"),
>          promiseTabUpdated("http://example.org/"),
>          Promise.resolve(window),
>        ]);

Hm. This all seems a bit racy, but I guess it's not really a new race, at least...

Comment 20

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/d8aafc5269fb
populate tabs in windows.create() return value r=kmag

Comment 21

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/d8aafc5269fb

Comment 22

[Will Bamberg [:wbamberg]]

I've updated https://developer.mozilla.org/en-US/Add-ons/WebExtensions/API/windows/create
to clarify that the returned Window always contains tabs, and to note that Firefox only supports this from version 52. Please let me know if this covers it.

Comment 23

[Tomislav Jovanovic :zombie]

Looks good, thanks.