Summary: | Remove Node::deprecatedParserAddChild | ||
---|---|---|---|
Product: | WebKit | Reporter: | Julien Chaffraix <jchaffraix> |
Component: | DOM | Assignee: | Julien Chaffraix <jchaffraix> |
Status: | RESOLVED FIXED | ||
Severity: | Normal | CC: | abarth, ap, cdumez, commit-queue, eric, rniwa, webkit.review.bot |
Priority: | P2 | ||
Version: | 528+ (Nightly build) | ||
Hardware: | All | ||
OS: | All | ||
Bug Depends on: | 54179 | ||
Bug Blocks: | |||
Attachments: |
Description
Julien Chaffraix
2011-05-13 16:46:09 PDT
Created attachment 93531 [details]
Proposed fix: Refactor XMLDocumentParser to remove the calls to deprecatedParserAddChild
Attachment 93531 [details] did not pass style-queue:
Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCor..." exit_code: 1
Source/WebCore/dom/XMLDocumentParser.h:46: Code inside a namespace should not be indented. [whitespace/indent] [4]
Total errors found: 1 in 6 files
If any of these errors are false positives, please file a bug against check-webkit-style.
Created attachment 93533 [details]
Updated patch with proper style.
Comment on attachment 93533 [details] Updated patch with proper style. View in context: https://bugs.webkit.org/attachment.cgi?id=93533&action=review > Source/WebCore/html/shadow/TextControlInnerElements.cpp:81 > + toContainerNode(parent)->parserAddChild(this); We should just change this to use appendChild. Comment on attachment 93533 [details] Updated patch with proper style. View in context: https://bugs.webkit.org/attachment.cgi?id=93533&action=review > Source/WebCore/html/shadow/TextControlInnerElements.cpp:81 > // FIXME: This code seems very wrong. Why are we magically adding |this| to the DOM here? > // We shouldn't be calling parser API methods outside of the parser! > - parent->deprecatedParserAddChild(this); > + toContainerNode(parent)->parserAddChild(this); I don't think this part of the change is correct. TextControlInnerElements is not part of the parser and therefore should not be calling "parser" methods like parserAddChild. The rest looks pretty good though. Comment on attachment 93533 [details] Updated patch with proper style. View in context: https://bugs.webkit.org/attachment.cgi?id=93533&action=review > Source/WebCore/dom/XMLDocumentParser.h:193 > + RefPtr<Text> m_terminalTextNode; I'm not sure I would call this "terminalTextNode". Maybe m_currentTextNode? Or m_openTextNode? Does "terminal text node" come from some spec somewhere? Comment on attachment 93533 [details] Updated patch with proper style. View in context: https://bugs.webkit.org/attachment.cgi?id=93533&action=review > Source/WebCore/dom/XMLDocumentParser.h:191 > + ContainerNode* m_currentNode; > + Vector<ContainerNode*> m_currentNodeStack; We should think about abstracting some of this out into an XMLConstructionSite like how we have an HTMLConstructionSite (in a later patch). The construction site model allows us to in the future consider doing parsing on a separate thread, and also just generally helps segment off the logic from this monolithic class. (In reply to comment #6) > (From update of attachment 93533 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=93533&action=review > > > Source/WebCore/dom/XMLDocumentParser.h:193 > > + RefPtr<Text> m_terminalTextNode; > > I'm not sure I would call this "terminalTextNode". Maybe m_currentTextNode? Or m_openTextNode? Does "terminal text node" come from some spec somewhere? The naming does not come from any specification. I found that it captures the essence of what this is better: your DOM tree leaves are Text node. I thought about m_currentTextNode but I found it too generic. I don't have better alternative thus I guess m_currentTextNode will do. n_leafNode ? (In reply to comment #9) > n_leafNode ? m_leafTextNode is more precise as we only cover this type of leaf nodes. Any of those 2 options would be very fine by me (actually a terminal node is a synonym of a leaf node). Eric, thoughts on that? Created attachment 94009 [details]
Updated after Eric's and Adam's review
Eric, if you don't like the new naming, just say it!
Comment on attachment 94009 [details] Updated after Eric's and Adam's review View in context: https://bugs.webkit.org/attachment.cgi?id=94009&action=review > Source/WebCore/html/shadow/TextControlInnerElements.cpp:82 > + toContainerNode(parent)->appendChild(this, ec); > + ASSERT(!ec); Can this line of code run arbitrary JavaScript? If so, will we end up with memory corruption because |renderer| or |this| can be freed? Either we'll have to make it resilient to DOM modifications, or continue cheating with the parser API. I'm not really familiar with this callsite. I thought mutation events were async these days anyway? (In reply to comment #14) > I thought mutation events were async these days anyway? Only during editing commands. We can slowly migrate to the world where this is true everywhere by declaring EventQueueScope. IMHO, we should do the rest of this patch and worry about that last callsite in another bug. (In reply to comment #16) > IMHO, we should do the rest of this patch and worry about that last callsite in another bug. I don't think this can cause problems as the mutation event is dispatched in the shadow tree. However this is also my opinion that this change does not belong to this patch but *you guys* asked this to be rolled in (and specifically marked the bug r- because of that), not me! It doesn't make sense to chagne from deprecatedParserAppendChild to parserAppendChild for a callsite which shouldn't call it in the first place. :) So we should fix the callsite first, before we remove deprecatedparserApendChild, IMO. > I don't think this can cause problems as the mutation event is dispatched in the shadow tree. However this is also my opinion that this change does not belong to this patch but *you guys* asked this to be rolled in (and specifically marked the bug r- because of that), not me!
Sorry for the back-and-forth. 99% of this patch is great. It's neither correct to remove the "deprecated" or to change to "appendChild", but for different reasons. That code probably needs a more substantial change to be correct. If we get down to one tricky instance of deprecatedParserAddChild in this patch, that's great!
> Sorry for the back-and-forth. 99% of this patch is great. It's neither correct to remove the "deprecated" or to change to "appendChild", but for different reasons. That code probably needs a more substantial change to be correct. If we get down to one tricky instance of deprecatedParserAddChild in this patch, that's great! No worries, I understand all the comments and the fact that we should try to nix this call. I had a look at that today and it looks like the overhaul of how we attach the shadow tree for <input> as part of bug 54179 will solve our problem. The change is actively worked on so I don't think I should try another way to solve this problem. I will thus just scale down and keep this last call around until the other bug is solved. Created attachment 94131 [details]
New patch - Removed the change in TextControlInnerElements and kept the deprecated methods as we still have one caller
Comment on attachment 94131 [details]
New patch - Removed the change in TextControlInnerElements and kept the deprecated methods as we still have one caller
This looks great. Thanks!
Comment on attachment 94131 [details] New patch - Removed the change in TextControlInnerElements and kept the deprecated methods as we still have one caller Clearing flags on attachment: 94131 Committed r86921: <http://trac.webkit.org/changeset/86921> All reviewed patches have been landed. Closing bug. Re-opening and adding the dependency on bug 54179. Thanks for the reviews Eric and Adam. Created attachment 94516 [details]
Final patch: Remove deprecatedParserAddChild
Comment on attachment 94516 [details]
Final patch: Remove deprecatedParserAddChild
Prettiest patch all day!
Comment on attachment 94516 [details] Final patch: Remove deprecatedParserAddChild Clearing flags on attachment: 94516 Committed r87128: <http://trac.webkit.org/changeset/87128> All reviewed patches have been landed. Closing bug. Mass moving XML DOM bugs to the "DOM" Component. |