Bug 121018 - Move <style scoped> code behind ENABLE(STYLE_SCOPED)
Summary: Move <style scoped> code behind ENABLE(STYLE_SCOPED)
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: CSS (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Andreas Kling
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2013-09-08 21:21 PDT by Andreas Kling
Modified: 2013-09-17 01:12 PDT (History)
10 users (show)

See Also:


Attachments
Patch (11.65 KB, patch)
2013-09-08 21:25 PDT, Andreas Kling
no flags Details | Formatted Diff | Diff
Patch (12.99 KB, patch)
2013-09-08 21:28 PDT, Andreas Kling
darin: review+
buildbot: commit-queue-
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Andreas Kling 2013-09-08 21:21:55 PDT
Move <style scoped> code behind ENABLE(STYLE_SCOPED)
Comment 1 Andreas Kling 2013-09-08 21:25:21 PDT
Created attachment 211006 [details]
Patch
Comment 2 Andreas Kling 2013-09-08 21:28:46 PDT
Created attachment 211007 [details]
Patch
Comment 3 Darin Adler 2013-09-08 21:38:05 PDT
Comment on attachment 211007 [details]
Patch

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.
Comment 4 Build Bot 2013-09-08 22:03:12 PDT
Comment on attachment 211007 [details]
Patch

Attachment 211007 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.appspot.com/results/1728263
Comment 5 Build Bot 2013-09-08 22:23:28 PDT
Comment on attachment 211007 [details]
Patch

Attachment 211007 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.appspot.com/results/1735162
Comment 6 Build Bot 2013-09-12 17:30:18 PDT
Comment on attachment 211007 [details]
Patch

Attachment 211007 [details] did not pass win-ews (win):
Output: http://webkit-queues.appspot.com/results/1846072
Comment 7 Andreas Kling 2013-09-17 01:12:34 PDT
Committed r155931: <http://trac.webkit.org/changeset/155931>