Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1637383003:
MacViews: Fix views_unittests BubbleDelegateTest.CloseReasons (Closed)
|
Created:
4 years, 10 months ago by Patti Lor Modified:
4 years, 9 months ago CC:
chromium-reviews, tfarina, msw+watch_chromium.org, groby+bubble_chromium.org, hcarmona+bubble_chromium.org, rouslan+bubble_chromium.org, chrome-apps-syd-reviews_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionMacViews: Fix views_unittests BubbleDelegateTest.CloseReasons
Test added in r360848, fails on Mac as the bubble's parent anchor is not being
shown before the bubble is expected to appear - this causes the bubble to be
treated as 'hidden' as its parent is not visible. This should actually be
expected behaviour on all platforms, so the failure is actually indicating
there's a bug on Aura (see http://crbug/590957).
Workaround this for now by showing the bubble's anchor widget first to allow
this to pass for all platforms.
Also add ScopedFakeNSWindowFocus to all tests to simulate focus on the parent,
as well as defend against test flakiness which might be caused by window
activation issues.
BUG=579380, 590957
Committed: https://crrev.com/71d4b0f60b78186e0eff2a287a9a2036488836a8
Cr-Commit-Position: refs/heads/master@{#379499}
Patch Set 1 #
Total comments: 4
Patch Set 2 : Verify bubble is visible after showing it. #Patch Set 3 : Temporary patch to verify failure on other platforms. #Patch Set 4 : Tentative fix for Aura not parenting bubbles properly, verify bubble invisible. #Patch Set 5 : Fix type of params.child (bool, not ptr). #
Total comments: 4
Patch Set 6 : Change test behaviour based on Aura vs non-Aura. #
Total comments: 2
Patch Set 7 : Cleanup, use less ifdefs. #Patch Set 8 : Fix ifndef (windows compile breaks). #
Total comments: 2
Patch Set 9 : Re-wrap comment. #
Total comments: 4
Patch Set 10 : Remove separate behaviour for Aura vs non-Aura. #Messages
Total messages: 31 (14 generated)
Description was changed from ========== Fix BubbleDelegateTest.CloseReasons on Mac, bubble widget was not being activated because its parent was not activated. BUG=579380 ========== to ========== Fix BubbleDelegateTest.CloseReasons on Mac, bubble widget was not being activated because its parent was not activated. BUG=579380 ==========
patricialor@chromium.org changed reviewers: + tapted@chromium.org
Description was changed from ========== Fix BubbleDelegateTest.CloseReasons on Mac, bubble widget was not being activated because its parent was not activated. BUG=579380 ========== to ========== Fix BubbleDelegateTest.CloseReasons on Mac, bubble widget was not being activated because its parent was not activated. Subject: MacViews: Fix views_unittests BubbleDelegateTest.CloseReasons Description: MacViews: Fix views_unittests BubbleDelegateTest.CloseReasons Test added in r360848, fails on Mac due to the bubble's parent not being activated (and thus being treated as 'hidden'). To fix, use ScopedFakeNSWindowFocus to simulate focus on the parent and call activate on it manually. BUG=579380 ==========
Description was changed from ========== Fix BubbleDelegateTest.CloseReasons on Mac, bubble widget was not being activated because its parent was not activated. Subject: MacViews: Fix views_unittests BubbleDelegateTest.CloseReasons Description: MacViews: Fix views_unittests BubbleDelegateTest.CloseReasons Test added in r360848, fails on Mac due to the bubble's parent not being activated (and thus being treated as 'hidden'). To fix, use ScopedFakeNSWindowFocus to simulate focus on the parent and call activate on it manually. BUG=579380 ========== to ========== Subject: MacViews: Fix views_unittests BubbleDelegateTest.CloseReasons Description: MacViews: Fix views_unittests BubbleDelegateTest.CloseReasons Test added in r360848, fails on Mac due to the bubble's parent not being activated (and thus being treated as 'hidden'). To fix, use ScopedFakeNSWindowFocus to simulate focus on the parent and call activate on it manually. BUG=579380 ==========
Description was changed from ========== Subject: MacViews: Fix views_unittests BubbleDelegateTest.CloseReasons Description: MacViews: Fix views_unittests BubbleDelegateTest.CloseReasons Test added in r360848, fails on Mac due to the bubble's parent not being activated (and thus being treated as 'hidden'). To fix, use ScopedFakeNSWindowFocus to simulate focus on the parent and call activate on it manually. BUG=579380 ========== to ========== MacViews: Fix views_unittests BubbleDelegateTest.CloseReasons Test added in r360848, fails on Mac due to the bubble's parent not being activated (and thus being treated as 'hidden'). To fix, use ScopedFakeNSWindowFocus to simulate focus on the parent and call activate on it manually. BUG=579380 ==========
https://codereview.chromium.org/1637383003/diff/1/ui/views/bubble/bubble_dele... File ui/views/bubble/bubble_delegate_unittest.cc (right): https://codereview.chromium.org/1637383003/diff/1/ui/views/bubble/bubble_dele... ui/views/bubble/bubble_delegate_unittest.cc:1: // Copyright (c) 2012 The Chromium Authors. All rights reserved. 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) https://codereview.chromium.org/1637383003/diff/1/ui/views/bubble/bubble_dele... ui/views/bubble/bubble_delegate_unittest.cc:290: bubble_widget->Show(); 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.
Description was changed from ========== MacViews: Fix views_unittests BubbleDelegateTest.CloseReasons Test added in r360848, fails on Mac due to the bubble's parent not being activated (and thus being treated as 'hidden'). To fix, use ScopedFakeNSWindowFocus to simulate focus on the parent and call activate on it manually. BUG=579380 ========== to ========== MacViews: Fix views_unittests BubbleDelegateTest.CloseReasons Test added in r360848, fails on Mac as the bubble's parent anchor is not being shown before the bubble is expected to appear - this causes the bubble to be treated as 'hidden' as its parent is not visible. Fix is to add ScopedFakeNSWindowFocus to all tests to simulate focus on the parent, as well as defend against test flakiness which might be caused by window activation issues. BUG=579380 ==========
Description was changed from ========== MacViews: Fix views_unittests BubbleDelegateTest.CloseReasons Test added in r360848, fails on Mac as the bubble's parent anchor is not being shown before the bubble is expected to appear - this causes the bubble to be treated as 'hidden' as its parent is not visible. Fix is to add ScopedFakeNSWindowFocus to all tests to simulate focus on the parent, as well as defend against test flakiness which might be caused by window activation issues. BUG=579380 ========== to ========== MacViews: Fix views_unittests BubbleDelegateTest.CloseReasons Test added in r360848, fails on Mac as the bubble's parent anchor is not being shown before the bubble is expected to appear - this causes the bubble to be treated as 'hidden' as its parent is not visible. This should actually be expected behaviour on all platforms, so the failure is actually indicating there's a bug on Aura. Fix is to make sure the bubble widget is parented properly with the anchor widget, by indicating the bubble is a child during widget creation. Also add ScopedFakeNSWindowFocus to all tests to simulate focus on the parent, as well as defend against test flakiness which might be caused by window activation issues. BUG=579380 ==========
Hi Trent, PTAL, it's still a WIP. The problem is that the bubble's layer isn't actually parented under the anchor's layer, so a IsVisible() check on the bubble won't check the anchor is visible as part of its ancestors. If we want this to be the case, I've added a line to the bubble delegate that turns on the child flag on the bubble widget, which parents the bubble layer properly and fixes the bubble show behaviour (thus making the test fail on Aura as well as Mac like we want). Unfortunately I think this also breaks a lot of other tests, so I am not sure if this is the right fix - have any ideas? https://chromiumcodereview.appspot.com/1637383003/diff/1/ui/views/bubble/bubb... File ui/views/bubble/bubble_delegate_unittest.cc (right): https://chromiumcodereview.appspot.com/1637383003/diff/1/ui/views/bubble/bubb... ui/views/bubble/bubble_delegate_unittest.cc:1: // Copyright (c) 2012 The Chromium Authors. All rights reserved. On 2016/01/28 02:50:53, tapted wrote: > 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) Done. https://chromiumcodereview.appspot.com/1637383003/diff/1/ui/views/bubble/bubb... ui/views/bubble/bubble_delegate_unittest.cc:290: bubble_widget->Show(); On 2016/01/28 02:50:53, tapted wrote: > 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. 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.
So, yes, I think this is a bug in Aura. (but we can't set child = true for bubbles - see below). The Aura bug is: When child = false, the bubble is managed by TransientWindowManager rather than ui::Layers. transient_window_manager.h says "// . If a transient parent is hidden, it hides all transient children." However, it currently allows a transient child to be initially *shown* with a hidden transient parent. It probably shouldn't. So can you file a new crbug on Aura? Then cite it below as http://crbug.com/XXXXX (i.e. XXXXX = bug number) I don't know if it's a priority to actually fix this bug on Aura, but it helps us move forward here. On 2016/03/01 00:30:47, Patti Lor wrote: > Hi Trent, PTAL, it's still a WIP. The problem is that the bubble's layer isn't > actually parented under the anchor's layer, so a IsVisible() check on the bubble > won't check the anchor is visible as part of its ancestors. If we want this to > be the case, I've added a line to the bubble delegate that turns on the child > flag on the bubble widget, which parents the bubble layer properly and fixes the > bubble show behaviour (thus making the test fail on Aura as well as Mac like we > want). > > Unfortunately I think this also breaks a lot of other tests, so I am not sure if > this is the right fix - have any ideas? Ah! Excellent find. So the difference between Widget::InitParams::parent and Widget::InitParams::child is subtle. Maybe child should be renamed something like `embed_in_parent`. It will force ChromeViewsDelegate to use a non-toplevel widget (so, e.g., the widget can't draw outside of its parents bounds). However, there are plenty of cases where ChromeViewsDelegate will use a non-toplevel widget even though `child` is false - e.g. on Linux, where bubbles stay inside their parent window. Since bubbles on Windows can draw outside of their parent window's bounds, we can't set child=true for bubbles. https://chromiumcodereview.appspot.com/1637383003/diff/80001/ui/views/bubble/... File ui/views/bubble/bubble_delegate.cc (right): https://chromiumcodereview.appspot.com/1637383003/diff/80001/ui/views/bubble/... ui/views/bubble/bubble_delegate.cc:181: // Only close the widget if it's been shown in the first place. Was this necessary to get the test to pass? Otherwise hopefully we can avoid it since I will need to think more about it https://chromiumcodereview.appspot.com/1637383003/diff/80001/ui/views/bubble/... File ui/views/bubble/bubble_delegate_unittest.cc (right): https://chromiumcodereview.appspot.com/1637383003/diff/80001/ui/views/bubble/... ui/views/bubble/bubble_delegate_unittest.cc:289: EXPECT_TRUE(bubble_widget->IsVisible()); OK - so the reason this currently works on Aura is some code in NativeWidgetAura::InitNativeWidget() gfx::NativeView parent = params.parent; if (!params.child) { .. if (parent && parent->type() != ui::wm::WINDOW_TYPE_UNKNOWN) { .. parent = NULL; } .. } if (parent) { parent->AddChild(window_); } else { .. } i.e. for a top-level widget (child = false), the `parent` is removed, and it's not added as a ChildWindow as far as Aura::Window::IsVisible(..) is concerned. So I think we can say here #if defined(USE_AURA) // Bubbles are top-level which, on Aura, causes the parent->child // relationship to be managed by wm::TransientWindowManager // rather than the ui::Layer hierarchy. TransientWindowManager // currently permits a child to be shown when its parent is hidden, // but it shouldn't. This is http://crbug.com/XXXXX. EXPECT_TRUE(bubble_widget->IsVisible()); #else EXPECT_FALSE(bubble_widget->IsVisible()); #endif anchor_widget->Show(); EXPECT_TRUE(bubble_widget->IsVisible());
Have checked the Chrome OS build, could you check Windows before I send for owners? Thanks! https://chromiumcodereview.appspot.com/1637383003/diff/80001/ui/views/bubble/... File ui/views/bubble/bubble_delegate.cc (right): https://chromiumcodereview.appspot.com/1637383003/diff/80001/ui/views/bubble/... ui/views/bubble/bubble_delegate.cc:181: // Only close the widget if it's been shown in the first place. On 2016/03/01 02:42:39, tapted wrote: > Was this necessary to get the test to pass? Otherwise hopefully we can avoid it > since I will need to think more about it Have reverted this. The child = true change prevented the bubble being shown without having shown the anchor, but it still closed on deactivation, so I added the check to prevent it from closing when it wasn't even visible. I think this is actually wrong though because the bubble was active without being visible. https://chromiumcodereview.appspot.com/1637383003/diff/80001/ui/views/bubble/... File ui/views/bubble/bubble_delegate_unittest.cc (right): https://chromiumcodereview.appspot.com/1637383003/diff/80001/ui/views/bubble/... ui/views/bubble/bubble_delegate_unittest.cc:289: EXPECT_TRUE(bubble_widget->IsVisible()); On 2016/03/01 02:42:39, tapted wrote: > OK - so the reason this currently works on Aura is some code in > NativeWidgetAura::InitNativeWidget() > > > gfx::NativeView parent = params.parent; > if (!params.child) { > .. > if (parent && parent->type() != ui::wm::WINDOW_TYPE_UNKNOWN) { > .. > parent = NULL; > } > .. > } > > if (parent) { > parent->AddChild(window_); > } else { > .. > } > > i.e. for a top-level widget (child = false), the `parent` is removed, and it's > not added as a ChildWindow as far as Aura::Window::IsVisible(..) is concerned. > > > So I think we can say here > > #if defined(USE_AURA) > // Bubbles are top-level which, on Aura, causes the parent->child > // relationship to be managed by wm::TransientWindowManager > // rather than the ui::Layer hierarchy. TransientWindowManager > // currently permits a child to be shown when its parent is hidden, > // but it shouldn't. This is http://crbug.com/XXXXX. > EXPECT_TRUE(bubble_widget->IsVisible()); > #else > EXPECT_FALSE(bubble_widget->IsVisible()); > #endif > > anchor_widget->Show(); > EXPECT_TRUE(bubble_widget->IsVisible()); > Done, have also applied the same ifdefs to the IsClosed() and CloseReason checks.
https://chromiumcodereview.appspot.com/1637383003/diff/100001/ui/views/bubble... File ui/views/bubble/bubble_delegate_unittest.cc (right): https://chromiumcodereview.appspot.com/1637383003/diff/100001/ui/views/bubble... ui/views/bubble/bubble_delegate_unittest.cc:288: bubble_widget->Show(); Can we get the tests in sync for the close_reason() check? I guess it might look like bubble->Show() EXPECT_FALSE(anchor->visible) #if aura EXPECT_TRUE(bubble->visible) #else EXPECT_FALSE(bubble->visible) anchor->Show() bubble->Activate() #endif EXPECT_TRUE(bubble->isactive) Then activating the anchor should cause the bubble to get dismissed consistently on all platforms.
PTAL, thank you. https://chromiumcodereview.appspot.com/1637383003/diff/100001/ui/views/bubble... File ui/views/bubble/bubble_delegate_unittest.cc (right): https://chromiumcodereview.appspot.com/1637383003/diff/100001/ui/views/bubble... ui/views/bubble/bubble_delegate_unittest.cc:288: bubble_widget->Show(); On 2016/03/01 07:09:23, tapted wrote: > Can we get the tests in sync for the close_reason() check? I guess it might > look like > > bubble->Show() > EXPECT_FALSE(anchor->visible) > #if aura > EXPECT_TRUE(bubble->visible) > #else > EXPECT_FALSE(bubble->visible) > anchor->Show() > bubble->Activate() > #endif > EXPECT_TRUE(bubble->isactive) > > Then activating the anchor should cause the bubble to get dismissed consistently > on all platforms. Done (without the else statement). I've left the comment in the same place though, is that ok?
lgtm https://chromiumcodereview.appspot.com/1637383003/diff/140001/ui/views/bubble... File ui/views/bubble/bubble_delegate_unittest.cc (right): https://chromiumcodereview.appspot.com/1637383003/diff/140001/ui/views/bubble... ui/views/bubble/bubble_delegate_unittest.cc:291: // Bubbles are top-level which, on Aura, causes the parent->child nit: re-wrap comment
Description was changed from ========== MacViews: Fix views_unittests BubbleDelegateTest.CloseReasons Test added in r360848, fails on Mac as the bubble's parent anchor is not being shown before the bubble is expected to appear - this causes the bubble to be treated as 'hidden' as its parent is not visible. This should actually be expected behaviour on all platforms, so the failure is actually indicating there's a bug on Aura. Fix is to make sure the bubble widget is parented properly with the anchor widget, by indicating the bubble is a child during widget creation. Also add ScopedFakeNSWindowFocus to all tests to simulate focus on the parent, as well as defend against test flakiness which might be caused by window activation issues. BUG=579380 ========== to ========== MacViews: Fix views_unittests BubbleDelegateTest.CloseReasons Test added in r360848, fails on Mac as the bubble's parent anchor is not being shown before the bubble is expected to appear - this causes the bubble to be treated as 'hidden' as its parent is not visible. This should actually be expected behaviour on all platforms, so the failure is actually indicating there's a bug on Aura (see http://crbug/590957). Workaround this for now by showing the bubble's anchor widget first for non-Aura platforms to allow this to pass. Also add ScopedFakeNSWindowFocus to all tests to simulate focus on the parent, as well as defend against test flakiness which might be caused by window activation issues. BUG=579380 ==========
patricialor@chromium.org changed reviewers: + sky@chromium.org
Have also updated the CL description. +sky for OWNERS in ui/views/*, PTAL, thank you. https://chromiumcodereview.appspot.com/1637383003/diff/140001/ui/views/bubble... File ui/views/bubble/bubble_delegate_unittest.cc (right): https://chromiumcodereview.appspot.com/1637383003/diff/140001/ui/views/bubble... ui/views/bubble/bubble_delegate_unittest.cc:291: // Bubbles are top-level which, on Aura, causes the parent->child On 2016/03/03 08:08:50, tapted wrote: > nit: re-wrap comment Done.
https://chromiumcodereview.appspot.com/1637383003/diff/160001/ui/views/bubble... File ui/views/bubble/bubble_delegate_unittest.cc (right): https://chromiumcodereview.appspot.com/1637383003/diff/160001/ui/views/bubble... ui/views/bubble/bubble_delegate_unittest.cc:74: ui::test::ScopedFakeNSWindowFocus fake_focus; oops - this should have an underscore on the end. We should probably be consistent with the comment about this too, like // Ensure tests running in parallel don't steal focus from the Widget on Mac.
sky@google.com changed reviewers: + msw@chromium.org - sky@chromium.org
sky->msw
Please cite 590957 in your CL description's BUG= https://chromiumcodereview.appspot.com/1637383003/diff/160001/ui/views/bubble... File ui/views/bubble/bubble_delegate_unittest.cc (right): https://chromiumcodereview.appspot.com/1637383003/diff/160001/ui/views/bubble... ui/views/bubble/bubble_delegate_unittest.cc:289: EXPECT_FALSE(anchor_widget->IsVisible()); Why don't we just show |anchor_widget| earlier in this test to simplify the expected behavior? You can add a separate test fixture dedicated to demonstrating the defective behavior on aura.
Description was changed from ========== MacViews: Fix views_unittests BubbleDelegateTest.CloseReasons Test added in r360848, fails on Mac as the bubble's parent anchor is not being shown before the bubble is expected to appear - this causes the bubble to be treated as 'hidden' as its parent is not visible. This should actually be expected behaviour on all platforms, so the failure is actually indicating there's a bug on Aura (see http://crbug/590957). Workaround this for now by showing the bubble's anchor widget first for non-Aura platforms to allow this to pass. Also add ScopedFakeNSWindowFocus to all tests to simulate focus on the parent, as well as defend against test flakiness which might be caused by window activation issues. BUG=579380 ========== to ========== MacViews: Fix views_unittests BubbleDelegateTest.CloseReasons Test added in r360848, fails on Mac as the bubble's parent anchor is not being shown before the bubble is expected to appear - this causes the bubble to be treated as 'hidden' as its parent is not visible. This should actually be expected behaviour on all platforms, so the failure is actually indicating there's a bug on Aura (see http://crbug/590957). Workaround this for now by showing the bubble's anchor widget first to allow this to pass for all platforms. Also add ScopedFakeNSWindowFocus to all tests to simulate focus on the parent, as well as defend against test flakiness which might be caused by window activation issues. BUG=579380,590957 ==========
PTAL, thanks. https://chromiumcodereview.appspot.com/1637383003/diff/160001/ui/views/bubble... File ui/views/bubble/bubble_delegate_unittest.cc (right): https://chromiumcodereview.appspot.com/1637383003/diff/160001/ui/views/bubble... ui/views/bubble/bubble_delegate_unittest.cc:74: ui::test::ScopedFakeNSWindowFocus fake_focus; On 2016/03/03 23:34:50, tapted wrote: > oops - this should have an underscore on the end. We should probably be > consistent with the comment about this too, like > > // Ensure tests running in parallel don't steal focus from the Widget on Mac. Done. https://chromiumcodereview.appspot.com/1637383003/diff/160001/ui/views/bubble... ui/views/bubble/bubble_delegate_unittest.cc:289: EXPECT_FALSE(anchor_widget->IsVisible()); On 2016/03/04 00:23:58, msw wrote: > Why don't we just show |anchor_widget| earlier in this test to simplify the > expected behavior? You can add a separate test fixture dedicated to > demonstrating the defective behavior on aura. Done, will add another test fixture for the Aura bug in a separate CL :)
lgtm, thanks
The CQ bit was checked by patricialor@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from tapted@chromium.org Link to the patchset: https://chromiumcodereview.appspot.com/1637383003/#ps180001 (title: "Remove separate behaviour for Aura vs non-Aura.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1637383003/180001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1637383003/180001
Message was sent while issue was closed.
Committed patchset #10 (id:180001)
Message was sent while issue was closed.
Description was changed from ========== MacViews: Fix views_unittests BubbleDelegateTest.CloseReasons Test added in r360848, fails on Mac as the bubble's parent anchor is not being shown before the bubble is expected to appear - this causes the bubble to be treated as 'hidden' as its parent is not visible. This should actually be expected behaviour on all platforms, so the failure is actually indicating there's a bug on Aura (see http://crbug/590957). Workaround this for now by showing the bubble's anchor widget first to allow this to pass for all platforms. Also add ScopedFakeNSWindowFocus to all tests to simulate focus on the parent, as well as defend against test flakiness which might be caused by window activation issues. BUG=579380,590957 ========== to ========== MacViews: Fix views_unittests BubbleDelegateTest.CloseReasons Test added in r360848, fails on Mac as the bubble's parent anchor is not being shown before the bubble is expected to appear - this causes the bubble to be treated as 'hidden' as its parent is not visible. This should actually be expected behaviour on all platforms, so the failure is actually indicating there's a bug on Aura (see http://crbug/590957). Workaround this for now by showing the bubble's anchor widget first to allow this to pass for all platforms. Also add ScopedFakeNSWindowFocus to all tests to simulate focus on the parent, as well as defend against test flakiness which might be caused by window activation issues. BUG=579380,590957 Committed: https://crrev.com/71d4b0f60b78186e0eff2a287a9a2036488836a8 Cr-Commit-Position: refs/heads/master@{#379499} ==========
Message was sent while issue was closed.
Patchset 10 (id:??) landed as https://crrev.com/71d4b0f60b78186e0eff2a287a9a2036488836a8 Cr-Commit-Position: refs/heads/master@{#379499} |
