Bug 60818 – Remove Node::deprecatedParserAddChild

• Status: RESOLVED
• Resolution: FIXED
• Component: DOM

[Julien Chaffraix]

As the name says it, let's remove this method that is left from previous refactorings.

[Julien Chaffraix]

Created attachment 93531 [details]
Proposed fix: Refactor XMLDocumentParser to remove the calls to deprecatedParserAddChild

[WebKit Review Bot]

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.

[Julien Chaffraix]

Created attachment 93533 [details]
Updated patch with proper style.

[Eric Seidel (no email)]

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.

[Adam Barth]

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.

[Eric Seidel (no email)]

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?

[Eric Seidel (no email)]

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.

[Julien Chaffraix]

(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.

[Adam Barth]

n_leafNode ?

[Julien Chaffraix]

(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?

[Julien Chaffraix]

Created attachment 94009 [details]
Updated after Eric's and Adam's review

Eric, if you don't like the new naming, just say it!

[Adam Barth]

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?

[Eric Seidel (no email)]

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.

[Eric Seidel (no email)]

I thought mutation events were async these days anyway?

[Ryosuke Niwa]

(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.

[Adam Barth]

IMHO, we should do the rest of this patch and worry about that last callsite in another bug.

[Julien Chaffraix]

(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!

[Eric Seidel (no email)]

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.

[Adam Barth]

> 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!

[Julien Chaffraix]

> 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.

[Julien Chaffraix]

Created attachment 94131 [details]
New patch - Removed the change in TextControlInnerElements and kept the deprecated methods as we still have one caller

[Adam Barth]

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!

[WebKit Commit Bot]

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>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Julien Chaffraix]

Re-opening and adding the dependency on bug 54179. Thanks for the reviews Eric and Adam.

[Julien Chaffraix]

Created attachment 94516 [details]
Final patch: Remove deprecatedParserAddChild

[Adam Barth]

Comment on attachment 94516 [details]
Final patch: Remove deprecatedParserAddChild

Prettiest patch all day!

[WebKit Commit Bot]

Comment on attachment 94516 [details]
Final patch: Remove deprecatedParserAddChild

Clearing flags on attachment: 94516

Committed r87128: <http://trac.webkit.org/changeset/87128>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Lucas Forschler]

Mass moving XML DOM bugs to the "DOM" Component.