Mac: Clicking an omnibox decoration should not highlight the omnibox.

chrome/browser/ui/cocoa/location_bar/autocomplete_text_field_unittest.mm

Index: chrome/browser/ui/cocoa/location_bar/autocomplete_text_field_unittest.mm
diff --git a/chrome/browser/ui/cocoa/location_bar/autocomplete_text_field_unittest.mm b/chrome/browser/ui/cocoa/location_bar/autocomplete_text_field_unittest.mm
index 6d138011b2687a550b13f28c65cb701d7d1a5b86..c094189b582c4f4d847fbca85512655af44f9395 100644
--- a/chrome/browser/ui/cocoa/location_bar/autocomplete_text_field_unittest.mm
+++ b/chrome/browser/ui/cocoa/location_bar/autocomplete_text_field_unittest.mm
@@ -789,10 +789,7 @@ TEST_F(AutocompleteTextFieldTest, HideFocusState) {
   EXPECT_TRUE([FieldEditor() shouldDrawInsertionPoint]);
 }
 
-// Verify that OnSetFocus for button decorations is only sent after the
-// decoration is picked as the target for the subsequent -mouseDown:. Otherwise
-// hiding a ButtonDecoration in OnSetFocus will prevent a call to
-// OnMousePressed, since it is already hidden at the time of mouseDown.
+// Verify that clicking on a button decoration does not focus the omnibox.
 TEST_F(AutocompleteTextFieldObserverTest, ButtonDecorationFocus) {

[erikchen, 2015/05/07 19:33:51]

This test was written for Omnitheatre, and is no longer relevant. I repurposed
the test into a regression test for this CL. 

Note that the previous CL checked that clicking a decoration (which auto-hides)
causes the omnibox to become focused. The auto-hide behavior is no longer
relevant, and of course the omnibox should not become focused.

[Scott Hess - ex-Googler, 2015/05/07 20:07:42]

Would it make sense to further revise this to not be tied to button decorations?
 The change seems to apply to any decoration.

[erikchen, 2015/05/08 01:51:46]

Good suggestion. I updated the test, and discovered a bug in my original CL.
I've made 3 changes:

1. Moved logic from becomeFirstResponder -> acceptsFirstResponder. This has the
same behavior, but makes more sense semantically.
2. I added a check for AcceptsMousePress(). This was a bug in the logic of the
original CL.
3. I added some logic to make the text field resign first responder when a
decoration is selected. This is technically not a mistake in the previous CL,
but it is a bug fix in the same vein.

   // Add the mock button.
   MockButtonDecoration mock_button;
@@ -800,16 +797,12 @@ TEST_F(AutocompleteTextFieldObserverTest, ButtonDecorationFocus) {
   AutocompleteTextFieldCell* cell = [field_ cell];
   [cell addLeftDecoration:&mock_button];
 
-  // Ensure button is hidden when OnSetFocus() is called.
-  EXPECT_CALL(field_observer_, OnSetFocus(false)).WillOnce(
-      testing::InvokeWithoutArgs(&mock_button, &MockButtonDecoration::Hide));
-
   // Ignore incidental calls.
   EXPECT_CALL(field_observer_, SelectionRangeForProposedRange(_))
       .WillRepeatedly(testing::Return(NSMakeRange(0, 0)));
   EXPECT_CALL(field_observer_, OnMouseDown(_));
 
-  // Still expect an OnMousePressed on the button.
+  // Expect an OnMousePressed on the button.
   EXPECT_CALL(mock_button, OnMousePressed(_, _))
       .WillOnce(testing::Return(true));
 
@@ -822,7 +815,8 @@ TEST_F(AutocompleteTextFieldObserverTest, ButtonDecorationFocus) {
 
   // Ensure the field is currently not first responder.
   [test_window() makePretendKeyWindowAndSetFirstResponder:nil];
-  EXPECT_NSNE([[field_ window] firstResponder], field_);
+  EXPECT_FALSE([base::mac::ObjCCast<NSView>(
+      [[field_ window] firstResponder]) isDescendantOf:field_]);

[Scott Hess - ex-Googler, 2015/05/07 20:07:42]

At this point is there any first responder at all for the window?  As-is this
feels like it could be FALSE for many reasons, the most specific one might be
preferable.

[erikchen, 2015/05/08 01:51:46]

The first responder is CocoaTestHelperWindow. I'd prefer to not gain specificity
at the cost of adding a dependency between this class and the default first
responder of the test window.

 
   // Execute button click event sequence.
   NSEvent* downEvent = Event(field_, click_location, NSLeftMouseDown);
@@ -837,12 +831,12 @@ TEST_F(AutocompleteTextFieldObserverTest, ButtonDecorationFocus) {
                                              dequeue:YES];
   [NSApp sendEvent:next_event];
 
-  // Expectations check that both OnSetFocus and OnMouseDown were called.
-  // Additionally, ensure button is hidden and field is firstResponder.
-  EXPECT_FALSE(mock_button.IsVisible());
-  EXPECT_TRUE(NSIsEmptyRect([cell frameForDecoration:&mock_left_decoration_
+  // Expectations check that OnMouseDown was called. Additionally, ensure
+  // button is visible and field is not firstResponder.
+  EXPECT_TRUE(mock_button.IsVisible());
+  EXPECT_FALSE(NSIsEmptyRect([cell frameForDecoration:&mock_button
                                              inFrame:[field_ bounds]]));

[Scott Hess - ex-Googler, 2015/05/07 20:07:42]

Alignment.

[erikchen, 2015/05/08 01:51:46]

I clang-format-ed the entire CL.

[Scott Hess - ex-Googler, 2015/05/08 19:38:28]

And the offending code is no longer present anyhow... if the previous version
was run through clang-format, then there's a bug somewhere.

[erikchen, 2015/05/08 20:02:20]

The previous code was not. I should have been more clear - I clang-format-ed the
updated CL.

-  EXPECT_TRUE([base::mac::ObjCCastStrict<NSView>(
+  EXPECT_FALSE([base::mac::ObjCCast<NSView>(
       [[field_ window] firstResponder]) isDescendantOf:field_]);

[Scott Hess - ex-Googler, 2015/05/07 20:07:42]

Here also.

[erikchen, 2015/05/08 01:51:46]

ditto.

 }