Get rid of createVisibleSelection() taking PositionWithAffinity

third_party/WebKit/Source/core/page/DragController.cpp

 /*
  * Copyright (C) 2007, 2009, 2010 Apple Inc. All rights reserved.
  * Copyright (C) 2008 Google Inc.
  *
  * 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 438 matching lines @@
   DCHECK(dragData);
   Document* doc = m_page->deprecatedLocalMainFrame()->documentAtPoint(
       dragData->clientPosition());
 
   if (doc &&
       (m_didInitiateDrag || doc->isPluginDocument() || hasEditableStyle(*doc)))
     return DragOperationNone;
   return dragOperation(dragData);
 }
 
+// Returns true if node at |point| is editable with populating |dragCaret| and
+// |range|, otherwise returns false.
+// TODO(yosin): We should return |VisibleSelection| rather than three values.
 static bool setSelectionToDragCaret(LocalFrame* frame,
                                     VisibleSelection& dragCaret,
                                     Range*& range,
                                     const IntPoint& point) {
   frame->selection().setSelection(dragCaret);
   if (frame->selection().isNone()) {
-    // TODO(xiaochengh): The use of updateStyleAndLayoutIgnorePendingStylesheets
+    // TODO(editing-dev): The use of
+    // updateStyleAndLayoutIgnorePendingStylesheets
     // needs to be audited.  See http://crbug.com/590369 for more details.
+    // |LocalFrame::positinForPoint()| requires clean layout.
     frame->document()->updateStyleAndLayoutIgnorePendingStylesheets();
+    const PositionWithAffinity& position = frame->positionForPoint(point);
+    if (!position.isConnected())

[tkent, 2016/10/20 08:05:19]

In the old code, what happened if !frame->positionForPoint(point).isConnected()?
Is the behavior of the old code and the new code identical?

[yosin_UTC9, 2016/10/20 08:34:00]

It makes empty selection since visible position canonicalization converts it to
null position.
On debug build, it hits DCHECK(position.isConnected()) in VisiblePosition
constructor.

[tkent, 2016/10/21 01:06:27]

Because this code is in |if (frame-selection().isNone())| clause, skipping
setSelection() won't change user-visible behavior, right?

[yosin_UTC9, 2016/10/21 01:23:40]

Yes, since we attempt to set selection to none on none selection.

+      return false;
 
-    dragCaret = createVisibleSelection(frame->positionForPoint(point));
-    frame->selection().setSelection(dragCaret);
+    frame->selection().setSelection(
+        SelectionInDOMTree::Builder().collapse(position).build());
+    dragCaret = frame->selection().selection();
     range = createRange(dragCaret.toNormalizedEphemeralRange());
   }
   return !frame->selection().isNone() && frame->selection().isContentEditable();
 }
 
 DispatchEventResult DragController::dispatchTextInputEventFor(
     LocalFrame* innerFrame,
     DragData* dragData) {
   // Layout should be clean due to a hit test performed in |elementUnderMouse|.
   DCHECK(!innerFrame->document()->needsLayoutTreeUpdate());
   DCHECK(m_page->dragCaretController().hasCaret());
   String text = m_page->dragCaretController().isContentRichlyEditable()
                     ? ""
                     : dragData->asPlainText();
-  Element* target = innerFrame->editor().findEventTargetFrom(
-      createVisibleSelection(m_page->dragCaretController().caretPosition()));
+  const PositionWithAffinity& caretPosition =
+      m_page->dragCaretController().caretPosition();
+  DCHECK(caretPosition.isConnected()) << caretPosition;
+  Element* target =
+      innerFrame->editor().findEventTargetFrom(createVisibleSelection(
+          SelectionInDOMTree::Builder().collapse(caretPosition).build()));
   return target->dispatchEvent(
       TextEvent::createForDrop(innerFrame->domWindow(), text));
 }
 
 bool DragController::concludeEditDrag(DragData* dragData) {
   DCHECK(dragData);
 
   HTMLInputElement* fileInput = m_fileInputElementUnderMouse;
   if (m_fileInputElementUnderMouse) {
     m_fileInputElementUnderMouse->setCanReceiveDroppedFiles(false);
@@ skipping 35 matching lines @@
   if (m_page->dragCaretController().hasCaret()) {
     // TODO(xiaochengh): The use of updateStyleAndLayoutIgnorePendingStylesheets
     // needs to be audited.  See http://crbug.com/590369 for more details.
     m_page->dragCaretController()
         .caretPosition()
         .position()
         .document()
         ->updateStyleAndLayoutIgnorePendingStylesheets();
   }
 
-  VisibleSelection dragCaret =
-      createVisibleSelection(m_page->dragCaretController().caretPosition());
+  const PositionWithAffinity& caretPosition =
+      m_page->dragCaretController().caretPosition();
+  if (!caretPosition.isConnected()) {
+    // "editing/pasteboard/drop-text-events-sideeffect-crash.html" and
+    // "editing/pasteboard/drop-text-events-sideeffect.html" reach here.
+    m_page->dragCaretController().clear();
+    return false;
+  }
+  VisibleSelection dragCaret = createVisibleSelection(
+      SelectionInDOMTree::Builder().collapse(caretPosition).build());
   m_page->dragCaretController().clear();
   // |innerFrame| can be removed by event handler called by
   // |dispatchTextInputEventFor()|.
   if (!innerFrame->selection().isAvailable()) {
     // "editing/pasteboard/drop-text-events-sideeffect-crash.html" reaches
     // here.
     return false;
   }
   Range* range = createRange(dragCaret.toNormalizedEphemeralRange());
   Element* rootEditableElement = innerFrame->selection().rootEditableElement();
@@ skipping 633 matching lines @@
 }
 
 DEFINE_TRACE(DragController) {
   visitor->trace(m_page);
   visitor->trace(m_documentUnderMouse);
   visitor->trace(m_dragInitiator);
   visitor->trace(m_fileInputElementUnderMouse);
 }
 
 }  // namespace blink