[webkit-reviews] review granted: [Bug 65140] REGRESSION (r84327-r84329): CSS stylesheets fail to load on www.flagstar.com login page : [Attachment 102709] Proposed fix: Add a temporary state that we merge back into the stylesheet on sheetLoaded(), also make the code more robust to disabled change.

bugzilla-daemon at webkit.org bugzilla-daemon at webkit.org
Mon Aug 8 15:50:10 PDT 2011


Alexey Proskuryakov <ap at webkit.org> has granted Julien Chaffraix
<jchaffraix at webkit.org>'s request for review:
Bug 65140: REGRESSION (r84327-r84329): CSS stylesheets fail to load on
www.flagstar.com login page
https://bugs.webkit.org/show_bug.cgi?id=65140

Attachment 102709: Proposed fix: Add a temporary state that we merge back into
the stylesheet on sheetLoaded(), also make the code more robust to disabled
change.
https://bugs.webkit.org/attachment.cgi?id=102709&action=review

------- Additional Comments from Alexey Proskuryakov <ap at webkit.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=102709&action=review


I'm going to say r+ as fixing a regression is good, but it seems like there
might be a cleaner solution. Some inconsistent ranting below...

> LayoutTests/fast/css/stylesheet-enable-second-alternate.html:2
> +<!DOCTYPE html PUBLIC "-//W3C//DTD XHTML 1.0 Transitional//EN"
"http://www.w3.org/TR/xhtml1/DTD/xhtml1-transitional.dtd">
> +<html xmlns="http://www.w3.org/1999/xhtml" lang="en" xml:lang="en">

Why all this XHTML boilerplate?

> LayoutTests/http/tests/css/resources/slow-loading-sheet.php:3
> +// We sleep here so that we are have enough time to test the different
attributes before the stylesheet is fully loaded.
> +usleep(100);

Sounds flaky.

> Source/WebCore/html/HTMLLinkElement.h:115
> +    enum PendingSheetDisabledStatus { Unset, Disabled, Enabled };

Do we need three states? From a naive point of view, every sheet is pending
enabled unless set otherwise. That would let you use a boolean, improving this
rather awkward naming.

If that's not accurate, perhaps a comment would help.

Also, should the name have "ViaScript" to match m_isEnabledViaScript? In fact,
it feels wrong to have these variables separate, as both just remember what was
set by a script.


More information about the webkit-reviews mailing list