New issue
Advanced search Search tips
Note: Color blocks (like or ) mean that a user may not be available. Tooltip shows the reason.

Issue 682878 link

Starred by 1 user

Issue metadata

Status: Fixed
Owner:
Closed: May 2017
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: ----
Pri: 3
Type: Bug



Sign in to add a comment

Utilize SynchronousMutationObserver in Blink

Project Member Reported by yosin@chromium.org, Jan 19 2017

Issue description

SynchronousMutationObserver provides DOM mutation observation in C++. Components observes DOM mutation should use this facility.

Current known clients are:
 - Range; since Range relocation should be done before other observers, we'll done Range relocation code to SynchronousMutationNotifier before notifier to other observers.
 - DocumentMarker
 - FrameCaret
 - FrameSelection
 
Project Member

Comment 1 by bugdroid1@chromium.org, Jan 20 2017

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

commit 6fae5081aab1d31d63ac072293056ac5e250469a
Author: yosin <yosin@chromium.org>
Date: Fri Jan 20 00:58:23 2017

Make SynchronousMutationObserver::didMergeTextNodes() to take two Text nodes

This patch makes |SynchronousMutationObserver::didMergeTextNodes()| to take
both merge destination and merge source |Text| node for a preparation of [1]
which uses both |Text| node for relocating selection.

[1] http://crrev.com/2637013002: Make FrameSelection to hold non-canonicalized
positions

BUG= 682878 
TEST=n/a; no behavior changes

Review-Url: https://codereview.chromium.org/2644063002
Cr-Commit-Position: refs/heads/master@{#444904}

[modify] https://crrev.com/6fae5081aab1d31d63ac072293056ac5e250469a/third_party/WebKit/Source/core/dom/Document.cpp
[modify] https://crrev.com/6fae5081aab1d31d63ac072293056ac5e250469a/third_party/WebKit/Source/core/dom/Document.h
[modify] https://crrev.com/6fae5081aab1d31d63ac072293056ac5e250469a/third_party/WebKit/Source/core/dom/DocumentTest.cpp
[modify] https://crrev.com/6fae5081aab1d31d63ac072293056ac5e250469a/third_party/WebKit/Source/core/dom/SynchronousMutationNotifier.cpp
[modify] https://crrev.com/6fae5081aab1d31d63ac072293056ac5e250469a/third_party/WebKit/Source/core/dom/SynchronousMutationNotifier.h
[modify] https://crrev.com/6fae5081aab1d31d63ac072293056ac5e250469a/third_party/WebKit/Source/core/dom/SynchronousMutationObserver.cpp
[modify] https://crrev.com/6fae5081aab1d31d63ac072293056ac5e250469a/third_party/WebKit/Source/core/dom/SynchronousMutationObserver.h
[modify] https://crrev.com/6fae5081aab1d31d63ac072293056ac5e250469a/third_party/WebKit/Source/core/dom/Text.cpp

Project Member

Comment 2 by bugdroid1@chromium.org, Jan 20 2017

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

commit f34f5cb8f4f2f4a54afe4094dc7933193220116e
Author: yosin <yosin@chromium.org>
Date: Fri Jan 20 07:16:51 2017

Introduce SynchronousMutationObserver::didChangeAttribute()

This patch introduces |SynchronousMutationObserver::didChangeAttribute()| to
observe attribute changes in |Element| for a preparation of [1] which tracks
attribute changes for invalidating cached |VisibleSelection|.

[1] http://crrev.com/2637013002: Make FrameSelection to hold non-canonicalized
positions

BUG= 682878 
TEST=run_webkit_unitetests --gtest_filter=DocumentTest.SynchronousMutationNotifierChangeAttribute)

