Bug 60818

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:

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

Julien Chaffraix

Reported 2011-05-13 16:46:09 PDT

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

Attachments
Proposed fix: Refactor XMLDocumentParser to remove the calls to deprecatedParserAddChild (14.13 KB, patch) 2011-05-13 16:50 PDT, Julien Chaffraix	no flags	Details Formatted Diff Diff
Updated patch with proper style. (14.98 KB, patch) 2011-05-13 17:00 PDT, Julien Chaffraix	eric: review-	Details Formatted Diff Diff
Updated after Eric's and Adam's review (14.39 KB, patch) 2011-05-18 17:33 PDT, Julien Chaffraix	no flags	Details Formatted Diff Diff
New patch - Removed the change in TextControlInnerElements and kept the deprecated methods as we still have one caller (10.85 KB, patch) 2011-05-19 15:31 PDT, Julien Chaffraix	no flags	Details Formatted Diff Diff
Final patch: Remove deprecatedParserAddChild (3.00 KB, patch) 2011-05-23 16:32 PDT, Julien Chaffraix	no flags	Details Formatted Diff Diff
Show Obsolete (4) View All Add attachment proposed patch, testcase, etc.

Julien Chaffraix

Comment 1 2011-05-13 16:50:19 PDT

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

WebKit Review Bot

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

Julien Chaffraix

Comment 3 2011-05-13 17:00:04 PDT

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

Eric Seidel (no email)

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

Adam Barth

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

Eric Seidel (no email)

Comment 6 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?

Eric Seidel (no email)

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

Julien Chaffraix

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

Adam Barth

Comment 9 2011-05-16 17:47:22 PDT

n_leafNode ?

Julien Chaffraix

Comment 10 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?

Julien Chaffraix

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

Adam Barth

Comment 12 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?

Eric Seidel (no email)

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

Eric Seidel (no email)

Comment 14 2011-05-19 05:37:41 PDT

I thought mutation events were async these days anyway?

Ryosuke Niwa

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

Adam Barth

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

Julien Chaffraix

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

Eric Seidel (no email)

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

Adam Barth

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

Julien Chaffraix

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

Julien Chaffraix

Comment 21 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

Adam Barth

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

WebKit Commit Bot

Comment 23 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>

WebKit Commit Bot

Comment 24 2011-05-19 19:39:40 PDT

All reviewed patches have been landed. Closing bug.

Julien Chaffraix

Comment 25 2011-05-19 20:05:40 PDT

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

Julien Chaffraix

Comment 26 2011-05-23 16:32:20 PDT

Created attachment 94516 [details] Final patch: Remove deprecatedParserAddChild

Adam Barth

Comment 27 2011-05-23 16:47:44 PDT

Comment on attachment 94516 [details] Final patch: Remove deprecatedParserAddChild Prettiest patch all day!

WebKit Commit Bot

Comment 28 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>

WebKit Commit Bot

Comment 29 2011-05-23 23:07:48 PDT

All reviewed patches have been landed. Closing bug.

Lucas Forschler

Comment 30 2019-02-06 09:03:39 PST

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

Note You need to log in before you can comment on or make changes to this bug.