Bug 77853

Created attachment 125771 [details] Patch patch - contains same forwarding as registration code in bug 67790. This was moved more or less verbatim from Element into ContainerNode. See discussion there regarding cleanup (moving into helper class or some such).

Attachment 125771 [details] did not pass style-queue: Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'ChangeLog', u'LayoutTests/ChangeLog', u'La..." exit_code: 1 Source/WebCore/dom/ContainerNode.cpp:1092: The return type should use PassOwnPtr instead of OwnPtr. [readability/pass_ptr] [5] Total errors found: 1 in 24 files If any of these errors are false positives, please file a bug against check-webkit-style.

Note: the function the style bot complains about is virtual, so I can't change the signature (or at least that should be a separate patch). Will try to fix Win build failure in a follow-up patch, provided the general direction of the patch is OK.

Comment on attachment 125771 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=125771&action=review > Source/WebCore/html/HTMLStyleElement.cpp:116 > + ContainerNode* scope = parentNode(); I have an alternative idea, but not sure if it's better. Instead of adding a new type of rare data, which seems rather high-caliber, we could declare a StyleScope interface, and have both element and ShadowRoot implement it. Since the rare data is just an int, we could just make it a member variable of the ShadowRoot. WDYT?

(In reply to comment #4) > I have an alternative idea, but not sure if it's better. Instead of adding a new type of rare data, which seems rather high-caliber, we could declare a StyleScope interface, and have both element and ShadowRoot implement it. Since the rare data is just an int, we could just make it a member variable of the ShadowRoot. WDYT? Hm, but how to get from [HTMLStyleElement]->parentNode() to the StyleScope interface? It seems I'd either need (a) dynamic_cast or (b) have to branch it like "if (parentNode->isElementNode()) { ... } else if (parentNode->isShadowRoot()) { ... }". In the latter case I could just call the respective implementation directly, without going through an interface (?). Alternatively, (c) I could declare the interface functions virtual on ContainerNode (thus making them directly accessible from parentNode()) and implement them in Element and ShadowRoot. Variant (d) of that: StyleScope could become a super-class of ContainerNode to encapsulate the interface, but implementation would still have to happen in Element & ShadowRoot. Using ContainerNodeRareData just seemed simpler to me... ;)

Comment on attachment 125771 [details] Patch This seems wrong. Why isn't ShadowRoot an Element if it is supposed to act like one?

(In reply to comment #6) > (From update of attachment 125771 [details]) > This seems wrong. Why isn't ShadowRoot an Element if it is supposed to act like one? Other than this behavior, there's really nothing about ShadowRoot that makes it look like an Element. It doesn't receive events, has no attributes, it's never rendered (so no styles or dimension/position information). Also, it can't be cloned into another ShadowRoot or appended to a document tree. (more here: http://dvcs.w3.org/hg/webcomponents/raw-file/tip/spec/shadow/index.html). If we were to make it an Element, we'd have to add a larger set of exceptions in handling the specific case of the ShadowRoot, compared to just this one.

Created attachment 126253 [details] Patch patch, should fix Win bot. Still ContainerNodeRareData version: it is the most straightforward, and I wanted to avoid making hasScopedHTMLStyleChild virtual, as it is used rather heavily in <style scoped> CSS code. However, if you still prefer any of the versions (a) to (d) of comment #5 - or '(e) something completely different' - I'm happy to change the implementation.

Comment on attachment 126253 [details] Patch The patch doesn't seem to apply. Also -- can we not just reuse NodeRareData? Do we have to introduce the ContainerNodeRareData?

Created attachment 126451 [details] Patch Certainly can do! Updated patch, moved registration to Node / NodeRareData as suggested.

Created attachment 126456 [details] Patch Same patch as above, missed a small bit of cleanup.

Comment on attachment 126456 [details] Patch This looks great, but can you please land it by hand? I could be superstitious and old-fashioned, but I'd watch the tree after landing this patch :)

Will do, once the tree goes back to a sane state.

Committed r107793: <http://trac.webkit.org/changeset/107793>