Bug 67790 - <style scoped>: Implement registering of <style scoped> with the scoping element
Summary: <style scoped>: Implement registering of <style scoped> with the scoping element
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: DOM (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Roland Steiner
URL:
Keywords:
Depends on: 67718 73893
Blocks: 49142 67720
  Show dependency treegraph
 
Reported: 2011-09-08 10:59 PDT by Roland Steiner
Modified: 2012-01-24 22:48 PST (History)
10 users (show)

See Also:


Attachments
work-in-progress (26.06 KB, patch)
2011-09-09 20:02 PDT, Roland Steiner
no flags Details | Formatted Diff | Diff
work-in-progress (35.50 KB, patch)
2011-09-09 20:05 PDT, Roland Steiner
no flags Details | Formatted Diff | Diff
Patch (37.21 KB, patch)
2011-09-14 16:13 PDT, Roland Steiner
no flags Details | Formatted Diff | Diff
updated patch (29.96 KB, patch)
2011-11-15 21:11 PST, Roland Steiner
no flags Details | Formatted Diff | Diff
patch with flag (30.54 KB, patch)
2011-12-05 20:56 PST, Roland Steiner
no flags Details | Formatted Diff | Diff
patch with flag, corrected (30.61 KB, patch)
2011-12-05 22:33 PST, Roland Steiner
no flags Details | Formatted Diff | Diff
patch with flag, for EWS (30.69 KB, patch)
2011-12-06 00:49 PST, Roland Steiner
no flags Details | Formatted Diff | Diff
patch, updated (32.36 KB, patch)
2012-01-23 23:26 PST, Roland Steiner
no flags Details | Formatted Diff | Diff
patch, updated (31.72 KB, patch)
2012-01-24 00:40 PST, Roland Steiner
dglazkov: review+
webkit.review.bot: commit-queue-
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Roland Steiner 2011-09-08 10:59:55 PDT
For fast processing of <style scoped> it is necessary that the containing element (i.e., the scope) "knows" of direct <style scoped> children.
Comment 1 Roland Steiner 2011-09-09 20:02:59 PDT
Created attachment 106955 [details]
work-in-progress

work-in-progress patch, also get linker symbols from EWS
Comment 2 Roland Steiner 2011-09-09 20:05:39 PDT
Created attachment 106956 [details]
work-in-progress

work-in-progress patch (combined with IDL patch to allow it to compile)
Comment 3 Gustavo Noronha (kov) 2011-09-10 07:09:58 PDT
Comment on attachment 106956 [details]
work-in-progress

Attachment 106956 [details] did not pass gtk-ews (gtk):
Output: http://queues.webkit.org/results/9639029
Comment 4 Roland Steiner 2011-09-14 16:13:24 PDT
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.
Comment 5 Roland Steiner 2011-11-15 21:11:53 PST
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.
Comment 6 Roland Steiner 2011-12-05 20:56:18 PST
Created attachment 117986 [details]
patch with flag

patch, same as before, just added ENABLE(STYLE_SCOPED) flag
Comment 7 Early Warning System Bot 2011-12-05 21:16:18 PST
Comment on attachment 117986 [details]
patch with flag

Attachment 117986 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/10689733
Comment 8 WebKit Review Bot 2011-12-05 21:27:37 PST
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
Comment 9 Gustavo Noronha (kov) 2011-12-05 22:02:16 PST
Comment on attachment 117986 [details]
patch with flag

Attachment 117986 [details] did not pass gtk-ews (gtk):
Output: http://queues.webkit.org/results/10736570
Comment 10 WebKit Review Bot 2011-12-05 22:32:40 PST
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
Comment 11 Roland Steiner 2011-12-05 22:33:08 PST
Created attachment 117989 [details]
patch with flag, corrected

missed a change, new patch
Comment 12 Roland Steiner 2011-12-06 00:37:27 PST
Note: Windows should be pacified as well, once the patch for bug 73893 is landed (<style scoped> being incorrectly enabled by default on Windows).
Comment 13 Roland Steiner 2011-12-06 00:49:19 PST
Created attachment 118004 [details]
patch with flag, for EWS

re-submit same patch to make sure EWS is happy
Comment 14 Roland Steiner 2012-01-04 21:18:12 PST
New Year request for review! :)
Comment 15 Adam Barth 2012-01-04 23:41:49 PST
> New Year request for review! :)

Do you have someone in mind who you think should review this patch?
Comment 16 Adam Barth 2012-01-04 23:42:05 PST
(and the other patches you pinged)
Comment 17 Dimitri Glazkov (Google) 2012-01-05 09:56:07 PST
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?
Comment 18 Roland Steiner 2012-01-10 00:52:50 PST
(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.
Comment 19 Roland Steiner 2012-01-23 23:26:42 PST
Created attachment 123702 [details]
patch, updated

Slightly updated version of patch (and de-conflicted)
Comment 20 Roland Steiner 2012-01-24 00:40:57 PST
Created attachment 123704 [details]
patch, updated

Slightly updated version of patch (and de-conflicted), this time with less cruft
Comment 21 Roland Steiner 2012-01-24 00:43:34 PST
(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.
Comment 22 WebKit Review Bot 2012-01-24 03:40:11 PST
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
Comment 23 Dimitri Glazkov (Google) 2012-01-24 08:59:26 PST
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.
Comment 24 Roland Steiner 2012-01-24 22:48:47 PST
Committed r105849: <http://trac.webkit.org/changeset/105849>