Reland [Mac] Re-Generate Text Suggestions on WebContents Update

Previously, each tab had its own SuggestedTextTouchBarController. Now,
because of http://crrev/c/1151613, there is only one
SuggestedTextTouchBarController per window. Because the controller does
not maintain the state of each tab, as a user switches tabs, the
controller needs to update its state to match the tab. When the
controller's WebContents change, new suggestions should be generated
for the selected text in the current tab.

The methods -webContentsTextSelectionChanged:range: and
-webContentsFinishedLoading were combined into a singular method,
-updateTextSelection:range:, because they had very similar
functionality.

The parameters for -requestSuggestionsForText:inRange: were removed
because they were always |text_| and |selectionRange_|.

A new method was created, -webContentsChanged, to get a tab's current
text and selected range and then call -updateTextSelection:range: with
the proper parameters.

When the controller gets a new pointer to a WebContents, the pointer
may be null. To avoid calling WebContents::IsFocusedElementEditable()
on a null pointer, a method -isTextfieldFocused was created to check
that |webContents_| is not a null pointer and that an editable element
is focused.

A new SuggestedTextTouchBarControllerBrowserTest was written,
SetWebContentsTest, to test that the SuggestedTextTouchBarController
properly updates its current text selection based on the new
WebContents.

This CL is the same as http://crrev/c/1153728 with the following
changes:
  - Browser tests for methods that are not restricted to MacOS 10.12.2+
    were modified to also run on earlier versions of MacOS. The
    expected behavior for different MacOS versions is accounted for in
    the tests.
  - A call to UnfocusTextfield() was added to SetWebContentsTest to
    ensure that a later call to FocusTextfield() works as intended and
    focuses an editable element.

