toolkit/mozapps/extensions/content/extensions.js should not use sync XHR

RESOLVED FIXED in mozilla17

Status

()

Toolkit
Add-ons Manager
RESOLVED FIXED
6 years ago
6 years ago

People

(Reporter: emk, Assigned: darktrojan)

Tracking

unspecified
mozilla17
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +
in-litmus -

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 3 obsolete attachments)

(Reporter)

Description

6 years ago
We are going to remove .responseType support from sync XHR (see bug 701787).
(Assignee)

Comment 1

6 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 2

6 years ago
Created attachment 651684 [details] [diff] [review]
fix
Attachment #651684 - Flags: review?(bmcbride)
(Assignee)

Comment 3

6 years ago
Comment on attachment 651684 [details] [diff] [review]
fix

Oh hey, this breaks a test.
Attachment #651684 - Flags: review?(bmcbride)
(Assignee)

Comment 4

6 years ago
Created attachment 651695 [details] [diff] [review]
fix and fixed test
Attachment #651684 - Attachment is obsolete: true
Attachment #651695 - Flags: review?(bmcbride)
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

6 years ago
Created attachment 652274 [details] [diff] [review]
fix
Attachment #651695 - Attachment is obsolete: true
Attachment #652274 - Flags: review?(bmcbride)
(Assignee)

Comment 7

6 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.
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 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

6 years ago
Created attachment 652373 [details] [diff] [review]
v4

> ::: 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 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+
https://hg.mozilla.org/mozilla-central/rev/68ecea83a7da
Status: ASSIGNED → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla17
You need to log in before you can comment on or make changes to this bug.