Prune layout update calls from Editor::*appliedEditing

third_party/WebKit/Source/core/editing/Editor.cpp

Index: third_party/WebKit/Source/core/editing/Editor.cpp
diff --git a/third_party/WebKit/Source/core/editing/Editor.cpp b/third_party/WebKit/Source/core/editing/Editor.cpp
index 5d0c85686a345b6edbb41d7c3b46bb40eaa3ee0f..3644833bb635cd8257e69da2971feacb80778056 100644
--- a/third_party/WebKit/Source/core/editing/Editor.cpp
+++ b/third_party/WebKit/Source/core/editing/Editor.cpp
@@ -883,13 +883,15 @@ static void dispatchEditableContentChangedEvents(Element* startRoot,
         Event::create(EventTypeNames::webkitEditableContentChanged));
 }
 
-static VisibleSelection correctedVisibleSelection(
-    const VisibleSelection& passedSelection) {
+static SelectionInDOMTree correctedSelectionAfterCommand(
+    const VisibleSelection& passedSelection,

[yoichio, 2017/03/21 06:41:57]

Could you use SelectionINDOMTree& passedSelection instead of VS?

[Xiaocheng, 2017/03/21 20:53:39]

It doesn't seem to introduce any benefit, since EditCommand::endingSelection()
is a VS.

Anyway it's not important. I can change if you insist.

[yoichio, 2017/03/22 01:48:57]

Yes, no benefit from a view of semantics.
However, we're going to reduce VS usage in code.
Eventually EditCommand::endingSelection() will turn Selection.

[Xiaocheng, 2017/03/22 02:12:44]

Sorry, I just realized that we can't change it in this patch.

VisibleSelection::asSelection() will hit DCHECK if the VS is ill-formed, which
means we need to check validity of the VS before calling VS::asSelection(). This
function does exactly the job.

+    Document* document) {
   if (!passedSelection.base().isConnected() ||
-      !passedSelection.extent().isConnected())
-    return VisibleSelection();
-  DCHECK(!passedSelection.base().document()->needsLayoutTreeUpdate());
-  return createVisibleSelection(passedSelection.asSelection());
+      !passedSelection.extent().isConnected() ||
+      passedSelection.base().document() != document ||
+      passedSelection.base().document() != passedSelection.extent().document())
+    return SelectionInDOMTree();
+  return passedSelection.asSelection();
 }
 
 void Editor::appliedEditing(CompositeEditCommand* cmd) {
@@ -909,19 +911,12 @@ void Editor::appliedEditing(CompositeEditCommand* cmd) {
       undoStep->endingRootEditableElement(), cmd->inputType(),
       cmd->textDataForInputEvent(), isComposingFromCommand(cmd));
 
-  // TODO(editing-dev): The use of updateStyleAndLayoutIgnorePendingStylesheets
-  // needs to be audited.  See http://crbug.com/590369 for more details.
-  // The clean layout is consumed by |mostBackwardCaretPosition|, called through
-  // |changeSelectionAfterCommand|. In the long term, we should postpone visible
-  // selection canonicalization so that selection update does not need layout.
-  frame().document()->updateStyleAndLayoutIgnorePendingStylesheets();
-
-  const VisibleSelection& newSelection =
-      correctedVisibleSelection(cmd->endingSelection());
+  const SelectionInDOMTree& newSelection = correctedSelectionAfterCommand(
+      cmd->endingSelection(), frame().document());
 
   // Don't clear the typing style with this selection change. We do those things
   // elsewhere if necessary.
-  changeSelectionAfterCommand(newSelection.asSelection(), 0);
+  changeSelectionAfterCommand(newSelection, 0);
 
   if (!cmd->preservesTypingStyle())
     clearTypingStyle();
@@ -943,7 +938,7 @@ void Editor::appliedEditing(CompositeEditCommand* cmd) {
     m_undoStack->registerUndoStep(m_lastEditCommand->ensureUndoStep());
   }
 
-  respondToChangedContents(newSelection.start());
+  respondToChangedContents(newSelection.base());
 }
 
 void Editor::unappliedEditing(UndoStep* cmd) {
@@ -956,22 +951,15 @@ void Editor::unappliedEditing(UndoStep* cmd) {
       InputEvent::InputType::HistoryUndo, nullAtom,
       InputEvent::EventIsComposing::NotComposing);
 
-  // TODO(editing-dev): The use of updateStyleAndLayoutIgnorePendingStylesheets
-  // needs to be audited.  See http://crbug.com/590369 for more details.
-  // In the long term, we should stop editing commands from storing
-  // VisibleSelections as starting and ending selections.
-  frame().document()->updateStyleAndLayoutIgnorePendingStylesheets();
-
-  const VisibleSelection& newSelection =
-      correctedVisibleSelection(cmd->startingSelection());
-  DCHECK(newSelection.isValidFor(*frame().document())) << newSelection;
+  const SelectionInDOMTree& newSelection = correctedSelectionAfterCommand(
+      cmd->startingSelection(), frame().document());
   changeSelectionAfterCommand(
-      newSelection.asSelection(),
+      newSelection,
       FrameSelection::CloseTyping | FrameSelection::ClearTypingStyle);
 
   m_lastEditCommand = nullptr;
   m_undoStack->registerRedoStep(cmd);
-  respondToChangedContents(newSelection.start());
+  respondToChangedContents(newSelection.base());
 }
 
 void Editor::reappliedEditing(UndoStep* cmd) {
@@ -984,21 +972,15 @@ void Editor::reappliedEditing(UndoStep* cmd) {
       InputEvent::InputType::HistoryRedo, nullAtom,
       InputEvent::EventIsComposing::NotComposing);
 
-  // TODO(editing-dev): The use of updateStyleAndLayoutIgnorePendingStylesheets
-  // needs to be audited.  See http://crbug.com/590369 for more details.
-  // In the long term, we should stop editing commands from storing
-  // VisibleSelections as starting and ending selections.
-  frame().document()->updateStyleAndLayoutIgnorePendingStylesheets();
-  const VisibleSelection& newSelection =
-      correctedVisibleSelection(cmd->endingSelection());
-  DCHECK(newSelection.isValidFor(*frame().document())) << newSelection;
+  const SelectionInDOMTree& newSelection = correctedSelectionAfterCommand(
+      cmd->endingSelection(), frame().document());
   changeSelectionAfterCommand(
-      newSelection.asSelection(),
+      newSelection,
       FrameSelection::CloseTyping | FrameSelection::ClearTypingStyle);
 
   m_lastEditCommand = nullptr;
   m_undoStack->registerUndoStep(cmd);
-  respondToChangedContents(newSelection.start());
+  respondToChangedContents(newSelection.base());
 }
 
 Editor* Editor::create(LocalFrame& frame) {