Bug: 717553
Change-Id: Id061e392dd279e7a3cc9a34a023eb9bd97e52940
Reviewed-on: https://chromium-review.googlesource.com/1160929Reviewed-by: Avi Drissman <avi@chromium.org>
Commit-Queue: Tessa Nijssen <tnijssen@google.com>
Cr-Commit-Position: refs/heads/master@{#580658}

chrome/browser/ui/cocoa/touchbar/suggested_text_touch_bar_controller.h

@@ -19,6 +19,10 @@ namespace content {
 class WebContents;
 }  // namespace content
 
+namespace gfx {
+class Range;
+}  // namespace gfx
+
 @interface SuggestedTextTouchBarController
     : NSObject<NSTouchBarDelegate, NSCandidateListTouchBarItemDelegate>
 
@@ -40,19 +44,15 @@ class WebContents;
      endSelectingCandidateAtIndex:(NSInteger)index
     API_AVAILABLE(macos(10.12.2));
 
-- (void)webContentsTextSelectionChanged:(const base::string16&)text
-                                  range:(NSRange)range
-    API_AVAILABLE(macos(10.12.2));
-
-- (void)webContentsFinishedLoading API_AVAILABLE(macos(10.12.2));
+- (void)updateTextSelection:(const base::string16&)text
+                      range:(const gfx::Range&)range;
 
 // Returns a range from start to the end of the word that the cursor is
 // currently in.
 - (NSRange)editingWordRangeFromText:(const base::string16&)text
                      cursorPosition:(size_t)cursor;
 
-- (void)requestSuggestionsForText:(NSString*)text
-                          inRange:(NSRange)range API_AVAILABLE(macos(10.12.2));
+- (void)requestSuggestions API_AVAILABLE(macos(10.12.2));
 
 // Select the range of the editing word and replace it with a suggestion
 // from the touch bar.
@@ -69,8 +69,8 @@ class WebContents;
 - (WebTextfieldTouchBarController*)controller;
 - (void)setWebContents:(content::WebContents*)webContents;
 - (content::WebContents*)webContents;
-- (void)setSelectionRange:(NSRange)range;
-- (NSRange)selectionRange;
+- (void)setSelectionRange:(const gfx::Range&)range;
+- (gfx::Range)selectionRange;
 
 @end
 

chrome/browser/ui/cocoa/touchbar/suggested_text_touch_bar_controller.mm

@@ -6,6 +6,7 @@
 
 #include "base/i18n/break_iterator.h"
 #include "base/strings/sys_string_conversions.h"
+#include "content/public/browser/render_widget_host_view.h"
 #include "content/public/browser/web_contents.h"
 #include "content/public/browser/web_contents_observer.h"
 #import "ui/base/cocoa/touch_bar_util.h"
@@ -32,16 +33,12 @@ class WebContentsTextObserver : public content::WebContentsObserver {
 
   void DidChangeTextSelection(const base::string16& text,
                               const gfx::Range& range) override {
-    if (@available(macOS 10.12.2, *)) {
-      [owner_ webContentsTextSelectionChanged:text range:range.ToNSRange()];
-    }
+    [owner_ updateTextSelection:text range:range];
   }
 
   void DidFinishLoad(content::RenderFrameHost* render_frame_host,
                      const GURL& validated_url) override {
-    if (@available(macOS 10.12.2, *)) {
-      [owner_ webContentsFinishedLoading];
-    }
+    [owner_ updateTextSelection:base::string16() range:gfx::Range()];
   }
 
  private:
@@ -80,6 +77,10 @@ class WebContentsTextObserver : public content::WebContentsObserver {
 
 @implementation SuggestedTextTouchBarController
 
+- (BOOL)isTextfieldFocused {
+  return webContents_ && webContents_->IsFocusedElementEditable();
+}
+
 - (instancetype)initWithWebContents:(content::WebContents*)webContents
                          controller:
                              (WebTextfieldTouchBarController*)controller {
@@ -94,7 +95,7 @@ class WebContentsTextObserver : public content::WebContentsObserver {
 }
 
 - (NSTouchBar*)makeTouchBar {
-  if (!webContents_ || !webContents_->IsFocusedElementEditable())
+  if (![self isTextfieldFocused])
     return nil;
 
   base::scoped_nsobject<NSTouchBar> touchBar([[ui::NSTouchBar() alloc] init]);
@@ -143,26 +144,19 @@ class WebContentsTextObserver : public content::WebContentsObserver {
   ui::LogTouchBarUMA(ui::TouchBarAction::TEXT_SUGGESTION);
 }
 
-- (void)webContentsTextSelectionChanged:(const base::string16&)text
-                                  range:(NSRange)range {
-  if (webContents_->IsFocusedElementEditable()) {
-    [self setText:base::SysUTF16ToNSString(text)];
-    selectionRange_ = range;
-    editingWordRange_ =
-        [self editingWordRangeFromText:text cursorPosition:range.location];
-    [self requestSuggestionsForText:text_ inRange:range];
-  } else {
-    [controller_ invalidateTouchBar];
-  }
-}
+- (void)updateTextSelection:(const base::string16&)text
+                      range:(const gfx::Range&)range {
+  if (@available(macOS 10.12.2, *)) {
+    if (![self isTextfieldFocused]) {
+      [controller_ invalidateTouchBar];
+      return;
+    }
 
-- (void)webContentsFinishedLoading {
-  if (webContents_->IsFocusedElementEditable()) {
-    // TODO(tnijssen): Actually request the current text and cursor position.
-    [self setText:@""];
-    selectionRange_ = NSMakeRange(0, 0);
-    editingWordRange_ = NSMakeRange(0, 0);
-    [self requestSuggestionsForText:@"" inRange:NSMakeRange(0, 0)];
+    text_.reset([base::SysUTF16ToNSString(text) retain]);
+    selectionRange_ = range.ToNSRange();
+    editingWordRange_ =
+        [self editingWordRangeFromText:text cursorPosition:range.start()];
+    [self requestSuggestions];
   }
 }
 
@@ -204,11 +198,11 @@ class WebContentsTextObserver : public content::WebContentsObserver {
   return NSMakeRange(location, length);
 }
 
-- (void)requestSuggestionsForText:(NSString*)text inRange:(NSRange)range {
+- (void)requestSuggestions {
   NSSpellChecker* spell_checker = [NSSpellChecker sharedSpellChecker];
   [spell_checker
-      requestCandidatesForSelectedRange:range
-                               inString:text
+      requestCandidatesForSelectedRange:selectionRange_
+                               inString:text_
                                   types:NSTextCheckingAllSystemTypes
                                 options:nil
                  inSpellDocumentWithTag:0
@@ -254,21 +248,34 @@ class WebContentsTextObserver : public content::WebContentsObserver {
 }
 
 - (void)setWebContents:(content::WebContents*)webContents {
-  // TODO(tnijssen): Update the text suggestions when we change web contents.
   webContents_ = webContents;
   observer_->UpdateWebContents(webContents);
+
+  if (![self isTextfieldFocused]) {
+    [controller_ invalidateTouchBar];
+    return;
+  }
+
+  const base::string16 text =
+      webContents_->GetTopLevelRenderWidgetHostView()->GetSurroundingText();
+  const gfx::Range range =
+      webContents_->GetTopLevelRenderWidgetHostView()->GetSelectedRange();
+  if (range.IsValid())
+    [self updateTextSelection:text range:range];
+  else
+    [self updateTextSelection:base::string16() range:gfx::Range()];
 }
 
 - (content::WebContents*)webContents {
   return webContents_;
 }
 
-- (void)setSelectionRange:(NSRange)range {
-  selectionRange_ = range;
+- (void)setSelectionRange:(const gfx::Range&)range {
+  selectionRange_ = range.ToNSRange();
 }
 
-- (NSRange)selectionRange {
-  return selectionRange_;
+- (gfx::Range)selectionRange {
+  return gfx::Range(selectionRange_);
 }
 
 @end

chrome/browser/ui/cocoa/touchbar/suggested_text_touch_bar_controller_browsertest.mm

@@ -52,8 +52,8 @@
 
 @implementation MockSuggestedTextTouchBarController
 
-- (void)requestSuggestionsForText:(NSString*)text inRange:(NSRange)range {
-  [self setSuggestions:@[ text ]];
+- (void)requestSuggestions {
+  [self setSuggestions:@[ [self text] ]];
   [[self controller] invalidateTouchBar];
 }
 
@@ -65,10 +65,6 @@
 
 namespace {
 
-NSString* const kSuggestedTextTouchBarId = @"suggested-text";
-NSString* const kText = @"text";
-NSString* const kEmptyText = @"";
-
 class SuggestedTextTouchBarControllerBrowserTest : public InProcessBrowserTest {
  public:
   void SetUp() override {
@@ -80,10 +76,8 @@ class SuggestedTextTouchBarControllerBrowserTest : public InProcessBrowserTest {
     web_textfield_controller_.reset(
         [[MockWebTextfieldTouchBarController alloc] init]);
     [web_textfield_controller_ resetNumInvalidations];
-    content::WebContents* web_contents =
-        browser()->tab_strip_model()->GetActiveWebContents();
     touch_bar_controller_.reset([[MockSuggestedTextTouchBarController alloc]
-        initWithWebContents:web_contents
+        initWithWebContents:GetActiveWebContents()
                  controller:web_textfield_controller_]);
   }
 
@@ -98,15 +92,10 @@ class SuggestedTextTouchBarControllerBrowserTest : public InProcessBrowserTest {
         browser(), GURL("data:text/html;charset=utf-8,<input type=\"text\">"));
   }
 
-  MockWebTextfieldTouchBarController* web_textfield_controller() const {
-    return web_textfield_controller_;
-  }
-
-  MockSuggestedTextTouchBarController* touch_bar_controller() const {
-    return touch_bar_controller_;
+  content::WebContents* GetActiveWebContents() {
+    return browser()->tab_strip_model()->GetActiveWebContents();
   }
 
- private:
   base::scoped_nsobject<MockWebTextfieldTouchBarController>
       web_textfield_controller_;
   base::scoped_nsobject<MockSuggestedTextTouchBarController>
@@ -118,13 +107,15 @@ class SuggestedTextTouchBarControllerBrowserTest : public InProcessBrowserTest {
 IN_PROC_BROWSER_TEST_F(SuggestedTextTouchBarControllerBrowserTest,
                        TouchBarTest) {
   if (@available(macOS 10.12.2, *)) {
+    NSString* const kSuggestedTextTouchBarId = @"suggested-text";
+
     // Touch bar shouldn't appear if the focused element is not a textfield.
     UnfocusTextfield();
-    EXPECT_FALSE([touch_bar_controller() makeTouchBar]);
+    EXPECT_FALSE([touch_bar_controller_ makeTouchBar]);
 
     // Touch bar should appear if a textfield is focused.
     FocusTextfield();
-    NSTouchBar* touch_bar = [touch_bar_controller() makeTouchBar];
+    NSTouchBar* touch_bar = [touch_bar_controller_ makeTouchBar];
     EXPECT_TRUE(touch_bar);
     EXPECT_TRUE([[touch_bar customizationIdentifier]
         isEqual:ui::GetTouchBarId(kSuggestedTextTouchBarId)]);
@@ -133,47 +124,81 @@ IN_PROC_BROWSER_TEST_F(SuggestedTextTouchBarControllerBrowserTest,
 
 // Tests that a change in text selection is handled properly.
 IN_PROC_BROWSER_TEST_F(SuggestedTextTouchBarControllerBrowserTest,
-                       TextSelectionChangedTest) {
+                       UpdateTextSelectionTest) {
+  NSString* const kText = @"text";
+  NSString* const kEmptyText = @"";
+  const gfx::Range kRange = gfx::Range(0, 4);
+  const gfx::Range kEmptyRange = gfx::Range();
+
+  // If not in a textfield,
+  //  1. Textfield text should not be saved.
+  //  2. Selected range should not be saved.
+  //  3. Touch bar should be invalidated (if on MacOS 10.12.2 or later).
+  UnfocusTextfield();
+  [touch_bar_controller_ setText:kEmptyText];
+  [touch_bar_controller_ setSelectionRange:kEmptyRange];
+  [web_textfield_controller_ resetNumInvalidations];
+
+  [touch_bar_controller_ updateTextSelection:base::SysNSStringToUTF16(kText)
+                                       range:kRange];
+  EXPECT_STREQ(kEmptyText.UTF8String, [touch_bar_controller_ text].UTF8String);
+  EXPECT_EQ(kEmptyRange, [touch_bar_controller_ selectionRange]);
+  if (@available(macOS 10.12.2, *))
+    EXPECT_EQ(1, [web_textfield_controller_ numInvalidations]);
+  else
+    EXPECT_EQ(0, [web_textfield_controller_ numInvalidations]);
+
+  // If in a textfield and on MacOS 10.12.2 or later,
+  //   1. Textfield text should be saved.
+  //   2. Selected range should be saved.
+  //   3. Suggestions should be generated based on text selection.
+  //   4. Touch bar should be invalidated.
+  FocusTextfield();
+  [touch_bar_controller_ setText:kEmptyText];
+  [touch_bar_controller_ setSelectionRange:kEmptyRange];
+  [web_textfield_controller_ resetNumInvalidations];
+
+  [touch_bar_controller_ updateTextSelection:base::SysNSStringToUTF16(kText)
+                                       range:kRange];
   if (@available(macOS 10.12.2, *)) {
-    const NSRange kRange = NSMakeRange(0, [kText length]);
-    const NSRange kEmptyRange = NSMakeRange(0, 0);
+    EXPECT_STREQ(kText.UTF8String, [touch_bar_controller_ text].UTF8String);
+    EXPECT_EQ(kRange, [touch_bar_controller_ selectionRange]);
+    EXPECT_STREQ(kText.UTF8String,
+                 [touch_bar_controller_ firstSuggestion].UTF8String);
+    EXPECT_EQ(1, [web_textfield_controller_ numInvalidations]);
+  } else {
+    EXPECT_STREQ(kEmptyText.UTF8String,
+                 [touch_bar_controller_ text].UTF8String);
+    EXPECT_EQ(kEmptyRange, [touch_bar_controller_ selectionRange]);
+    EXPECT_EQ(0, [web_textfield_controller_ numInvalidations]);
+  }
+}
 
-    // If not in a textfield,
-    //  1. Textfield text should not be saved.
-    //  2. Selected range should not be saved.
-    //  3. Touch bar should be invalidated.
-    UnfocusTextfield();
-    [touch_bar_controller() setText:kEmptyText];
-    [touch_bar_controller() setSelectionRange:kEmptyRange];
-    [web_textfield_controller() resetNumInvalidations];
-
-    [touch_bar_controller()
-        webContentsTextSelectionChanged:base::SysNSStringToUTF16(kText)
-                                  range:kRange];
-    EXPECT_TRUE([kEmptyText isEqualToString:[touch_bar_controller() text]]);
-    EXPECT_EQ(gfx::Range(kEmptyRange),
-              gfx::Range([touch_bar_controller() selectionRange]));
-    EXPECT_EQ(1, [web_textfield_controller() numInvalidations]);
-
-    // If in a textfield,
-    //   1. Textfield text should be saved.
-    //   2. Selected range should be saved.
-    //   3. Suggestions should be generated based on text selection.
-    //   4. Touch bar should be invalidated.
-    FocusTextfield();
-    [touch_bar_controller() setText:kEmptyText];
-    [touch_bar_controller() setSelectionRange:kEmptyRange];
-    [web_textfield_controller() resetNumInvalidations];
-
-    [touch_bar_controller()
-        webContentsTextSelectionChanged:base::SysNSStringToUTF16(kText)
-                                  range:kRange];
-    EXPECT_TRUE([kText isEqualToString:[touch_bar_controller() text]]);
-    EXPECT_EQ(gfx::Range(kRange),
-              gfx::Range([touch_bar_controller() selectionRange]));
-    EXPECT_TRUE(
-        [kText isEqualToString:[touch_bar_controller() firstSuggestion]]);
-    EXPECT_EQ(1, [web_textfield_controller() numInvalidations]);
+// Tests that a change in WebContents is handled properly.
+IN_PROC_BROWSER_TEST_F(SuggestedTextTouchBarControllerBrowserTest,
+                       SetWebContentsTest) {
+  NSString* const kText = @"text";
+  const gfx::Range kRange = gfx::Range(1, 1);
+
+  // A null-pointer should not break the controller.
+  UnfocusTextfield();
+  [touch_bar_controller_ setWebContents:nullptr];
+  EXPECT_FALSE([touch_bar_controller_ webContents]);
+
+  FocusTextfield();
+  [touch_bar_controller_ setText:kText];
+  [touch_bar_controller_ setSelectionRange:kRange];
+
+  // The text selection should change on MacOS 10.12.2 and later if the
+  // WebContents pointer is not null.
+  [touch_bar_controller_ setWebContents:GetActiveWebContents()];
+  EXPECT_EQ(GetActiveWebContents(), [touch_bar_controller_ webContents]);
+  if (@available(macOS 10.12.2, *)) {
+    EXPECT_STRNE(kText.UTF8String, [touch_bar_controller_ text].UTF8String);
+    EXPECT_NE(kRange, [touch_bar_controller_ selectionRange]);
+  } else {
+    EXPECT_STREQ(kText.UTF8String, [touch_bar_controller_ text].UTF8String);
+    EXPECT_EQ(kRange, [touch_bar_controller_ selectionRange]);
   }
 }
 

chrome/browser/ui/cocoa/touchbar/suggested_text_touch_bar_controller_unittest.mm

@@ -39,11 +39,11 @@ TEST_F(SuggestedTextTouchBarControllerUnitTest, CollapsedCandidateListTest) {
   if (@available(macOS 10.12.2, *)) {
     base::scoped_nsobject<NSCandidateListTouchBarItem> item;
 
-    [controller_ setSelectionRange:NSMakeRange(0, 0)];
+    [controller_ setSelectionRange:gfx::Range()];
     item.reset([controller_ createCandidateListItem]);
     EXPECT_FALSE([item isCollapsed]);
 
-    [controller_ setSelectionRange:NSMakeRange(0, 1)];
+    [controller_ setSelectionRange:gfx::Range(0, 1)];
     item.reset([controller_ createCandidateListItem]);
     EXPECT_TRUE([item isCollapsed]);
   }