Review-Url: https://codereview.chromium.org/2641803003
Cr-Commit-Position: refs/heads/master@{#445006}

[modify] https://crrev.com/f34f5cb8f4f2f4a54afe4094dc7933193220116e/third_party/WebKit/Source/core/dom/DocumentTest.cpp
[modify] https://crrev.com/f34f5cb8f4f2f4a54afe4094dc7933193220116e/third_party/WebKit/Source/core/dom/Element.cpp
[modify] https://crrev.com/f34f5cb8f4f2f4a54afe4094dc7933193220116e/third_party/WebKit/Source/core/dom/SynchronousMutationNotifier.cpp
[modify] https://crrev.com/f34f5cb8f4f2f4a54afe4094dc7933193220116e/third_party/WebKit/Source/core/dom/SynchronousMutationNotifier.h
[modify] https://crrev.com/f34f5cb8f4f2f4a54afe4094dc7933193220116e/third_party/WebKit/Source/core/dom/SynchronousMutationObserver.cpp
[modify] https://crrev.com/f34f5cb8f4f2f4a54afe4094dc7933193220116e/third_party/WebKit/Source/core/dom/SynchronousMutationObserver.h

Project Member

Comment 3 by bugdroid1@chromium.org, Jan 20 2017

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

commit d9ed982647cfc7940ef9f3eee5bf087a894a6133
Author: yosin <yosin@chromium.org>
Date: Fri Jan 20 08:53:00 2017

Make FrameCaret to utilize SynchronousMutationObserver

This patch makes |FrameCaret| to utilize |SynchronousMutationObserver| to
simplify DOM mutation by removing call from |FrameSelection| for improving code
health and a preparation of [1].

[1] http://crrev.com/2637013002: WIP Make FrameSelection to hold non-
canonicalized positions

BUG= 682878 
TEST=n/a; no behavior changes

Review-Url: https://codereview.chromium.org/2643033005
Cr-Commit-Position: refs/heads/master@{#445019}

[modify] https://crrev.com/d9ed982647cfc7940ef9f3eee5bf087a894a6133/third_party/WebKit/Source/core/editing/FrameCaret.cpp
[modify] https://crrev.com/d9ed982647cfc7940ef9f3eee5bf087a894a6133/third_party/WebKit/Source/core/editing/FrameCaret.h
[modify] https://crrev.com/d9ed982647cfc7940ef9f3eee5bf087a894a6133/third_party/WebKit/Source/core/editing/FrameSelection.cpp

Project Member

Comment 4 by bugdroid1@chromium.org, Jan 20 2017

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

commit cafd47a29528e260f26587c8eab0db161855e851
Author: yosin <yosin@chromium.org>
Date: Fri Jan 20 11:58:35 2017

Make FrameSelection to utilize SynchronousMutationObserver

This patch makes |FrameSelection| to utilize |SynchronousMutationObserver| to
simplify DOM mutation observing by removing call from |Document| for improving
code health and a preparation of [1].

[1] http://crrev.com/2637013002: WIP Make Document to hold non-
canonicalized positions

BUG= 682878 
TEST=n/a; no behavior changes

Review-Url: https://codereview.chromium.org/2638423005
Cr-Commit-Position: refs/heads/master@{#445043}

[modify] https://crrev.com/cafd47a29528e260f26587c8eab0db161855e851/third_party/WebKit/Source/core/dom/CharacterData.cpp
[modify] https://crrev.com/cafd47a29528e260f26587c8eab0db161855e851/third_party/WebKit/Source/core/dom/Document.cpp
[modify] https://crrev.com/cafd47a29528e260f26587c8eab0db161855e851/third_party/WebKit/Source/core/editing/FrameSelection.cpp
[modify] https://crrev.com/cafd47a29528e260f26587c8eab0db161855e851/third_party/WebKit/Source/core/editing/FrameSelection.h

Project Member

Comment 5 by bugdroid1@chromium.org, Jan 25 2017

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

commit eb6e1f0d57a18e16ffa21254bf954610112f858b
Author: yosin <yosin@chromium.org>
Date: Wed Jan 25 03:23:48 2017

Revert of Introduce SynchronousMutationObserver::didChangeAttribute() (patchset #3 id:60001 of https://codereview.chromium.org/2641803003/ )

Reason for revert:
Performance regression on blink_perf.dom: modify-element-title/modify-element-title

http://crbug.com/684394

Original issue's description:
> Introduce SynchronousMutationObserver::didChangeAttribute()
>
> This patch introduces |SynchronousMutationObserver::didChangeAttribute()| to
> observe attribute changes in |Element| for a preparation of [1] which tracks
> attribute changes for invalidating cached |VisibleSelection|.
>
> [1] http://crrev.com/2637013002: Make FrameSelection to hold non-canonicalized
> positions
>
> BUG= 682878 
> TEST=run_webkit_unitetests --gtest_filter=DocumentTest.SynchronousMutationNotifierChangeAttribute)
>
> Review-Url: https://codereview.chromium.org/2641803003
> Cr-Commit-Position: refs/heads/master@{#445006}
> Committed: https://chromium.googlesource.com/chromium/src/+/f34f5cb8f4f2f4a54afe4094dc7933193220116e

TBR=tkent@chromium.org
# Not skipping CQ checks because original CL landed more than 1 days ago.
BUG= 682878 

Review-Url: https://codereview.chromium.org/2650353002
Cr-Commit-Position: refs/heads/master@{#445929}

[modify] https://crrev.com/eb6e1f0d57a18e16ffa21254bf954610112f858b/third_party/WebKit/Source/core/dom/DocumentTest.cpp
[modify] https://crrev.com/eb6e1f0d57a18e16ffa21254bf954610112f858b/third_party/WebKit/Source/core/dom/Element.cpp
[modify] https://crrev.com/eb6e1f0d57a18e16ffa21254bf954610112f858b/third_party/WebKit/Source/core/dom/SynchronousMutationNotifier.cpp
[modify] https://crrev.com/eb6e1f0d57a18e16ffa21254bf954610112f858b/third_party/WebKit/Source/core/dom/SynchronousMutationNotifier.h
[modify] https://crrev.com/eb6e1f0d57a18e16ffa21254bf954610112f858b/third_party/WebKit/Source/core/dom/SynchronousMutationObserver.cpp
[modify] https://crrev.com/eb6e1f0d57a18e16ffa21254bf954610112f858b/third_party/WebKit/Source/core/dom/SynchronousMutationObserver.h

Comment 6 by yosin@chromium.org, Mar 6 2017

Cc: rlanday@chromium.org
Project Member

Comment 7 by bugdroid1@chromium.org, Mar 21 2017

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

commit c8529921548accf5f93751f6f997bebe7e0683bd
Author: rlanday <rlanday@chromium.org>
Date: Tue Mar 21 19:46:09 2017

Refactor DocumentMarkerController to use SynchronousMutationObserver

I had a previous CL up where I made this change combined with some behavioral
changes to DocumentMarkerController::shiftMarker():
https://codereview.chromium.org/2692093003

In this CL, I am only refactoring DMC to use SMO without making any other
behavioral changes. Whereas the other CL did all the marker updates in one pass
through shiftMarkers(), the most straightforward way to avoid introducing any
behavioral changes is to call removeMarkers() on the text being replaced, shift
the markers for the remove, and then shift the markers in the other direction
for the insert.

I will update the marker-shifting behavior to handle text replacement more
gracefully (i.e. not splitting the markers when text is removed, keeping
replaced text at the start/end of the marked range inside the range after
replacement) in a follow-up CL.

BUG= 682878 

Review-Url: https://codereview.chromium.org/2755953004
Cr-Commit-Position: refs/heads/master@{#458518}

[modify] https://crrev.com/c8529921548accf5f93751f6f997bebe7e0683bd/third_party/WebKit/Source/core/dom/CharacterData.cpp
[modify] https://crrev.com/c8529921548accf5f93751f6f997bebe7e0683bd/third_party/WebKit/Source/core/dom/Document.cpp
[modify] https://crrev.com/c8529921548accf5f93751f6f997bebe7e0683bd/third_party/WebKit/Source/core/editing/markers/DocumentMarkerController.cpp
[modify] https://crrev.com/c8529921548accf5f93751f6f997bebe7e0683bd/third_party/WebKit/Source/core/editing/markers/DocumentMarkerController.h

Comment 8 by yosin@chromium.org, May 24 2017

Status: Fixed (was: Started)
We have no candidates for SMO.

Sign in to add a comment