1234308 - tabs.onCreated callback should receive a bare tab object as its argument

Status: RESOLVED FIXED

(WebExtensions :: Untriaged, defect, P4)

Description

[zzt]

Attachment: firefox-dev-edition-45-0a2-linux_x64.png

User Agent: Mozilla/5.0 (X11; Fedora; Linux x86_64; rv:43.0) Gecko/20100101 Firefox/43.0
Build ID: 20151210112434

Steps to reproduce:

chrome.tabs.onCreated.addListener(function onCreated(tab) {
  console.log(tab); // { tab: { tab: ... } }
});


Actual results:

The tab paremeter is an nested object


Expected results:

The tab paremeter should be a plain object

Firefox dev-edition 45.0a2 (2015-12-21)
Linux x64

Comment 1

[Matthew Wein [:mattw]]

Attachment: Fixes the tabs.onCreated callback to receive a bare tabs object

Comment 2

[Kris Maglione [:kmag]]

Comment on attachment 8712803 [details] [diff] [review]
Fixes the tabs.onCreated callback to receive a bare tabs object

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

This is very close. We just need to be a little stricter.

::: browser/components/extensions/test/browser/browser_ext_tabs_create.js
@@ +123,5 @@
>                browser.tabs.onUpdated.addListener(onUpdated);
>              });
>  
> +            browser.tabs.onCreated.addListener(tab => {
> +              browser.test.assertTrue("id" in tab, `Expected tabs.onCreated callback to receive tab object`);

We need to test that this is actually called. The easiest way would be to add a |createdPromise| to match the |updatedPromise| above.

Comment 3

[Kris Maglione [:kmag]]

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

Comment 4

[Matthew Wein [:mattw]]

Attachment: Fixes the tabs.onCreated callback to receive a bare tabs object (patch v2)

Comment 5

[Kris Maglione [:kmag]]

Comment on attachment 8712886 [details] [diff] [review]
Fixes the tabs.onCreated callback to receive a bare tabs object (patch v2)

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

::: browser/components/extensions/test/browser/browser_ext_tabs_create.js
@@ +132,3 @@
>              });
>  
> +            promiseTabs.create(test.create).then(createdPromise).then(tab => {

The argument to .then() should be a function, not a promise, so this should look something like the other handlers:

  promiseTabs.create(test.create).then(() => {
    return createdPromise;
  }).then(tab => {

It would be better to use the tab returned by .create(), though, so something like this would work:

  Promise.all([
    promiseTabs.create(test.create),
    createdPromise,
  }).then(([tab]) => {

The Promise.all returns a new promise which resolves when all of the promises in the array you pass it resolves. The ([tab]) in the callback destructures the array of results that that promise returns, and gives you the tab returned by promiseTabs.create.

Comment 6

[Matthew Wein [:mattw]]

I also have createdPromise resolve with a tab, so I think the array passed into the .then() call will be [tab, tab].  If that's true then the destructuring would seem a bit confusing.  Should I go ahead and make createdPromise resolve with nothing to ensure that .then() gets the tab returned by promiseTabs.create?

Comment 7

[Kris Maglione [:kmag]]

You should probably change it to resolve with nothing, yes, but it doesn't really matter. You'll still get a two-element array as the result, and the first element will still be the tab from .create().

I wouldn't worry too much about it being confusing. Our code does this all over the place.

Comment 8

[Matthew Wein [:mattw]]

Attachment: Fixes the tabs.onCreated callback to receive a bare tabs object (patch v3)

Comment 9

[Matthew Wein [:mattw]]

Sounds good.

Comment 10

[Matthew Wein [:mattw]]

Attachment: Fix tabs.onCreated callback to receive bare tabs object ()

Comment 11

[Kris Maglione [:kmag]]

Comment on attachment 8712948 [details] [diff] [review]
Fix tabs.onCreated callback to receive bare tabs object ()

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

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

::: browser/components/extensions/test/browser/browser_ext_tabs_create.js
@@ +126,5 @@
> +            let createdPromise = new Promise(resolve => {
> +              let onCreated = tab => {
> +                browser.test.assertTrue("id" in tab, `Expected tabs.onCreated callback to receive tab object`);
> +                resolve();
> +              }

You're missing a semicolon.

Comment 12

[Matthew Wein [:mattw]]

Attachment: Fix tabs.onCreated callback to receive bare tabs object ()

Comment 13

[Kris Maglione [:kmag]]

Comment on attachment 8712956 [details] [diff] [review]
Fix tabs.onCreated callback to receive bare tabs object ()

In the future, you can just set r+ when you update a patch that already has r+

Comment 14

[Matthew Wein [:mattw]]

Okay cool, will do.

Comment 15

[Matthew Wein [:mattw]]

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

Comment 16

[Pulsebot]

https://hg.mozilla.org/integration/fx-team/rev/1b4278d9c84c

Comment 17

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/1b4278d9c84c