Bug 60818

Summary: Remove Node::deprecatedParserAddChild
Product: WebKit Reporter: Julien Chaffraix <jchaffraix>
Component: DOMAssignee: 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 Flags
Proposed fix: Refactor XMLDocumentParser to remove the calls to deprecatedParserAddChild
none
Updated patch with proper style.
eric: review-
Updated after Eric's and Adam's review
none
New patch - Removed the change in TextControlInnerElements and kept the deprecated methods as we still have one caller
none
Final patch: Remove deprecatedParserAddChild none

Description Julien Chaffraix 2011-05-13 16:46:09 PDT
As the name says it, let's remove this method that is left from previous refactorings.
Comment 1 Julien Chaffraix 2011-05-13 16:50:19 PDT
Created attachment 93531 [details]
Proposed fix: Refactor XMLDocumentParser to remove the calls to deprecatedParserAddChild
Comment 2 WebKit Review Bot 2011-05-13 16:53:20 PDT
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.
Comment 3 Julien Chaffraix 2011-05-13 17:00:04 PDT
Created attachment 93533 [details]
Updated patch with proper style.
Comment 4 Eric Seidel (no email) 2011-05-16 11:36:19 PDT
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 5 Adam Barth 2011-05-16 11:37:26 PDT
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 6 Eric Seidel (no email) 2011-05-16 11:37:47 PDT
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 7 Eric Seidel (no email) 2011-05-16 11:39:24 PDT
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.
Comment 8 Julien Chaffraix 2011-05-16 17:20:27 PDT
(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.
Comment 9 Adam Barth 2011-05-16 17:47:22 PDT
n_leafNode ?
Comment 10 Julien Chaffraix 2011-05-16 19:58:26 PDT
(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?
Comment 11 Julien Chaffraix 2011-05-18 17:33:05 PDT
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 12 Adam Barth 2011-05-18 18:12:22 PDT
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?
Comment 13 Eric Seidel (no email) 2011-05-19 05:37:16 PDT
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.
Comment 14 Eric Seidel (no email) 2011-05-19 05:37:41 PDT
I thought mutation events were async these days anyway?
Comment 15 Ryosuke Niwa 2011-05-19 08:16:00 PDT
(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.
Comment 16 Adam Barth 2011-05-19 09:34:07 PDT
IMHO, we should do the rest of this patch and worry about that last callsite in another bug.
Comment 17 Julien Chaffraix 2011-05-19 09:39:55 PDT
(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!
Comment 18 Eric Seidel (no email) 2011-05-19 09:45:20 PDT
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.
Comment 19 Adam Barth 2011-05-19 11:15:17 PDT
> 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!
Comment 20 Julien Chaffraix 2011-05-19 12:35:04 PDT
> 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.
Comment 21 Julien Chaffraix 2011-05-19 15:31:40 PDT
Created attachment 94131 [details]
New patch - Removed the change in TextControlInnerElements and kept the deprecated methods as we still have one caller
Comment 22 Adam Barth 2011-05-19 17:37:10 PDT
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 23 WebKit Commit Bot 2011-05-19 19:39:33 PDT
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>
Comment 24 WebKit Commit Bot 2011-05-19 19:39:40 PDT
All reviewed patches have been landed.  Closing bug.
Comment 25 Julien Chaffraix 2011-05-19 20:05:40 PDT
Re-opening and adding the dependency on bug 54179. Thanks for the reviews Eric and Adam.
Comment 26 Julien Chaffraix 2011-05-23 16:32:20 PDT
Created attachment 94516 [details]
Final patch: Remove deprecatedParserAddChild
Comment 27 Adam Barth 2011-05-23 16:47:44 PDT
Comment on attachment 94516 [details]
Final patch: Remove deprecatedParserAddChild

Prettiest patch all day!
Comment 28 WebKit Commit Bot 2011-05-23 23:07:40 PDT
Comment on attachment 94516 [details]
Final patch: Remove deprecatedParserAddChild

Clearing flags on attachment: 94516

Committed r87128: <http://trac.webkit.org/changeset/87128>
Comment 29 WebKit Commit Bot 2011-05-23 23:07:48 PDT
All reviewed patches have been landed.  Closing bug.
Comment 30 Lucas Forschler 2019-02-06 09:03:39 PST
Mass moving XML DOM bugs to the "DOM" Component.