Closed
Bug 704751
Opened 13 years ago
Closed 12 years ago
toolkit/mozapps/extensions/content/extensions.js should not use sync XHR
Categories
(Toolkit :: Add-ons Manager, defect)
Toolkit
Add-ons Manager
Tracking
()
RESOLVED
FIXED
mozilla17
People
(Reporter: emk, Assigned: darktrojan)
References
Details
Attachments
(1 file, 3 obsolete files)
(deleted),
patch
|
Unfocused
:
review+
|
Details | Diff | Splinter Review |
We are going to remove .responseType support from sync XHR (see bug 701787).
Assignee | ||
Comment 1•12 years ago
|
||
It also shouldn't kill the detail view when the optionsURL points to something useless.
Assignee: nobody → geoff
Status: NEW → ASSIGNED
OS: Windows 7 → All
Hardware: x86_64 → All
Assignee | ||
Comment 3•12 years ago
|
||
Comment on attachment 651684 [details] [diff] [review] fix Oh hey, this breaks a test.
Attachment #651684 -
Flags: review?(bmcbride)
Assignee | ||
Comment 4•12 years ago
|
||
Attachment #651684 -
Attachment is obsolete: true
Attachment #651695 -
Flags: review?(bmcbride)
Comment 5•12 years ago
|
||
Comment on attachment 651695 [details] [diff] [review] fix and fixed test Review of attachment 651695 [details] [diff] [review]: ----------------------------------------------------------------- I think that test was more correct before the change - like other views, the details view shouldn't be calling notifyViewChanged() until its fully loaded, which includes the settings. ::: toolkit/mozapps/extensions/content/extensions.js @@ +2889,5 @@ > + } > + }).bind(this); > + xhr.send(); > + } catch(e) { > + Cu.reportError(e); Doesn't this still need an onerror handler?
Attachment #651695 -
Flags: review?(bmcbride) → review-
Assignee | ||
Comment 6•12 years ago
|
||
Attachment #651695 -
Attachment is obsolete: true
Attachment #652274 -
Flags: review?(bmcbride)
Assignee | ||
Comment 7•12 years ago
|
||
(In reply to Blair McBride (:Unfocused) from comment #5) > Comment on attachment 651695 [details] [diff] [review] > ::: toolkit/mozapps/extensions/content/extensions.js > @@ +2889,5 @@ > > + } > > + }).bind(this); > > + xhr.send(); > > + } catch(e) { > > + Cu.reportError(e); > > Doesn't this still need an onerror handler? I can't think of a reason why onerror would get called unless the addon is doing something really odd with optionsURL, but I put it in anyway. For file based protocols xhr.send throws a file not found error rather than calling onerror.
Comment 8•12 years ago
|
||
Hmm, which brings up something else - we shouldn't really be allowing remote URLs for inline or dialog settings types. I see we already have bug 609101 for that - there was some question on how to handle it, so I've commented there.
Comment 9•12 years ago
|
||
Comment on attachment 652274 [details] [diff] [review] fix Review of attachment 652274 [details] [diff] [review]: ----------------------------------------------------------------- ::: toolkit/mozapps/extensions/content/extensions.js @@ +2661,5 @@ > gridRows[i].removeAttribute("first-row"); > } > } > > + this.fillSettingsRows(aScrollToPreferences, (function() { Bug 767236 is working on naming all functions - from now on, all functions should be named (makes debugging stuff so much easier). For callbacks, we settled on using the convention outerFunction_functionCallbackIsFor. So this would be updateView_fillSettingsRows. Same for the other anonymous functions added elsewhere. @@ +2663,5 @@ > } > > + this.fillSettingsRows(aScrollToPreferences, (function() { > + this.updateState(); > + gViewController.updateCommands(); Apparently calling updateCommands isn't even needed here - updateState() already calls it. May as well remove it now. @@ +2846,5 @@ > > + try { > + var xhr = new XMLHttpRequest(); > + xhr.open("GET", this._addon.optionsURL, true); > + xhr.responseType = 'xml'; Nit: Double quotes. @@ +2872,5 @@ > + } > + > + // Ensure the page has loaded and force the XBL bindings to be synchronously applied, > + // then notify observers. > + if (gViewController.viewPort.selectedPanel.hasAttribute("loading")) { If this is true, the callback never gets called. @@ +2897,5 @@ > + aCallback(); > + }; > + xhr.send(); > + } catch(e) { > + Cu.reportError(e); Logging an error here, but not in onerror above. ::: toolkit/mozapps/extensions/test/browser/browser_inlinesettings.js @@ +515,5 @@ > // enable > var button = gManagerWindow.document.getElementById("detail-enable-btn"); > EventUtils.synthesizeMouseAtCenter(button, { clickCount: 1 }, gManagerWindow); > > + observer.callback = function() { Why does this test need changed?
Attachment #652274 -
Flags: review?(bmcbride) → review-
Assignee | ||
Comment 10•12 years ago
|
||
> ::: toolkit/mozapps/extensions/test/browser/browser_inlinesettings.js
> @@ +515,5 @@
> > // enable
> > var button = gManagerWindow.document.getElementById("detail-enable-btn");
> > EventUtils.synthesizeMouseAtCenter(button, { clickCount: 1 }, gManagerWindow);
> >
> > + observer.callback = function() {
>
> Why does this test need changed?
Because loading the options is now async. That is the point of this bug.
Attachment #652274 -
Attachment is obsolete: true
Attachment #652373 -
Flags: review?(bmcbride)
Comment 11•12 years ago
|
||
Comment on attachment 652373 [details] [diff] [review] v4 Review of attachment 652373 [details] [diff] [review]: ----------------------------------------------------------------- ::: toolkit/mozapps/extensions/content/extensions.js @@ +2890,5 @@ > + } > + if (aCallback) > + aCallback(); > + }).bind(this); > + xhr.onerror = function fillSettingsRows_onerror(event) { Nit: aEvent
Attachment #652373 -
Flags: review?(bmcbride) → review+
Assignee | ||
Comment 12•12 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/68ecea83a7da (Try: https://tbpl.mozilla.org/?tree=Try&rev=e611d711a765)
Flags: in-testsuite+
Flags: in-litmus-
Comment 13•12 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/68ecea83a7da
Status: ASSIGNED → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla17
You need to log in
before you can comment on or make changes to this bug.
Description
•