Bug 118286

Summary: is/toHTMLStyleElement should use Element* for its argument
Product: WebKit Reporter: Kangil Han <kangil.han>
Component: WebCore Misc.Assignee: Kangil Han <kangil.han>
Status: RESOLVED FIXED    
Severity: Normal CC: commit-queue, esprehn+autocc, glenn, kling, koivisto, macpherson, menard
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Patch
none
Patch
none
Patch none

Kangil Han
Reported 2013-07-02 01:32:13 PDT
is/toHTMLStyleElement should use Element* for its argument
Attachments
Patch (7.04 KB, patch)
2013-07-02 01:35 PDT, Kangil Han
no flags
Patch (7.22 KB, patch)
2013-07-02 05:04 PDT, Kangil Han
no flags
Patch (7.24 KB, patch)
2013-07-02 05:29 PDT, Kangil Han
no flags
Kangil Han
Comment 1 2013-07-02 01:35:14 PDT
Kangil Han
Comment 2 2013-07-02 01:37:55 PDT
I would prefer gradual change of argument not to break bot. :)
Kangil Han
Comment 3 2013-07-02 01:42:32 PDT
For the record, this is a follow up patch for https://bugs.webkit.org/show_bug.cgi?id=118235#c2
Andreas Kling
Comment 4 2013-07-02 04:22:32 PDT
Comment on attachment 205878 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=205878&action=review > Source/WebCore/css/CSSStyleSheet.cpp:68 > - || isHTMLStyleElement(parentNode) > + || isHTMLStyleElement(toElement(parentNode)) Are we sure that 'parentNode' is always an Element at this point? > Source/WebCore/dom/Node.cpp:2536 > for (Node* child = firstChild(); child; child = child->nextSibling()) { > - if (isHTMLStyleElement(child) && toHTMLStyleElement(child)->isRegisteredAsScoped()) > + if (isHTMLStyleElement(toElement(child)) && toHTMLStyleElement(toElement(child))->isRegisteredAsScoped()) > count++; > } This looks wrong; you're calling toElement() on every child node, wouldn't this assert? Maybe we could rewrite this loop using ElementTraversal instead? Then you wouldn't need to worry about Nodes at all.
Kangil Han
Comment 5 2013-07-02 05:04:17 PDT
Kangil Han
Comment 6 2013-07-02 05:05:46 PDT
(In reply to comment #4) > > Source/WebCore/css/CSSStyleSheet.cpp:68 > > - || isHTMLStyleElement(parentNode) > > + || isHTMLStyleElement(toElement(parentNode)) > > Are we sure that 'parentNode' is always an Element at this point? > Done. > > Source/WebCore/dom/Node.cpp:2536 > > for (Node* child = firstChild(); child; child = child->nextSibling()) { > > - if (isHTMLStyleElement(child) && toHTMLStyleElement(child)->isRegisteredAsScoped()) > > + if (isHTMLStyleElement(toElement(child)) && toHTMLStyleElement(toElement(child))->isRegisteredAsScoped()) > > count++; > > } > > This looks wrong; you're calling toElement() on every child node, wouldn't this assert? > Maybe we could rewrite this loop using ElementTraversal instead? Then you wouldn't need to worry about Nodes at all. Done. Thx! :)
Andreas Kling
Comment 7 2013-07-02 05:10:06 PDT
Comment on attachment 205893 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=205893&action=review r=me, but... > Source/WebCore/dom/Node.cpp:2539 > for (Node* child = firstChild(); child; child = child->nextSibling()) { > - if (isHTMLStyleElement(child) && toHTMLStyleElement(child)->isRegisteredAsScoped()) > - count++; > + if (child->isElementNode()) { > + Element* element = toElement(child); > + if (isHTMLStyleElement(element) && toHTMLStyleElement(element)->isRegisteredAsScoped()) > + count++; > + } > } You can simplify this loop by using ElementTraversal: for (Element* element = ElementTraversal::firstWithin(this); element; element = ElementTraversal::next(element, this)) { if (isHTMLStyleElement(element) && toHTMLStyleElement(element)->isRegisteredAsScoped()) count++; }
Kangil Han
Comment 8 2013-07-02 05:18:49 PDT
(In reply to comment #7) > > Source/WebCore/dom/Node.cpp:2539 > > for (Node* child = firstChild(); child; child = child->nextSibling()) { > > - if (isHTMLStyleElement(child) && toHTMLStyleElement(child)->isRegisteredAsScoped()) > > - count++; > > + if (child->isElementNode()) { > > + Element* element = toElement(child); > > + if (isHTMLStyleElement(element) && toHTMLStyleElement(element)->isRegisteredAsScoped()) > > + count++; > > + } > > } > > You can simplify this loop by using ElementTraversal: > > for (Element* element = ElementTraversal::firstWithin(this); element; element = ElementTraversal::next(element, this)) { > if (isHTMLStyleElement(element) && toHTMLStyleElement(element)->isRegisteredAsScoped()) > count++; > } Cool! I will try this! :)
Kangil Han
Comment 9 2013-07-02 05:29:48 PDT
Kangil Han
Comment 10 2013-07-02 05:31:45 PDT
(In reply to comment #7) > You can simplify this loop by using ElementTraversal: > > for (Element* element = ElementTraversal::firstWithin(this); element; element = ElementTraversal::next(element, this)) { > if (isHTMLStyleElement(element) && toHTMLStyleElement(element)->isRegisteredAsScoped()) > count++; > } Done! :)
WebKit Commit Bot
Comment 11 2013-07-02 06:48:56 PDT
Comment on attachment 205896 [details] Patch Clearing flags on attachment: 205896 Committed r152290: <http://trac.webkit.org/changeset/152290>
WebKit Commit Bot
Comment 12 2013-07-02 06:48:58 PDT
All reviewed patches have been landed. Closing bug.
Note You need to log in before you can comment on or make changes to this bug.