Improve stylesheet error loading handling

• Component: Page Loading
• Status: RESOLVED DUPLICATE

[Rob Buis]

Improve stylesheet error loading handling

[Rob Buis]

Created attachment 386211 [details]
Patch

[Rob Buis]

Created attachment 386317 [details]
Patch

[youenn fablet]

Comment on attachment 386317 [details]
Patch

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

> Source/WebCore/ChangeLog:12
> +        Behavior matches Firefox.

Change looks fine to me, I am wondering about backward compat though.
Does it match Chrome behavior? If not, do you know if Chrome is planning to align here?

[Darin Adler]

Comment on attachment 386317 [details]
Patch

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

Behavior change looks good, but the way it’s implemented seems really confusing and sort of backward.

> Source/WebCore/css/StyleSheetContents.cpp:327
> +        const_cast<CachedCSSStyleSheet*>(cachedStyleSheet)->setStatus(CachedResource::LoadError);

Seems really unfortunate to do this with a const_cast and a side effect. Can we do something else to make it clear this is correct? Like do this at another level, inside the cache, instead of here?

> Source/WebCore/loader/cache/CachedCSSStyleSheet.cpp:155
> +    bool typeOK = mimeType.isEmpty() || equalLettersIgnoringASCIICase(mimeType, "text/css") || equalLettersIgnoringASCIICase(mimeType, "application/x-unknown-content-type") || !isValidContentType(mimeType);

I don’t understand this change.

This code says "all invalid content types are OK". Why is that correct? Nothing in the change log or the comments makes that clear.

[Rob Buis]

(In reply to youenn fablet from comment #3)
> Comment on attachment 386317 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=386317&action=review
> 
> > Source/WebCore/ChangeLog:12
> > +        Behavior matches Firefox.
> 
> Change looks fine to me, I am wondering about backward compat though.
> Does it match Chrome behavior? If not, do you know if Chrome is planning to
> align here?

No, I try to explicitly state the implementations that support the feature, so if I do not list it it means I believe the implementation does not support it.

I don't know if Chrome is planning to support it, I could ask but I think I'll just upload a patch and see if there is interest.

I noticed recently that this existing bug is probably more relevant, so I'll move my work over there:
https://bugs.webkit.org/show_bug.cgi?id=189913

[Rob Buis]

Comment on attachment 386317 [details]
Patch

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

>> Source/WebCore/css/StyleSheetContents.cpp:327
>> +        const_cast<CachedCSSStyleSheet*>(cachedStyleSheet)->setStatus(CachedResource::LoadError);
> 
> Seems really unfortunate to do this with a const_cast and a side effect. Can we do something else to make it clear this is correct? Like do this at another level, inside the cache, instead of here?

Yes that was a bit hacky. I believe I found a cleaner way, continuing my work on https://bugs.webkit.org/show_bug.cgi?id=189913.

>> Source/WebCore/loader/cache/CachedCSSStyleSheet.cpp:155
>> +    bool typeOK = mimeType.isEmpty() || equalLettersIgnoringASCIICase(mimeType, "text/css") || equalLettersIgnoringASCIICase(mimeType, "application/x-unknown-content-type") || !isValidContentType(mimeType);
> 
> I don’t understand this change.
> 
> This code says "all invalid content types are OK". Why is that correct? Nothing in the change log or the comments makes that clear.

Will provide a link to the spec soon, whenever this shows up on https://bugs.webkit.org/show_bug.cgi?id=189913.

[Rob Buis]

*** This bug has been marked as a duplicate of bug 189913 ***