Bug 51354 – Don't block rendering and script execution on deferred stylesheets

• Status: RESOLVED
• Resolution: FIXED
• Component: Page Loading

[Antti Koivisto]

Bug 50758 deferred loading of stylesheets that are not currently needed. However, they would still block rendering and script execution. Deferred stylesheets should also be made non-blocking.

[Antti Koivisto]

Created attachment 77038 [details]
patch

 - Don't add low priority stylesheets to the document pending sheet count.
 - Resolve media attribute fully for the link element stylesheet load.

[Alexey Proskuryakov]

Comment on attachment 77038 [details]
patch

This is clearly an important change, but it looks like it will break -[WebView setMediaStyle:], won't it?

I don't think that we can break this API.

[Alexey Proskuryakov]

I didn't realize it clearly enough when reviewing the previous patch that we had an API to set media style. But that patch was only changing performance, not correctness AFAICT.

[Joseph Pecoraro]

Would this patch fix:
<http://webkit.org/b/39455> <link> elements with media queries that do not affect the current page can be delayed

It looks like it would to me. Very nice change!

[Joseph Pecoraro]

Err, commented on the wrong bug. I should have commented on bug 50758.
In any case, please update bug 39455 if you think that is fixed!

[Alexey Proskuryakov]

Actually, a comment from bug 39455 seems relevant here:

Firing load events before alternate stylesheets have loaded can easily have compatibility consequences - a script running from onload may want to switch stylesheets, for example.

[Antti Koivisto]

(In reply to comment #6)
> Actually, a comment from bug 39455 seems relevant here:
> 
> Firing load events before alternate stylesheets have loaded can easily have compatibility consequences - a script running from onload may want to switch stylesheets, for example.

This patch should have no impact on when load events fire.

(besides me completely disagreeing about prioritization between non-proven compatibility issues vs. proven performance issues)

[Antti Koivisto]

(In reply to comment #2)
> (From update of attachment 77038 [details])
> This is clearly an important change, but it looks like it will break -[WebView setMediaStyle:], won't it?
> 
> I don't think that we can break this API.

Could you be a bit more specific? How does this patch break setMediaStyle?

[Alexey Proskuryakov]

> Could you be a bit more specific? How does this patch break setMediaStyle?

We should block rendering and script execution for stylesheets of type foobar if someone called -[WebView setMediaStyle:@"foobar"], of course. This is needed for the same reason that makes "screen" stylesheets block script execution in Safari.

[Antti Koivisto]

(In reply to comment #9)
> > Could you be a bit more specific? How does this patch break setMediaStyle?
> 
> We should block rendering and script execution for stylesheets of type foobar if someone called -[WebView setMediaStyle:@"foobar"], of course. This is needed for the same reason that makes "screen" stylesheets block script execution in Safari.

Ok, that is easy to fix. Anything else?

(setMediaStyle is generally pretty broken as searching for '"screen"' reveals)

[Alexey Proskuryakov]

Apparently, we're breaking scripts that access "unnecessary" stylesheets via CSSOM before a load event fires. I'm ready to accept that level of brokenness, even though debugging will be a huge pain if this breaks a live Web site (clearly, the problem would be intermittent). If you can think of any remedy, that would be great.

> Ok, that is easy to fix. Anything else?

I didn't notice anything else when looking through the patch. Those hardcoded "screen" and "print" were commanding most attention.

One thing that seemed worth double checking was whether the document pending sheet count was used for anything else. It can get ugly if it is, or if someone reuses it later. Speaking of which, it looks like a comment in Document.h really needs to be updated.

> This patch should have no impact on when load events fire.

Is this covered by existing regression tests? Seems important enough to add a new test if it isn't.

[Antti Koivisto]

(In reply to comment #11)
> Apparently, we're breaking scripts that access "unnecessary" stylesheets via CSSOM before a load event fires. I'm ready to accept that level of brokenness, even though debugging will be a huge pain if this breaks a live Web site (clearly, the problem would be intermittent). If you can think of any remedy, that would be great.

Yeah, that is going to happen. Note that we already did this with alternate stylesheets.

> One thing that seemed worth double checking was whether the document pending sheet count was used for anything else. It can get ugly if it is, or if someone reuses it later. Speaking of which, it looks like a comment in Document.h really needs to be updated.

I searched around quite a bit and couldn't find anything. The purpose of this collection is to block rendering (and rendering dependent script execution) until sheets have been loaded. Since non-matching sheets by definition don't participate in the rendering, they shouldn't be there either. 

Load even firing is based on actual resource loads, not this collection.

> > This patch should have no impact on when load events fire.
> 
> Is this covered by existing regression tests? Seems important enough to add a new test if it isn't.

Yes, fast/css/sheet-collection-link.html covers this case

[Antti Koivisto]

Created attachment 77101 [details]
Updated patch

Use FrameView::mediaType() instead of hardcoding "screen" and "print".

[Alexey Proskuryakov]

Comment on attachment 77101 [details]
Updated patch

View in context: https://bugs.webkit.org/attachment.cgi?id=77101&action=review

Please do update the comment in Document.h that says "Track the number of currently loading top-level stylesheets."

> WebCore/html/HTMLLinkElement.cpp:235
> -            if (m_loading)
> -                document()->removePendingSheet();
> +            removePendingSheet();

HTMLLinkElement::removePendingSheet() doesn't check m_loading - is it OK to remove the check?

> WebCore/html/HTMLLinkElement.cpp:467
> +    if (type == None)
> +        return;

Can ASSERT_NOT_REACHED() be added here?

> WebCore/html/HTMLLinkElement.h:139
> +    PendingSheetType m_pendingSheetType;

I think that the new code would read better if this was a boolean variable - "type" doesn't really mean anything. Looks like type None sheets shouldn't ever be pending, so switching to a boolean would also make understanding this code easier.

[Alexey Proskuryakov]

I'm wondering if non-screen stylesheets in @import are going to hold us back now.

[Antti Koivisto]

(In reply to comment #14)
> HTMLLinkElement::removePendingSheet() doesn't check m_loading - is it OK to remove the check?
> Can ASSERT_NOT_REACHED() be added here?

Yea and no. The m_loading check was part of the fragile state management in the old code. This patch keeps track of whether we made the request in the first place so the m_loading check (and other similar checks) can be removed. That None check is equivalent of the m_loading check and so a valid case.

> I think that the new code would read better if this was a boolean variable - "type" doesn't really mean anything. Looks like type None sheets shouldn't ever be pending, so switching to a boolean would also make understanding this code easier.

I found it difficult to represent three states with a boolean.

[Antti Koivisto]

(In reply to comment #15)
> I'm wondering if non-screen stylesheets in @import are going to hold us back now.

Yeah. I suspect @imports with media rules are rarer, so it might be less important. I don't have any proof though.

[Antti Koivisto]

http://trac.webkit.org/changeset/74476

[WebKit Review Bot]

http://trac.webkit.org/changeset/74476 might have broken GTK Linux 32-bit Release
The following tests are not passing:
http/tests/local/stylesheet-and-script-load-order-media-print.html