Improve stylesheet error loading handling
Created attachment 386211 [details] Patch
Created attachment 386317 [details] Patch
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?
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.
(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
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.
*** This bug has been marked as a duplicate of bug 189913 ***