[webkit-reviews] review denied: [Bug 65140] REGRESSION (r84327-r84329): CSS stylesheets fail to load on www.flagstar.com login page : [Attachment 103566] Proposed fix 3: Even better fix as the previous could lose some information in the way, also tightened the checks between disabled() and m_isEnabledViaScript.

[bugzilla-daemon at webkit.org]

Antti Koivisto <koivisto at iki.fi> has denied 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 103566: Proposed fix 3: Even better fix as the previous could lose
some information in the way, also tightened the checks between disabled() and
m_isEnabledViaScript.
https://bugs.webkit.org/attachment.cgi?id=103566&action=review

------- Additional Comments from Antti Koivisto <koivisto at iki.fi>
View in context: https://bugs.webkit.org/attachment.cgi?id=103566&action=review


The logic is pretty convoluted and hard to follow. Is that the case with the
spec as well? 

r- for all the style issues and open questions.

> Source/WebCore/html/HTMLLinkElement.cpp:84
>  void HTMLLinkElement::setDisabled(bool disabled)
>  {

The code seems to assume this is only ever invoked from script, via DOM. It
would be nice to assert that somehow.

> Source/WebCore/html/HTMLLinkElement.cpp:89
> +	   // If we are in the middle of loading a stylesheet, we store the
information from this call
> +	   // as it is a common pattern to disable / enable the stylesheet
through JS (regardless of
> +	   // whether the sheet was loaded).
> +	   // See https://bugs.webkit.org/show_bug.cgi?id=65140

The verbose comment here adds little information. We don't generally link to
bugzilla on every code change.

> Source/WebCore/html/HTMLLinkElement.cpp:91
> +	   if (m_relAttribute.m_isStyleSheet && m_loading)
> +	       setEnabledViaScript(!disabled);

Why these specific conditions? Can't non-stylesheet links be disabled? Why only
when loading? setDisabled() does not have these tests in other branches.

Could the setEnabledViaScript call simply be moved to the top of this function?
It seems to be called in all cases, except for these tests.

> Source/WebCore/html/HTMLLinkElement.cpp:99
> +    if (wasDisabled == disabled) {
> +	   // If JS forces the sheet to be enabled, we need to force a
recalcStyleSelector or
> +	   // we may not apply the stylesheet (for example for alternate
stylesheet).

I don't quite understand why we need to take some special action in a case
where nothing supposedly changes?

> Source/WebCore/html/HTMLLinkElement.cpp:100
> +	   if (isEnabledViaScript() != !disabled) {

Triple negatives make my head hurt.

> Source/WebCore/html/HTMLLinkElement.cpp:102
> +	       setEnabledViaScript(!disabled);
> +	       document()->styleSelectorChanged(DeferRecalcStyle);

If changing the enabled view script state requires style recalc, would it
better to trigger it in that function itself?

> Source/WebCore/html/HTMLLinkElement.cpp:474
> +    if (m_sheet)
> +	   return m_sheet->disabled();

Please handle the special !m_sheet case as early return and the regular case at
the end of the function.

> Source/WebCore/html/HTMLLinkElement.cpp:479
> +    // FF disagrees with the spec and always has an associated stylesheet if
the 'rel' attribute indicates a
> +    // stylesheet and href is provided (regardless of whether the resource
may be in error). As we store the
> +    // enabled state in m_isEnabledViaScript while loading, return this
information to be consistent with FF
> +    // and our future self here.

Do you mean that we violate the spec here to be compatible with FF? Why? What
does the spec say?

Our future self? I don't understand the comment.

> Source/WebCore/html/HTMLLinkElement.h:60
> -    bool isEnabledViaScript() const { return m_isEnabledViaScript; }
> +    bool isEnabledViaScript() const {
checkDisabledAndEnabledViaScriptState(); return m_isEnabledViaScript ==
Enabled; }

Since the API is setDisabled, reversing the logic might make some of the code
read better (isDisabledViaScript())

> Source/WebCore/html/HTMLLinkElement.h:61
> +    void setEnabledViaScript(bool enabled) { m_isEnabledViaScript = enabled
? Enabled : Disabled; }

Why is the setter public?  Since the API is setDisabled, reversing the logic
might make some of the code read better.

> Source/WebCore/html/HTMLLinkElement.h:104
> +    void checkDisabledAndEnabledViaScriptState() const
> +    {
> +	   // When we are loading, m_isEnabledViaScript stores the information
if JS calls us and can thus have any value.
> +	   // If m_isEnabledViaScript is Unset, the style sheet may be unabled
or disabled.
> +	   // Finally if we are not in the 2 previous cases, then JS should
override our disabled value so we must make sure
> +	   // that both values match.
> +	   ASSERT(isLoading() || m_isEnabledViaScript == Unset ||
((m_isEnabledViaScript == Disabled && sheet()->disabled()) ||
(m_isEnabledViaScript == Enabled && !sheet()->disabled())));
> +    }

Confusingly this function is a pure assertion. It would be better to replace it
with a boolean debug-only function (isDisabledStateConsistent() or something)
and ASSERT on that instead.

The comment is overly verbose and adds little value.

> Source/WebCore/html/HTMLLinkElement.h:118
> -    bool m_isEnabledViaScript;
> +    enum EnabledViaScriptState { Unset, Enabled, Disabled };
> +    EnabledViaScriptState m_isEnabledViaScript;

m_is* naming is for booleans. Something like m_scriptState might be good, with
values like Unset/EnabledViaScript/DisabledViaScript.