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

Status: RESOLVED FIXED

(Toolkit :: Add-ons Manager, defect)

Description

[Masatoshi Kimura [:emk]]

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

Comment 1

[Geoff Lankow (:darktrojan)]

It also shouldn't kill the detail view when the optionsURL points to something useless.

Comment 2

[Geoff Lankow (:darktrojan)]

Attachment: fix

Comment 3

[Geoff Lankow (:darktrojan)]

Comment on attachment 651684 [details] [diff] [review]
fix

Oh hey, this breaks a test.

Comment 4

[Geoff Lankow (:darktrojan)]

Attachment: fix and fixed test

Comment 5

[Blair McBride [:Unfocused] (UNAVAILABLE)]

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?

Comment 6

[Geoff Lankow (:darktrojan)]

Attachment: fix

Comment 7

[Geoff Lankow (:darktrojan)]

(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

[Blair McBride [:Unfocused] (UNAVAILABLE)]

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

[Blair McBride [:Unfocused] (UNAVAILABLE)]

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?

Comment 10

[Geoff Lankow (:darktrojan)]

Attachment: 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.

Comment 11

[Blair McBride [:Unfocused] (UNAVAILABLE)]

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

Comment 12

[Geoff Lankow (:darktrojan)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/68ecea83a7da

(Try: https://tbpl.mozilla.org/?tree=Try&rev=e611d711a765)

Comment 13

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/68ecea83a7da