Audit the use of updateStyleAndLayoutIgnorePendingStylesheets in InputMethodController::setSelectio…

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

 /*
  * Copyright (C) 2006, 2007, 2008, 2011 Apple Inc. All rights reserved.
  * Copyright (C) 2008 Nokia Corporation and/or its subsidiary(-ies)
  *
  * Redistribution and use in source and binary forms, with or without
  * modification, are permitted provided that the following conditions
  * are met:
  * 1. Redistributions of source code must retain the above copyright
  *    notice, this list of conditions and the following disclaimer.
  * 2. Redistributions in binary form must reproduce the above copyright
@@ skipping 237 matching lines @@
 
     if (text.length()) {
         confirmComposition(text);
         return true;
     }
 
     if (confirmBehavior == DoNotKeepSelection)
         return confirmComposition(composingText(), DoNotKeepSelection);
 
     SelectionOffsetsScope selectionOffsetsScope(this);
-    return confirmComposition();
+    bool result = confirmComposition();
+
+    // TODO(xiaochengh): The use of updateStyleAndLayoutIgnorePendingStylesheets
+    // needs to be audited. see http://crbug.com/590369 for more details.
+    frame().document()->updateStyleAndLayoutIgnorePendingStylesheets();

[yosin_UTC9, 2016/09/08 04:04:41]

This is the first pattern which updates layout after calling mutator.
Could you explain why do we do this?
I think we should avoid this pattern. 
Caller of |confirmCompositionOrInsertText()| should update layout.

[Xiaocheng, 2016/09/08 06:39:09]

It is in fact the dtor of the SelectionOffsetsScope that requires clean layout.
Is there anything we can do in this case?

+
+    return result;
 }
 
 void InputMethodController::cancelComposition()
 {
     if (!hasComposition())
         return;
 
     Editor::RevealSelectionScope revealSelectionScope(&editor());
 
     if (frame().selection().isNone())
@@ skipping 78 matching lines @@
     //    !hasComposition() && test.isEmpty().
     if (text.isEmpty()) {
         if (hasComposition()) {
             confirmComposition(emptyString());
         } else {
             // It's weird to call |setComposition()| with empty text outside composition, however some IME
             // (e.g. Japanese IBus-Anthy) did this, so we simply delete selection without sending extra events.
             TypingCommand::deleteSelection(*frame().document(), TypingCommand::PreventSpellChecking);
         }
 
+        // TODO(xiaochengh): The use of updateStyleAndLayoutIgnorePendingStylesheets
+        // needs to be audited. see http://crbug.com/590369 for more details.
+        frame().document()->updateStyleAndLayoutIgnorePendingStylesheets();
+
         setEditableSelectionOffsets(selectedRange);
         return;
     }
 
     // We should send a 'compositionstart' event only when the given text is not empty because this
     // function doesn't create a composition node when the text is empty.
     if (!hasComposition()) {
         target->dispatchEvent(CompositionEvent::create(EventTypeNames::compositionstart, frame().domWindow(), frame().selectedText()));
         if (!frame().document())
             return;
@@ skipping 31 matching lines @@
     m_isDirty = true;
     m_hasComposition = true;
     if (!m_compositionRange)
         m_compositionRange = Range::create(baseNode->document());
     m_compositionRange->setStart(baseNode, baseOffset);
     m_compositionRange->setEnd(baseNode, extentOffset);
 
     if (baseNode->layoutObject())
         baseNode->layoutObject()->setShouldDoFullPaintInvalidation();
 
+    // TODO(xiaochengh): The use of updateStyleAndLayoutIgnorePendingStylesheets
+    // needs to be audited. see http://crbug.com/590369 for more details.
+    frame().document()->updateStyleAndLayoutIgnorePendingStylesheets();
+
     // We shouldn't close typing in the middle of setComposition.
     setEditableSelectionOffsets(selectedRange, NotUserTriggered);
 
     if (underlines.isEmpty()) {
         frame().document()->markers().addCompositionMarker(m_compositionRange->startPosition(), m_compositionRange->endPosition(), Color::black, false, LayoutTheme::theme().platformDefaultCompositionBackgroundColor());
         return;
     }
     for (const auto& underline : underlines) {
         unsigned underlineStart = baseOffset + underline.startOffset;
         unsigned underlineEnd = baseOffset + underline.endOffset;
@@ skipping 73 matching lines @@
 }
 
 bool InputMethodController::setSelectionOffsets(const PlainTextRange& selectionOffsets, FrameSelection::SetSelectionOptions options)
 {
     if (selectionOffsets.isNull())
         return false;
     Element* rootEditableElement = frame().selection().rootEditableElement();
     if (!rootEditableElement)
         return false;
 
-    // TODO(dglazkov): The use of updateStyleAndLayoutIgnorePendingStylesheets needs to be audited.
-    // see http://crbug.com/590369 for more details.
-    rootEditableElement->document().updateStyleAndLayoutIgnorePendingStylesheets();
+    DCHECK(!rootEditableElement->document().needsLayoutTreeUpdate());
 
     const EphemeralRange range = selectionOffsets.createRange(*rootEditableElement);
     if (range.isNull())
         return false;
 
     return frame().selection().setSelectedRange(range, VP_DEFAULT_AFFINITY, SelectionDirectionalMode::NonDirectional, options);
 }
 
 bool InputMethodController::setEditableSelectionOffsets(const PlainTextRange& selectionOffsets, FrameSelection::SetSelectionOptions options)
 {
@@ skipping 66 matching lines @@
     TypingCommand::deleteSelection(*frame().document());
 }
 
 DEFINE_TRACE(InputMethodController)
 {
     visitor->trace(m_frame);
     visitor->trace(m_compositionRange);
 }
 
 } // namespace blink