1202532 - The callback argument of windows.update() is optional

Status: RESOLVED FIXED

(WebExtensions :: Untriaged, defect)

Description

[Edgar Chen [:edgar] (OOO ~ 07/13, 07/16 ~ 07/20)]

I got below error with Adblock Plus,

> Extension error: TypeError: f is not a function resource://gre/modules/ExtensionUtils.jsm 21
> runSafeWithoutClone@resource://gre/modules/ExtensionUtils.jsm:21:14
> runSafe@resource://gre/modules/ExtensionUtils.jsm:37:33
> fire@resource://gre/modules/ExtensionUtils.jsm:129:39
> onStateChange@chrome://browser/content/ext-tabs.js:172:13
> callListeners@chrome://browser/content/tabbrowser.xml:536:24
> _callProgressListeners@chrome://browser/content/tabbrowser.xml:557:13
> mTabProgressListener/<._callProgressListeners@chrome://browser/content/tabbrowser.xml:606:22
> mTabProgressListener/<.onStateChange@chrome://browser/content/tabbrowser.xml:772:1

Comment 1

[Edgar Chen [:edgar] (OOO ~ 07/13, 07/16 ~ 07/20)]

Attachment: WIP, Patch, v1

Comment 2

[Edgar Chen [:edgar] (OOO ~ 07/13, 07/16 ~ 07/20)]

Found another error in windows.getAll() when trying to write a test case. Will provide a fix for it here as well.

Comment 3

[Edgar Chen [:edgar] (OOO ~ 07/13, 07/16 ~ 07/20)]

Attachment: Patch, v2

Comment 4

[Edgar Chen [:edgar] (OOO ~ 07/13, 07/16 ~ 07/20)]

Attachment: MozReview Request: Bug 1202532 - The callback argument of windows.update() is optional

Bug 1202532 - The callback argument of windows.update() is optional

Comment 5

[Edgar Chen [:edgar] (OOO ~ 07/13, 07/16 ~ 07/20)]

Comment on attachment 8660538 [details]
MozReview Request: Bug 1202532 - The callback argument of windows.update() is optional

Bug 1202532 - The callback argument of windows.update() is optional

Comment 6

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

Comment on attachment 8660538 [details]
MozReview Request: Bug 1202532 - The callback argument of windows.update() is optional

https://reviewboard.mozilla.org/r/19129/#review16979

Thanks for fixing it. It looks nice aside from an issue in the test.

::: browser/components/extensions/test/browser/browser_ext_windows_update.js:4
(Diff revision 1)
> +      waitForFocus(aResolve);

I don't think this does what you want. It waits until |window| (which is always |window1| here) is in focus. I think you want to check if Services.focus.activeWindow == <desired-window>. If it's not, you can add a "focus" listener on <desired-window> and wait for that.

::: browser/components/extensions/test/browser/browser_ext_windows_update.js:8
(Diff revision 1)
> +  let window1 = Services.wm.getMostRecentWindow("navigator:browser");

This is just the pre-defined global |window|, so you could just say |let window1 = window;|.

::: browser/components/extensions/test/browser/browser_ext_windows_update.js:12
(Diff revision 1)
> +  yield promiseWaitForFocus();

I think you might be getting lucky here. Most of the time, the focus will still be on |window1| at this point. You should probably pass |window2| in as an argument to promiseWaitForFocus so that it knows what window to wait to be in focus.

Comment 7

[Edgar Chen [:edgar] (OOO ~ 07/13, 07/16 ~ 07/20)]

(In reply to Bill McCloskey (:billm) from comment #6)
> Comment on attachment 8660538 [details]
> MozReview Request: Bug 1202532 - The callback argument of windows.update()
> is optional
> 
> https://reviewboard.mozilla.org/r/19129/#review16979
> 
> Thanks for fixing it. It looks nice aside from an issue in the test.
> 
> :::
> browser/components/extensions/test/browser/browser_ext_windows_update.js:4
> (Diff revision 1)
> > +      waitForFocus(aResolve);
> 
> I don't think this does what you want. It waits until |window| (which is
> always |window1| here) is in focus. I think you want to check if
> Services.focus.activeWindow == <desired-window>. If it's not, you can add a
> "focus" listener on <desired-window> and wait for that.
> 
> :::
> browser/components/extensions/test/browser/browser_ext_windows_update.js:8
> (Diff revision 1)
> > +  let window1 = Services.wm.getMostRecentWindow("navigator:browser");
> 
> This is just the pre-defined global |window|, so you could just say |let
> window1 = window;|.
> 
> :::
> browser/components/extensions/test/browser/browser_ext_windows_update.js:12
> (Diff revision 1)
> > +  yield promiseWaitForFocus();
> 
> I think you might be getting lucky here. Most of the time, the focus will
> still be on |window1| at this point. You should probably pass |window2| in
> as an argument to promiseWaitForFocus so that it knows what window to wait
> to be in focus.

Thanks for the review, I will address your comment on next version.

Comment 8

[Edgar Chen [:edgar] (OOO ~ 07/13, 07/16 ~ 07/20)]

Comment on attachment 8660538 [details]
MozReview Request: Bug 1202532 - The callback argument of windows.update() is optional

Bug 1202532 - The callback argument of windows.update() is optional

Comment 9

[Edgar Chen [:edgar] (OOO ~ 07/13, 07/16 ~ 07/20)]

Try result looks good: https://treeherder.mozilla.org/#/jobs?repo=try&revision=5aa65f53c414&group_state=expanded

Comment 10

[Pulsebot]

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

Comment 11

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/5ed60fcba7c9