1205221 - chrome.tabs.onUpdate event doesn't be sent when updating tab's attribute

Status: RESOLVED FIXED

(WebExtensions :: Untriaged, defect)

Description

[Edgar Chen [:edgar]]

Found this when writing a test for tabs.onUpdate event.

Comment 1

[Edgar Chen [:edgar]]

Attachment: WIP, Patch, v1

Comment 2

[Edgar Chen [:edgar]]

Attachment: WIP, Patch, v2

Comment 3

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Edgar, how is this going? Do you want to ask for review?

Comment 4

[Edgar Chen [:edgar]]

(In reply to Bill McCloskey (:billm) from comment #3)
> Edgar, how is this going? Do you want to ask for review?

Hi Bill, I was working on other issues these days.
Patch is almost ready, but I would like to write some more tests.
I will provide a formal patch for review early next week.
Thank you.

Comment 5

[Edgar Chen [:edgar]]

Attachment: MozReview Request: Bug 1205221 - Fix chrome.tabs.onUpdate event doesn't be fired correctly when tab's attribute is changed. r=billm

Bug 1205221 - Fix chrome.tabs.onUpdate event doesn't be fired correctly when tab's attribute is changed.

Comment 6

[Edgar Chen [:edgar]]

Comment on attachment 8675528 [details]
MozReview Request: Bug 1205221 - Fix chrome.tabs.onUpdate event doesn't be fired correctly when tab's attribute is changed. r=billm

Bug 1205221 - Fix chrome.tabs.onUpdate event doesn't be fired correctly when tab's attribute is changed.

Comment 7

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Comment on attachment 8675528 [details]
MozReview Request: Bug 1205221 - Fix chrome.tabs.onUpdate event doesn't be fired correctly when tab's attribute is changed. r=billm

https://reviewboard.mozilla.org/r/22385/#review20215

Thanks. Could you please rebase over bug 1197422? That added a WindowListManager.browserWindows generator that I think would help you.

::: browser/components/extensions/ext-tabs.js:122
(Diff revision 1)
> -        WindowListManager.addOpenListener(windowListener, false);
> +        WindowListManager.addOpenListener(windowListener);

I don't really understand how or why you're changing the fireOnExisting parameter. Could you please explain that more?

Comment 8

[Edgar Chen [:edgar]]

https://reviewboard.mozilla.org/r/22385/#review20263

::: browser/components/extensions/ext-tabs.js:122
(Diff revision 1)
> -        WindowListManager.addOpenListener(windowListener, false);
> +        WindowListManager.addOpenListener(windowListener);

I change the behavior of  `WindowListManager.addOpenListener()` a bit. `WindowListManager.addOpenListener()` now won't call listener immediately with existing window any more (I also remove the `fireOnExisting` parameter).

After review the caller of `WindowListManager.addOpenListener()` again, I just realize this change affects the `window.onCreate` behavior [1]. With this change, `window.onCreate` won't be triggered by existing window. But it seems like it's the behavior we expect for `window.onCreate`, I guess.

[1] https://dxr.mozilla.org/mozilla-central/source/browser/components/extensions/ext-windows.js#18

Comment 9

[Edgar Chen [:edgar]]

ni for comment #8.
I will rebase over bug 1197422 and ask review again.
Thank you.

Comment 10

[Edgar Chen [:edgar]]

Comment on attachment 8675528 [details]
MozReview Request: Bug 1205221 - Fix chrome.tabs.onUpdate event doesn't be fired correctly when tab's attribute is changed. r=billm

Bug 1205221 - Fix chrome.tabs.onUpdate event doesn't be fired correctly when tab's attribute is changed.

Comment 11

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Comment on attachment 8675528 [details]
MozReview Request: Bug 1205221 - Fix chrome.tabs.onUpdate event doesn't be fired correctly when tab's attribute is changed. r=billm

https://reviewboard.mozilla.org/r/22385/#review21087

Thanks!

::: browser/components/extensions/ext-utils.js:494
(Diff revision 2)
> +  _openListeners: new Map(),

I don't think you want this. \_openListeners is a property of WindowListManager.

::: browser/components/extensions/ext-utils.js:516
(Diff revision 2)
> -    if (needOpenListener) {
> +    // Register listener to all existing window.

"Register listener on all existing windows."

::: browser/components/extensions/ext-utils.js:518
(Diff revision 2)
> -      this.addWindowListener(window, type, listener);
> +      if (type == "progress") {
> +        window.gBrowser.addTabsProgressListener(listener);
> +      } else {
> +        window.addEventListener(type, listener);
> +      }

I don't think this needs to be changed.

::: browser/components/extensions/ext-utils.js:542
(Diff revision 2)
> +    // Unregister listener to all existing window.

"Unregister listener from all existing windows."

::: browser/components/extensions/test/browser/browser_ext_tabs_onUpdated.js:11
(Diff revision 2)
> +  yield extension.awaitMessage("finish");

Please use awaitFinish here.

::: browser/components/extensions/test/browser/browser_ext_tabs_onUpdated.js:28
(Diff revision 2)
> +          browser.test.sendMessage("finish");

Please use notifyPass here.

::: browser/components/extensions/test/browser/browser_ext_tabs_onUpdated.js:50
(Diff revision 2)
> +          browser.test.sendMessage("finish");

notifyPass

::: browser/components/extensions/test/browser/browser_ext_tabs_onUpdated.js:73
(Diff revision 2)
> +          browser.test.sendMessage("finish");

Please use notifyPass here.

::: browser/components/extensions/test/browser/browser_ext_tabs_onUpdated.js:116
(Diff revision 2)
> +            browser.test.sendMessage("finish");

notifyPass

Comment 12

[Edgar Chen [:edgar]]

https://reviewboard.mozilla.org/r/22385/#review21087

> I don't think you want this. \_openListeners is a property of WindowListManager.

Thanks for catching this.

Comment 13

[Edgar Chen [:edgar]]

Comment on attachment 8675528 [details]
MozReview Request: Bug 1205221 - Fix chrome.tabs.onUpdate event doesn't be fired correctly when tab's attribute is changed. r=billm

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

Comment 14

[Edgar Chen [:edgar]]

browser_ext_tabs_onUpdated.js got an error during try runs [1],

62 INFO TEST-UNEXPECTED-FAIL | browser/components/extensions/test/browser/browser_ext_tabs_onUpdated.js | uncaught exception - NS_ERROR_NOT_AVAILABLE: Component returned failure code: 0x80040111 (NS_ERROR_NOT_AVAILABLE) [nsISpeculativeConnect.speculativeConnect] at chrome://browser/content/newtab/newTab.js:1278

Will update testcase to fix it.

[1] https://treeherder.mozilla.org/#/jobs?repo=try&revision=d83ede38fdb0&group_state=expanded

Comment 15

[Edgar Chen [:edgar]]

Comment on attachment 8675528 [details]
MozReview Request: Bug 1205221 - Fix chrome.tabs.onUpdate event doesn't be fired correctly when tab's attribute is changed. r=billm

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

Comment 16

[Edgar Chen [:edgar]]

Try result: https://treeherder.mozilla.org/#/jobs?repo=try&revision=83b612d26ea6&group_state=expanded

Comment 17

[Pulsebot]

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

Comment 18

[Carsten Book [:Tomcat]]

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