Select All present even all selectable text has been selected

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

Index: third_party/WebKit/Source/core/editing/FrameSelection.cpp
diff --git a/third_party/WebKit/Source/core/editing/FrameSelection.cpp b/third_party/WebKit/Source/core/editing/FrameSelection.cpp
index 791876c4c3587624b89d3f70758fba01ff29f22c..a723008d4d272179485a8c7b56e495562de2d49d 100644
--- a/third_party/WebKit/Source/core/editing/FrameSelection.cpp
+++ b/third_party/WebKit/Source/core/editing/FrameSelection.cpp
@@ -626,16 +626,7 @@ static Node* nonBoundaryShadowTreeRootNode(const Position& position) {
              : nullptr;
 }
 
-void FrameSelection::selectAll() {
-  if (isHTMLSelectElement(document().focusedElement())) {
-    HTMLSelectElement* selectElement =
-        toHTMLSelectElement(document().focusedElement());
-    if (selectElement->canSelectAll()) {
-      selectElement->selectAll();
-      return;
-    }
-  }
-
+Node* FrameSelection::selectAllRoot() {

[yosin_UTC9, 2017/02/27 03:46:40]

Since this function returns root node for "SelectAll", can we call this function
as |computeRootNodeForSelectAll()| or another name to describe what this
function does?

[amaralp, 2017/02/27 05:02:18]

Renamed to |computeRootNodeForSelectAll()|.

   Node* root = nullptr;
   Node* selectStartTarget = nullptr;
   if (computeVisibleSelectionInDOMTreeDeprecated().isContentEditable()) {
@@ -657,18 +648,33 @@ void FrameSelection::selectAll() {
     }
   }
   if (!root || editingIgnoresContent(*root))
-    return;
+    return nullptr;
 
   if (selectStartTarget) {
     const Document& expectedDocument = document();
     if (selectStartTarget->dispatchEvent(Event::createCancelableBubble(
             EventTypeNames::selectstart)) != DispatchEventResult::NotCanceled)
-      return;
+      return nullptr;
     // |root| may be detached due to selectstart event.
     if (!root->isConnected() || expectedDocument != root->document())
+      return nullptr;
+  }
+  return root;
+}
+
+void FrameSelection::selectAll() {
+  if (isHTMLSelectElement(document().focusedElement())) {

[yosin_UTC9, 2017/02/27 03:46:40]

Since document.focusedElement() can be null, please add null check for it.

[amaralp, 2017/02/27 05:02:18]

|isHTMLSelectElement()| already does the null check. Also this is how it was
done prior to my changes.
Do you still want the null check?

+    HTMLSelectElement* selectElement =
+        toHTMLSelectElement(document().focusedElement());
+    if (selectElement->canSelectAll()) {
+      selectElement->selectAll();
       return;
+    }
   }
 
+  Node* root = selectAllRoot();
+  if (!root)
+    return;
   setSelection(SelectionInDOMTree::Builder()
                    .selectAllChildren(*root)
                    .setIsHandleVisible(isHandleVisible())
@@ -677,6 +683,21 @@ void FrameSelection::selectAll() {
   notifyLayoutObjectOfSelectionChange(UserTriggered);
 }
 
+bool FrameSelection::canSelectAll() {

[yosin_UTC9, 2017/02/27 03:46:40]

It is better to implement in |Editor| class rather than here?

[amaralp, 2017/02/27 05:02:18]

Moved it to |Editor|.

+  if (isHTMLSelectElement(document().focusedElement())) {

[yosin_UTC9, 2017/02/27 03:46:40]

Since document.focusedElement() can be null, please add null check for it.

[amaralp, 2017/02/27 05:02:18]

|isHTMLSelectElement()| already does the null check.

+    HTMLSelectElement* selectElement =
+        toHTMLSelectElement(document().focusedElement());
+    if (selectElement->canSelectAll()) {

[yosin_UTC9, 2017/02/27 03:46:40]

nit: We don't need to have braces.

[amaralp, 2017/02/27 05:02:18]

Done.

+      return false;
+    }
+  }
+  Node* root = selectAllRoot();
+  if (!root)
+    return false;
+  return !(selectionInDOMTree().base() == Position::firstPositionInNode(root) &&
+           selectionInDOMTree().extent() == Position::lastPositionInNode(root));
+}
+
 bool FrameSelection::setSelectedRange(const EphemeralRange& range,
                                       TextAffinity affinity,
                                       SelectionDirectionalMode directional,