Wait for style sheet to load before querying for imported sheets in style editor

RESOLVED FIXED in Firefox 30



Developer Tools: Style Editor
4 years ago
4 years ago


(Reporter: harth, Assigned: harth)


Firefox 30

Firefox Tracking Flags

(Not tracked)


(Whiteboard: [qa-])


(1 attachment, 1 obsolete attachment)



4 years ago
We also list @imported style sheets in the style editor. We find them by iterating through the rules of each style sheet looking for @import rules. If the sheet isn't loaded yet, this throws an exception.

It's rare that it hasn't loaded before it's included in the stylesheets list, but it can happen if there's an import rule.

Comment 1

4 years ago
Created attachment 8375111 [details] [diff] [review]
async getCSSRules()

This change is basically just the product of a chain reaction of making one thing async. Asking for review from resident Task.jsm expert.

I'm really hoping this will fix the orange here: bug 949355. Try is good so far: https://tbpl.mozilla.org/?tree=Try&rev=cf73273617e5
Assignee: nobody → fayearthur
Attachment #8375111 - Flags: review?(pbrosset)
Comment on attachment 8375111 [details] [diff] [review]
async getCSSRules()

Review of attachment 8375111 [details] [diff] [review]:

Looks good. Should be a lot more stable this way.
The only thing that might be a problem is the getCSSRules method as my comment below explains.
r=me with this fixed (or clarified if I'm wrong).

::: toolkit/devtools/server/actors/stylesheets.js
@@ +111,5 @@
>    }),
>    /**
> +   * Add all the stylesheets in this document and its subframes.
> +   * Assumes the document is loaded.

Can you add a comment about the @return value?

@@ +127,5 @@
> +        for (let iframe of doc.getElementsByTagName("iframe")) {
> +          documents.push(iframe.contentDocument);
> +        }
> +      }
> +      throw new Task.Result(actors);

I have to admit this looks very strange, I had no idea this was the recommended Task.jsm way of resolving to a value.

@@ +138,5 @@
>     *
>     * @param {[DOMStyleSheet]} styleSheets
>     *        Stylesheets to add
>     * @return {[object]}
>     *         Array of actors for each StyleSheetActor created

Comment should be updated here too since the method now returns a promise.

@@ +162,5 @@
>     *
>     * @param  {DOMStyleSheet} styleSheet
>     *         Style sheet to search
>     * @return {array}
>     *         All the imported stylesheets

Comment should be updated here too since the method now returns a promise.

@@ +341,2 @@
> +  getCSSRules: function() {

Please comment this method

@@ +366,5 @@
> +    }.bind(this);
> +
> +    ownerNode.addEventListener("load", onSheetLoaded, false);
> +
> +    return deferred.promise;

It seems that if this method is called several times quickly before the stylesheet is loaded, it will add several event listeners.
I think we should create and cache the promise once so as to return it to subsequent callers of this method.
It's particularly important that this method is public.
Attachment #8375111 - Flags: review?(pbrosset)

Comment 3

4 years ago
Created attachment 8375994 [details] [diff] [review]
async getCSSRules(), addressing comments

Thanks for the comments, I've updated the patch.
Attachment #8375111 - Attachment is obsolete: true
Attachment #8375994 - Flags: review?(pbrosset)
Comment on attachment 8375994 [details] [diff] [review]
async getCSSRules(), addressing comments

Review of attachment 8375994 [details] [diff] [review]:

Looks good to me.
Attachment #8375994 - Flags: review?(pbrosset) → review+


4 years ago
Keywords: checkin-needed
Keywords: checkin-needed
Whiteboard: [fixed-in-fx-team]
Last Resolved: 4 years ago
Resolution: --- → FIXED
Whiteboard: [fixed-in-fx-team]
Target Milestone: --- → Firefox 30


4 years ago
Whiteboard: [qa-]
You need to log in before you can comment on or make changes to this bug.