Closed
Bug 1202532
Opened 9 years ago
Closed 9 years ago
The callback argument of windows.update() is optional
Categories
(WebExtensions :: Untriaged, defect)
WebExtensions
Untriaged
Tracking
(firefox43 fixed)
RESOLVED
FIXED
mozilla43
Tracking | Status | |
---|---|---|
firefox43 | --- | fixed |
People
(Reporter: edgar, Assigned: edgar)
Details
Attachments
(1 file, 2 obsolete files)
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
Assignee | ||
Comment 1•9 years ago
|
||
Assignee | ||
Updated•9 years ago
|
Assignee: nobody → echen
Assignee | ||
Comment 2•9 years ago
|
||
Found another error in windows.getAll() when trying to write a test case. Will provide a fix for it here as well.
Assignee | ||
Comment 3•9 years ago
|
||
Attachment #8657991 -
Attachment is obsolete: true
Assignee | ||
Comment 4•9 years ago
|
||
Bug 1202532 - The callback argument of windows.update() is optional
Assignee | ||
Updated•9 years ago
|
Attachment #8658144 -
Attachment is obsolete: true
Assignee | ||
Comment 5•9 years ago
|
||
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
Attachment #8660538 -
Flags: review?(wmccloskey)
Attachment #8660538 -
Flags: review?(wmccloskey) → review+
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.
Assignee | ||
Comment 7•9 years ago
|
||
(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.
Assignee | ||
Comment 8•9 years ago
|
||
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
Assignee | ||
Comment 9•9 years ago
|
||
Try result looks good: https://treeherder.mozilla.org/#/jobs?repo=try&revision=5aa65f53c414&group_state=expanded
Assignee | ||
Updated•9 years ago
|
Keywords: checkin-needed
Comment 10•9 years ago
|
||
https://hg.mozilla.org/integration/fx-team/rev/5ed60fcba7c9
Keywords: checkin-needed
Comment 11•9 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/5ed60fcba7c9
Status: NEW → RESOLVED
Closed: 9 years ago
status-firefox43:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla43
Updated•6 years ago
|
Product: Toolkit → WebExtensions
You need to log in
before you can comment on or make changes to this bug.
Description
•