Bug 67790

Summary: <style scoped>: Implement registering of <style scoped> with the scoping element
Product: WebKit Reporter: Roland Steiner <rolandsteiner>
Component: DOMAssignee: Roland Steiner <rolandsteiner>
Status: RESOLVED FIXED    
Severity: Normal CC: abarth, dglazkov, dominicc, gustavo, hyatt, koivisto, macpherson, morrita, webkit.review.bot, xan.lopez
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: All   
OS: All   
Bug Depends on: 67718, 73893    
Bug Blocks: 49142, 67720    
Attachments:
Description Flags
work-in-progress
none
work-in-progress
none
Patch
none
updated patch
none
patch with flag
none
patch with flag, corrected
none
patch with flag, for EWS
none
patch, updated
none
patch, updated dglazkov: review+, webkit.review.bot: commit-queue-

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