MacViews: Fix views_unittests BubbleDelegateTest.CloseReasons

ui/views/bubble/bubble_delegate_unittest.cc

 // Copyright (c) 2012 The Chromium Authors. All rights reserved.

[tapted, 2016/01/28 02:50:53]

Don't forget to wrap the CL description. It should also incorporate some of the
findings below (e.g. that the bubble anchor should be shown before showing the
bubble, and that the ScopedFakeNSWindowFocus is being added to all tests to be
defensive against test flakiness triggered by window activation.

(although I think currently the tests are not flaky -- but that's just because
the test doesn't ever spin a runloop)

[Patti Lor, 2016/03/01 00:30:47]

Done.

 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include <stddef.h>
 
 #include "base/macros.h"
 #include "ui/base/hit_test.h"
 #include "ui/events/event_utils.h"
 #include "ui/views/bubble/bubble_delegate.h"
 #include "ui/views/bubble/bubble_frame_view.h"
 #include "ui/views/controls/button/label_button.h"
 #include "ui/views/test/test_widget_observer.h"
 #include "ui/views/test/views_test_base.h"
 #include "ui/views/widget/widget.h"
 #include "ui/views/widget/widget_observer.h"
 
+#if defined(OS_MACOSX)
+#include "ui/base/test/scoped_fake_nswindow_focus.h"
+#endif
+
 namespace views {
 
 namespace {
 
 class TestBubbleDelegateView : public BubbleDelegateView {
  public:
   TestBubbleDelegateView(View* anchor_view)
       : BubbleDelegateView(anchor_view, BubbleBorder::TOP_LEFT),
         view_(new View()) {
     view_->SetFocusable(true);
@@ skipping 242 matching lines @@
   EXPECT_FALSE(bubble_widget->CanActivate());
 }
 
 TEST_F(BubbleDelegateTest, CloseReasons) {
   {
     scoped_ptr<Widget> anchor_widget(CreateTestWidget());
     BubbleDelegateView* bubble_delegate = new BubbleDelegateView(
         anchor_widget->GetContentsView(), BubbleBorder::NONE);
     bubble_delegate->set_close_on_deactivate(true);
     Widget* bubble_widget = BubbleDelegateView::CreateBubble(bubble_delegate);
+#if defined(OS_MACOSX)
+    // Bubble widget will not show unless anchor widget is activated in the
+    // first place, which doesn't happen by default on Mac.
+    ui::test::ScopedFakeNSWindowFocus fake_focus;
+    anchor_widget->Activate();
+#endif
     bubble_widget->Show();

[tapted, 2016/01/28 02:50:53]

I think the fix should be to call anchor_widget->Show() before this line on all
platforms. The test then wants to activate a different window, which should
cause the bubble to close itself.

But
 1) it's weird that this works on other platforms. Probably *after* this line
there should also be `EXPECT_TRUE(bubble_widget->IsVisible());` which would
currently fail on all platforms (when it really shouldn't -- and adding
anchor_widget->Show() should fix that). It's still a little odd the rest of the
test passes (i.e. why is the close reason set when the bubble was never shown).
Then,

 2) Activate() is asynchronous on Mac (also Linux, but this test gets around
that by using non-toplevel / Ash widgets which don't exist on Mac, so we can't
use them). So even after the Show() call, the call to anchor_widget->Activate();
won't achieve much on Mac without the ScopedFakeNSWindowFocus. And.. really that
should be affecting lots of tests in this file.

So I think the steps to take are:

1) Add EXPECT_TRUE(bubble_widget->IsVisible()) after this line, and to `git cl
try` to verify that that fails on other platforms.
2) Add anchor_widget->Show() above this line, and `git cl try` to verify that
bubble_widget->IsVisible() is now true after the line
3) Verify that the bubble_widget->IsClosed() test still fails on Mac (because
activation is asynchronous).
4) Put the ScopedFakeNSWindowFocus as a data member of BubbleDelegateTest.
Rationale: Any test in this file that has set_close_on_deactivate(true) could
behave flakily since the file assumes activation is synchronous when (on Mac)
it's not. 

[Patti Lor, 2016/03/01 00:30:47]

So as discussed offline, this isn't actually the case, meaning there's a bug on
Aura. For now I've added the check from step 1 as you suggested, expecting a
failure on that line as well as the following two expects on all platforms.

     anchor_widget->Activate();
     EXPECT_TRUE(bubble_widget->IsClosed());
     EXPECT_EQ(BubbleDelegateView::CloseReason::DEACTIVATION,
               bubble_delegate->close_reason());
   }
 
   {
     scoped_ptr<Widget> anchor_widget(CreateTestWidget());
     BubbleDelegateView* bubble_delegate = new BubbleDelegateView(
         anchor_widget->GetContentsView(), BubbleBorder::NONE);
@@ skipping 22 matching lines @@
         close_button,
         ui::MouseEvent(ui::ET_MOUSE_PRESSED, gfx::Point(0, 0), gfx::Point(0, 0),
                        ui::EventTimeForNow(), ui::EF_NONE, ui::EF_NONE));
     EXPECT_TRUE(bubble_widget->IsClosed());
     EXPECT_EQ(BubbleDelegateView::CloseReason::CLOSE_BUTTON,
               bubble_delegate->close_reason());
   }
 }
 
 }  // namespace views