[webkit-reviews] review granted: [Bug 121018] Move <style scoped> code behind ENABLE(STYLE_SCOPED) : [Attachment 211007] Patch

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has granted Andreas Kling <akling at apple.com>'s
request for review:
Bug 121018: Move <style scoped> code behind ENABLE(STYLE_SCOPED)
https://bugs.webkit.org/show_bug.cgi?id=121018

Attachment 211007: Patch
https://bugs.webkit.org/attachment.cgi?id=211007&action=review

------- Additional Comments from Darin Adler <darin at apple.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=211007&action=review


r=me as long as it doesn’t break the build

> Source/WebCore/css/StyleResolver.cpp:469
> +#if ENABLE(STYLE_SCOPED)
>      if (parent->hasScopedHTMLStyleChild())
>	   return 0;
> +#endif

The way I’d prefer to do this is to have hasScopedHTMLStyleChild be defined but
be an inline function that returns false.

> Source/WebCore/css/StyleResolver.cpp:662
> +#if ENABLE(STYLE_SCOPED)
>      if (element->hasScopedHTMLStyleChild())
>	   return false;
> +#endif

Ditto.

> Source/WebCore/css/StyleResolver.cpp:749
> +#if ENABLE(STYLE_SCOPED)
>      if (state.styledElement()->hasScopedHTMLStyleChild())
>	   return 0;
> +#endif

Ditto.

> Source/WebCore/dom/Node.h:591
> +#if ENABLE(STYLE_SCOPED)
>      virtual void registerScopedHTMLStyleChild();
>      virtual void unregisterScopedHTMLStyleChild();
>      size_t numberOfScopedHTMLStyleChildren() const;
> +#endif

Bizarre to have this all in Node!

> Source/WebCore/dom/Node.h:637
> +#if ENABLE(STYLE_SCOPED)
>	   HasScopedHTMLStyleChildFlag = 1 << 22,
> +#endif

Not sure this is needed.

> Source/WebCore/dom/ShadowRoot.h:75
> +#if ENABLE(STYLE_SCOPED)
>      virtual void registerScopedHTMLStyleChild() OVERRIDE;
>      virtual void unregisterScopedHTMLStyleChild() OVERRIDE;
> +#endif

At least mark these final?

> Source/WebCore/html/HTMLStyleElement.cpp:82
> +#if ENABLE(STYLE_SCOPED)
>      else if (name == scopedAttr &&
ContextFeatures::styleScopedEnabled(&document()))
>	   scopedAttributeChanged(!value.isNull());
> +#endif

Are we keeping this ContextFeatures thing at all? Who is using it in WebKit?

> Source/WebCore/html/HTMLStyleElement.cpp:187
> +bool HTMLStyleElement::scoped() const
> +{
> +    return fastHasAttribute(scopedAttr) &&
ContextFeatures::styleScopedEnabled(&document());
> +}
> +
> +void HTMLStyleElement::setScoped(bool scopedValue)
> +{
> +    setBooleanAttribute(scopedAttr, scopedValue);
> +}

These should be generated by the bindings rather than writing explicit C++
code.

> Source/WebCore/html/HTMLStyleElement.cpp:213
> +#if ENABLE(STYLE_SCOPED)
>	   if (m_scopedStyleRegistrationState == NotRegistered && (scoped() ||
isInShadowTree()))
>	       registerWithScopingNode(scoped());
> +#endif

Lame how this calls scoped() twice. That’s not necessarily a fast function.