Select All present even all selectable text has been selected

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

 /*
  * Copyright (C) 2004, 2008, 2009, 2010 Apple Inc. All rights reserved.
  *
  * 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
  *    notice, this list of conditions and the following disclaimer in the
@@ skipping 608 matching lines @@
 }
 
 // Returns a shadow tree node for legacy shadow trees, a child of the
 // ShadowRoot node for new shadow trees, or 0 for non-shadow trees.
 static Node* nonBoundaryShadowTreeRootNode(const Position& position) {
   return position.anchorNode() && !position.anchorNode()->isShadowRoot()
              ? position.anchorNode()->nonBoundaryShadowTreeRootNode()
              : 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()) {
     root = highestEditableRoot(
         computeVisibleSelectionInDOMTreeDeprecated().start());
     if (Node* shadowRoot = nonBoundaryShadowTreeRootNode(
             computeVisibleSelectionInDOMTreeDeprecated().start()))
       selectStartTarget = shadowRoot->ownerShadowHost();
     else
       selectStartTarget = root;
   } else {
     root = nonBoundaryShadowTreeRootNode(
         computeVisibleSelectionInDOMTreeDeprecated().start());
     if (root) {
       selectStartTarget = root->ownerShadowHost();
     } else {
       root = document().documentElement();
       selectStartTarget = document().body();
     }
   }
   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())
                    .build());
   selectFrameElementInParentIfFullySelected();
   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,
                                       SetSelectionOptions options) {
   if (range.isNull())
     return false;
   m_selectionEditor->resetLogicalRange();
   // Since |FrameSeleciton::setSelection()| dispatches events and DOM tree
   // can be modified by event handlers, we should create |Range| object before
   // calling it.
@@ skipping 500 matching lines @@
 }
 
 void showTree(const blink::FrameSelection* sel) {
   if (sel)
     sel->showTreeForThis();
   else
     LOG(INFO) << "Cannot showTree for <null> FrameSelection.";
 }
 
 #endif