New issue
Advanced search Search tips

Issue 730412 link

Starred by 1 user

Issue metadata

Status: Fixed
Owner:
Closed: Jun 2017
Components:
EstimatedDays: ----
NextAction: ----
OS: ----
Pri: 3
Type: Task



Sign in to add a comment

Cleanup code for insertBefore(), appendChild(), and replaceChild()

Project Member Reported by tkent@chromium.org, Jun 7 2017

Issue description

Code related to ContainerNode::InsertBefore(), AppendChild(), and ReplaceChild() is messy. We should clean it up and restructure it to follow algorithms in the DOM standard.

 
Project Member

Comment 1 by bugdroid1@chromium.org, Jun 8 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/fa36fc2377dd89fb3b0965447f6af26b99875d71

commit fa36fc2377dd89fb3b0965447f6af26b99875d71
Author: Kent Tamura <tkent@chromium.org>
Date: Thu Jun 08 02:07:32 2017

Split ContainerNode::CollectChildrenAndRemoveFromOldParentWithOptionalRecheck().

Split it into:
 - Calling CollectChildrenAndRemoveFromOldParent()
 - Detect extra DOM mutation
 - Re-check
and fold them into callsites.

Also, optimize ReplaceChild() so that it does re-check only once.

Bug:  730412 
Change-Id: I164bcc91a4b50acf128ad19e248e6e76be6295b8
Reviewed-on: https://chromium-review.googlesource.com/526916
Reviewed-by: Hayato Ito <hayato@chromium.org>
Commit-Queue: Kent Tamura <tkent@chromium.org>
Cr-Commit-Position: refs/heads/master@{#477862}
[modify] https://crrev.com/fa36fc2377dd89fb3b0965447f6af26b99875d71/third_party/WebKit/Source/core/dom/ContainerNode.cpp
[modify] https://crrev.com/fa36fc2377dd89fb3b0965447f6af26b99875d71/third_party/WebKit/Source/core/dom/ContainerNode.h

Project Member

Comment 2 by bugdroid1@chromium.org, Jun 8 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/e974155892ab414b593094728883e70b4422a595

commit e974155892ab414b593094728883e70b4422a595
Author: Kent Tamura <tkent@chromium.org>
Date: Thu Jun 08 04:14:25 2017

Rename ContainerNode::CheckAcceptChild() to EnsurePreInsertionValidity().

https://dom.spec.whatwg.org/#concept-node-ensure-pre-insertion-validity

Bug:  730412 
Change-Id: Ia5857b40597006cc47578d057fd10c363e6684c6
Reviewed-on: https://chromium-review.googlesource.com/527835
Reviewed-by: Hayato Ito <hayato@chromium.org>
Commit-Queue: Kent Tamura <tkent@chromium.org>
Cr-Commit-Position: refs/heads/master@{#477883}
[modify] https://crrev.com/e974155892ab414b593094728883e70b4422a595/third_party/WebKit/Source/core/dom/ContainerNode.cpp
[modify] https://crrev.com/e974155892ab414b593094728883e70b4422a595/third_party/WebKit/Source/core/dom/ContainerNode.h

Project Member

Comment 3 by bugdroid1@chromium.org, Jun 9 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/1b1d24823dbc613529ee4f21269aec33ae0dea2a

commit 1b1d24823dbc613529ee4f21269aec33ae0dea2a
Author: Kent Tamura <tkent@chromium.org>
Date: Fri Jun 09 04:26:35 2017

Setting null to caption/tHead IDL attributes should not throw.

Implementations of these setters should do nothing if null is passed.

Also, this CL changes the first argument of EnsurePreInsertionValidity()
to |const Node&|. Binding layer ensures that it's not null.

Bug:  687116 ,  730412 
Change-Id: I34f867458a07139c8e9f949b791ad1567a8f7de9
Reviewed-on: https://chromium-review.googlesource.com/527837
Commit-Queue: Kent Tamura <tkent@chromium.org>
Reviewed-by: Hayato Ito <hayato@chromium.org>
Cr-Commit-Position: refs/heads/master@{#478200}
[delete] https://crrev.com/1ee189bd85e14ad9bedca7946d43b5054fe760af/third_party/WebKit/LayoutTests/external/wpt/html/semantics/tabular-data/the-table-element/caption-methods-expected.txt
[modify] https://crrev.com/1b1d24823dbc613529ee4f21269aec33ae0dea2a/third_party/WebKit/LayoutTests/external/wpt/html/semantics/tabular-data/the-table-element/tHead-expected.txt
[modify] https://crrev.com/1b1d24823dbc613529ee4f21269aec33ae0dea2a/third_party/WebKit/LayoutTests/html/tabular_data/table_exceptions-expected.txt
[modify] https://crrev.com/1b1d24823dbc613529ee4f21269aec33ae0dea2a/third_party/WebKit/LayoutTests/html/tabular_data/table_exceptions.html
[modify] https://crrev.com/1b1d24823dbc613529ee4f21269aec33ae0dea2a/third_party/WebKit/Source/core/dom/ContainerNode.cpp
[modify] https://crrev.com/1b1d24823dbc613529ee4f21269aec33ae0dea2a/third_party/WebKit/Source/core/dom/ContainerNode.h
[modify] https://crrev.com/1b1d24823dbc613529ee4f21269aec33ae0dea2a/third_party/WebKit/Source/core/html/HTMLTableElement.cpp

Project Member

Comment 4 by bugdroid1@chromium.org, Jun 9 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/e1e904a2d015c18729aeca521b79c97d1a6b3600

commit e1e904a2d015c18729aeca521b79c97d1a6b3600
Author: Kent Tamura <tkent@chromium.org>
Date: Fri Jun 09 04:31:26 2017

DOM: Introduce a function to check reference child's parent.

Add CheckReferenceChildParent(), and call it as the step 3 of
EnsurePreInsertionValidity().

Add more step comments.

Bug:  730412 
Change-Id: Ie4170f600938f68e846ae43d80d863089c3064b4
Reviewed-on: https://chromium-review.googlesource.com/527840
Reviewed-by: Hayato Ito <hayato@chromium.org>
Commit-Queue: Kent Tamura <tkent@chromium.org>
Cr-Commit-Position: refs/heads/master@{#478201}
[modify] https://crrev.com/e1e904a2d015c18729aeca521b79c97d1a6b3600/third_party/WebKit/Source/core/dom/ContainerNode.cpp

Project Member

Comment 5 by bugdroid1@chromium.org, Jun 9 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/e72f7b8defe19a8b79897902ef0a9b8898099c51

commit e72f7b8defe19a8b79897902ef0a9b8898099c51
Author: Kent Tamura <tkent@chromium.org>
Date: Fri Jun 09 08:32:50 2017

Cleanup of code around ContainerNode::ContainsConsideringHostElements().

- Add ExceptionState argument.
  When the function returns true, we always need to throw an exception.

- CheckAcceptChildGuaranteedNodeTypes() uses ContainsConsideringHostElements().
  Also, an optimization in this function is unnecessary. Node::contains()
  has it.

Bug:  730412 
Change-Id: I9a76abed010ab6b397b59c060f96c0deb8028e5d
Reviewed-on: https://chromium-review.googlesource.com/527866
Reviewed-by: Hayato Ito <hayato@chromium.org>
Commit-Queue: Kent Tamura <tkent@chromium.org>
Cr-Commit-Position: refs/heads/master@{#478237}
[modify] https://crrev.com/e72f7b8defe19a8b79897902ef0a9b8898099c51/third_party/WebKit/Source/core/dom/ContainerNode.cpp
[modify] https://crrev.com/e72f7b8defe19a8b79897902ef0a9b8898099c51/third_party/WebKit/Source/core/dom/ContainerNode.h

Project Member

Comment 6 by bugdroid1@chromium.org, Jun 9 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/684293cc2bd184466b67bc85a7adcfedf25da064

commit 684293cc2bd184466b67bc85a7adcfedf25da064
Author: Kent Tamura <tkent@chromium.org>
Date: Fri Jun 09 10:57:35 2017

Fold some parts of ContainerNode::CheckAcceptChildGuaranteedNodeTypes() into RecheckNodeInsertionStructuralPrereq().

CheckAcceptChildGuaranteedNodeTypes() has some unnecessary checks for
RecheckNodeInsertionStructuralPrereq().

Bug:  730412 
Change-Id: Idf3df5a3b6b30f887d6b3ac255b6c6cc5befb406
Reviewed-on: https://chromium-review.googlesource.com/528727
Reviewed-by: Hayato Ito <hayato@chromium.org>
Commit-Queue: Kent Tamura <tkent@chromium.org>
Cr-Commit-Position: refs/heads/master@{#478245}
[modify] https://crrev.com/684293cc2bd184466b67bc85a7adcfedf25da064/third_party/WebKit/Source/core/dom/ContainerNode.cpp

Project Member

Comment 7 by bugdroid1@chromium.org, Jun 12 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/6268b94535ffc078ccab12768344ba2735190191

commit 6268b94535ffc078ccab12768344ba2735190191
Author: Kent Tamura <tkent@chromium.org>
Date: Mon Jun 12 03:08:49 2017

Merge ContainerNode::CheckAcceptChildGuaranteedNodeTypes() into EnsurePreInsertionValidity().

EnsurePreInsertionValidity() was the only callsite of
CheckAcceptChildGuaranteedNodeTypes().

Bug:  730412 
Change-Id: I3f623f6f2511c8634f3832739ac064975547a7d9
Reviewed-on: https://chromium-review.googlesource.com/530606
Reviewed-by: Hayato Ito <hayato@chromium.org>
Commit-Queue: Kent Tamura <tkent@chromium.org>
Cr-Commit-Position: refs/heads/master@{#478542}
[modify] https://crrev.com/6268b94535ffc078ccab12768344ba2735190191/third_party/WebKit/Source/core/dom/ContainerNode.cpp
[modify] https://crrev.com/6268b94535ffc078ccab12768344ba2735190191/third_party/WebKit/Source/core/dom/ContainerNode.h

Project Member

Comment 8 by bugdroid1@chromium.org, Jun 12 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/91aff5625b214afb9005b0dac0d42a218359e51a

commit 91aff5625b214afb9005b0dac0d42a218359e51a
Author: Kent Tamura <tkent@chromium.org>
Date: Mon Jun 12 08:15:01 2017

Code cleanup: Reorder code in ContainerNode::InsertBefore().

so that it matches to the DOM specification more.
This CL has no behavior changes.

Bug:  730412 
Change-Id: I8881f2286e098238e255553bea8d2e1ab8b13ca4
Reviewed-on: https://chromium-review.googlesource.com/530924
Reviewed-by: Hayato Ito <hayato@chromium.org>
Commit-Queue: Kent Tamura <tkent@chromium.org>
Cr-Commit-Position: refs/heads/master@{#478563}
[modify] https://crrev.com/91aff5625b214afb9005b0dac0d42a218359e51a/third_party/WebKit/Source/core/dom/ContainerNode.cpp

Project Member

Comment 9 by bugdroid1@chromium.org, Jun 13 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/16c3b926d3b231e7fe37f4c33e7235c55f209f92

commit 16c3b926d3b231e7fe37f4c33e7235c55f209f92
Author: Kent Tamura <tkent@chromium.org>
Date: Tue Jun 13 11:14:17 2017

Rename ContainerNode::ContainsConsideringHostElements() to IsHostIncludingInclusiveAncestorOfThis().

The new name matches to what this operation is called in the standard.
https://dom.spec.whatwg.org/#concept-tree-host-including-inclusive-ancestor

Bug:  730412 
Change-Id: I3e78ae884d7f9aadb3693a073f83211aef35c351
Reviewed-on: https://chromium-review.googlesource.com/532796
Reviewed-by: Hayato Ito <hayato@chromium.org>
Commit-Queue: Kent Tamura <tkent@chromium.org>
Cr-Commit-Position: refs/heads/master@{#478963}
[modify] https://crrev.com/16c3b926d3b231e7fe37f4c33e7235c55f209f92/third_party/WebKit/Source/core/dom/ContainerNode.cpp
[modify] https://crrev.com/16c3b926d3b231e7fe37f4c33e7235c55f209f92/third_party/WebKit/Source/core/dom/ContainerNode.h

Comment 10 by tkent@chromium.org, Jun 14 2017

Status: Fixed (was: Assigned)

Sign in to add a comment