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

RESOLVED FIXED 67790

https://bugs.webkit.org/show_bug.cgi?id=67790

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

Roland Steiner

Reported 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.

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
Show Obsolete (8) View All Add attachment proposed patch, testcase, etc.

Roland Steiner

Comment 1 2011-09-09 20:02:59 PDT

Created attachment 106955 [details] work-in-progress work-in-progress patch, also get linker symbols from EWS

Roland Steiner

Comment 2 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)

Gustavo Noronha (kov)

Comment 3 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

Roland Steiner

Comment 4 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.

Roland Steiner

Comment 5 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.

Roland Steiner

Comment 6 2011-12-05 20:56:18 PST

Created attachment 117986 [details] patch with flag patch, same as before, just added ENABLE(STYLE_SCOPED) flag

Early Warning System Bot

Comment 7 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

WebKit Review Bot

Comment 8 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

Gustavo Noronha (kov)

Comment 9 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

WebKit Review Bot

Comment 10 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

Roland Steiner

Comment 11 2011-12-05 22:33:08 PST

Created attachment 117989 [details] patch with flag, corrected missed a change, new patch

Roland Steiner

Comment 12 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).

Roland Steiner

Comment 13 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

Roland Steiner

Comment 14 2012-01-04 21:18:12 PST

New Year request for review! :)

Adam Barth

Comment 15 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?

Adam Barth

Comment 16 2012-01-04 23:42:05 PST

(and the other patches you pinged)

Dimitri Glazkov (Google)

Comment 17 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?

Roland Steiner

Comment 18 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.

Roland Steiner

Comment 19 2012-01-23 23:26:42 PST

Created attachment 123702 [details] patch, updated Slightly updated version of patch (and de-conflicted)

Roland Steiner

Comment 20 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

Roland Steiner

Comment 21 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.

WebKit Review Bot

Comment 22 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

Dimitri Glazkov (Google)

Comment 23 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.

Roland Steiner

Comment 24 2012-01-24 22:48:47 PST

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

Note You need to log in before you can comment on or make changes to this bug.

Status RESOLVED

Resolution FIXED

Priority P2

Severity Normal

Classification Unclassified

Version 528+ (Nightly build)

Hardware All

OS All

Product WebKit

Component DOM

Assignee

Roland Steiner

Reported

2011-09-08 10:59 PDT

Modified

2012-01-24 22:48 PST History

CC List

10 users Show

URL

Keywords

Depends on

67718 73893

Blocks

49142 67720

Dependencies

tree graph