<style scoped>: Implement registering of <style scoped> with the scoping element

• Component: DOM
• Status: RESOLVED FIXED

[Roland Steiner]

For fast processing of <style scoped> it is necessary that the containing element (i.e., the scope) "knows" of direct <style scoped> children.

[Roland Steiner]

Created attachment 106955 [details]
work-in-progress

work-in-progress patch, also get linker symbols from EWS

[Roland Steiner]

Created attachment 106956 [details]
work-in-progress

work-in-progress patch (combined with IDL patch to allow it to compile)

[Gustavo Noronha (kov)]

Comment on attachment 106956 [details]
work-in-progress

Attachment 106956 [details] did not pass gtk-ews (gtk):
Output: http://queues.webkit.org/results/9639029

[Roland Steiner]

Created attachment 107417 [details]
Patch

Functional patch, but will not compile without the patch for 67718. Also still a bit-work-in-progressy, as scoped stylesheets are not registered unless in the document, which may or may not matter.

[Roland Steiner]

Created attachment 115316 [details]
updated patch

Updated patch - removed conflicts and work-in-progress comments, otherwise the same as the previous patch. Will r? once 67718 has landed.

[Roland Steiner]

Created attachment 117986 [details]
patch with flag

patch, same as before, just added ENABLE(STYLE_SCOPED) flag

[Early Warning System Bot]

Comment on attachment 117986 [details]
patch with flag

Attachment 117986 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/10689733

[WebKit Review Bot]

Comment on attachment 117986 [details]
patch with flag

Attachment 117986 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/10731897

[Gustavo Noronha (kov)]

Comment on attachment 117986 [details]
patch with flag

Attachment 117986 [details] did not pass gtk-ews (gtk):
Output: http://queues.webkit.org/results/10736570

[WebKit Review Bot]

Comment on attachment 117986 [details]
patch with flag

Attachment 117986 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/10739018

[Roland Steiner]

Created attachment 117989 [details]
patch with flag, corrected

missed a change, new patch

[Roland Steiner]

Note: Windows should be pacified as well, once the patch for bug 73893 is landed (<style scoped> being incorrectly enabled by default on Windows).

[Roland Steiner]

Created attachment 118004 [details]
patch with flag, for EWS

re-submit same patch to make sure EWS is happy

[Roland Steiner]

New Year request for review! :)

[Adam Barth]

> New Year request for review! :)

Do you have someone in mind who you think should review this patch?

[Adam Barth]

(and the other patches you pinged)

[Dimitri Glazkov (Google)]

Comment on attachment 118004 [details]
patch with flag, for EWS

View in context: https://bugs.webkit.org/attachment.cgi?id=118004&action=review

> Source/WebCore/dom/Element.cpp:1851
> +std::size_t Element::numberOfScopedHTMLStyleChildren() const

The std:: prefix is not needed here.

> Source/WebCore/dom/Element.h:298
> +    void registerScopedHTMLStyleChild();

I wonder if we should avoid adding more members to Element.h. It's already pretty fat. Fat classes suck.

Can these methods be organized into a helper class somehow? Just thinking outloud.

> Source/WebCore/dom/Element.h:301
> +    std::size_t numberOfScopedHTMLStyleChildren() const;

Ditto.

> Source/WebCore/dom/ElementRareData.h:58
> +    std::size_t m_numberOfScopedHTMLStyleChildren;

Ditto.

> Source/WebCore/testing/Internals.idl:34
> +        unsigned long numberOfScopedHTMLStyleChildren(in Element element) raises(DOMException);

Isn't there a [Conditional] directive or something to avoid the ifdefs?

[Roland Steiner]

(In reply to comment #15)
> Do you have someone in mind who you think should review this patch?

Well, Antti reviewed some of the patches so far. I guess Dave Hyatt would also be an obvious (if overworked) candidate.

[Roland Steiner]

Created attachment 123702 [details]
patch, updated

Slightly updated version of patch (and de-conflicted)

[Roland Steiner]

Created attachment 123704 [details]
patch, updated

Slightly updated version of patch (and de-conflicted), this time with less cruft

[Roland Steiner]

(In reply to comment #17)
> > Source/WebCore/dom/Element.cpp:1851
> > +std::size_t Element::numberOfScopedHTMLStyleChildren() const
> 
> The std:: prefix is not needed here.

removed all occurrences of std::.

> > Source/WebCore/dom/Element.h:298
> > +    void registerScopedHTMLStyleChild();
> 
> I wonder if we should avoid adding more members to Element.h. It's already pretty fat. Fat classes suck.
> 
> Can these methods be organized into a helper class somehow? Just thinking outloud.

At this point I would like to avoid anything that would increase the patch size. I'm happy to follow up with a clean-up patch afterwards.

> > Source/WebCore/testing/Internals.idl:34
> > +        unsigned long numberOfScopedHTMLStyleChildren(in Element element) raises(DOMException);
> 
> Isn't there a [Conditional] directive or something to avoid the ifdefs?

I took out a leaf from the ShadowRoot book and made the function unconditional - it simply returns 0 in the unsupported case.

[WebKit Review Bot]

Comment on attachment 123704 [details]
patch, updated

Attachment 123704 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/11118699

New failing tests:
media/audio-garbage-collect.html

[Dimitri Glazkov (Google)]

Comment on attachment 123704 [details]
patch, updated

View in context: https://bugs.webkit.org/attachment.cgi?id=123704&action=review

> Source/WebCore/dom/Element.h:310
> +#if ENABLE(STYLE_SCOPED)
> +    void registerScopedHTMLStyleChild();
> +    void unregisterScopedHTMLStyleChild();
> +    bool hasScopedHTMLStyleChild() const;
> +    size_t numberOfScopedHTMLStyleChildren() const;
> +#endif

This plumbing-through just looks unkempt. Please consider cleaning up in follow-up patches.

> Source/WebCore/testing/Internals.cpp:167
> +std::size_t Internals::numberOfScopedHTMLStyleChildren(const Element* element, ExceptionCode& ec) const

Please kill remaining std prefixes.

[Roland Steiner]

Committed r105849: <http://trac.webkit.org/changeset/105849>