Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1891823004:
MacViews: Fix BubbleDialogDelegateTest.CloseMethods (Closed)
|
Created:
4 years, 8 months ago by Patti Lor Modified:
4 years, 8 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 BubbleDialogDelegateTest.CloseMethods
BubbleDialogDelegateTest.CloseMethods, added in http://crrev.com/1809933003
(r382045) fails on MacViews because bubbles cannot be shown unless their parents
are already visible. This is expected behaviour, so fix it by showing the parent
anchor widget immediately after creation (which affects all tests calling
CreateTestWidget()).
Note this test passes on Aura because of http://crbug.com/590957.
BUG=590957, 600921
Committed: https://crrev.com/bfe263607137b4c4af5cb42fcf9a2d6cb7cc4183
Cr-Commit-Position: refs/heads/master@{#388720}
Patch Set 1 #
Total comments: 2
Patch Set 2 : Move call to Widget::Show() to CreateTestWidget(). #Messages
Total messages: 18 (7 generated)
patricialor@chromium.org changed reviewers: + tapted@chromium.org
PTAL Trent! It's a one line fix for the m51 test regression bug (crbug.com/600921).
https://codereview.chromium.org/1891823004/diff/1/ui/views/bubble/bubble_dial... File ui/views/bubble/bubble_dialog_delegate_unittest.cc (right): https://codereview.chromium.org/1891823004/diff/1/ui/views/bubble/bubble_dial... ui/views/bubble/bubble_dialog_delegate_unittest.cc:305: bubble_widget->Show(); hum - this should need it too. I guess the particular things being tested don't require the widget to be shown since the events are send directly. But that might mean we are missing out on test coverage for a lot of these, and things are just passing by chance. Can you experiment with calling Show() inside CreateTestWidget(), and removing the calls to anchor_widget->Show() from all the tests? There might be a regression, or perhaps a test explicitly wants a hidden parent, and to show it at a particular time. But I had a quick skim and it looks like the default case should really be to show the widgets being created by CreateTestWidget() by default.
Description was changed from ========== MacViews: Fix BubbleDialogDelegateTest.CloseMethods BubbleDialogDelegateTest.CloseMethods, added in crrev.com/1809933003 (r382045) fails on MacViews because bubbles cannot be shown unless their parents are already visible. This is expected behaviour, so fix it by showing the parent anchor widget first. Note this test passes on Aura because of crbug.com/590957. BUG=590957, 600921 ========== to ========== MacViews: Fix BubbleDialogDelegateTest.CloseMethods BubbleDialogDelegateTest.CloseMethods, added in http://crrev.com/1809933003 (r382045) fails on MacViews because bubbles cannot be shown unless their parents are already visible. This is expected behaviour, so fix it by showing the parent anchor widget immediately after creation (which affects all tests calling CreateTestWidget()). Note this test passes on Aura because of http://crbug.com/590957. BUG=590957, 600921 ==========
Yep, I don't think any of the current tests rely on having the anchor hidden at first - have updated the comment on CreateTestWidget() as well. https://codereview.chromium.org/1891823004/diff/1/ui/views/bubble/bubble_dial... File ui/views/bubble/bubble_dialog_delegate_unittest.cc (right): https://codereview.chromium.org/1891823004/diff/1/ui/views/bubble/bubble_dial... ui/views/bubble/bubble_dialog_delegate_unittest.cc:305: bubble_widget->Show(); On 2016/04/15 00:47:27, tapted wrote: > hum - this should need it too. I guess the particular things being tested don't > require the widget to be shown since the events are send directly. But that > might mean we are missing out on test coverage for a lot of these, and things > are just passing by chance. > > Can you experiment with calling Show() inside CreateTestWidget(), and removing > the calls to anchor_widget->Show() from all the tests? > > There might be a regression, or perhaps a test explicitly wants a hidden parent, > and to show it at a particular time. But I had a quick skim and it looks like > the default case should really be to show the widgets being created by > CreateTestWidget() by default. Done.
lgtm
patricialor@chromium.org changed reviewers: + sky@chromium.org
PTAL sky for owners review in ui/views. Thanks!
LGTM
The CQ bit was checked by patricialor@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1891823004/20001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1891823004/20001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_chromium_rel_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by patricialor@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1891823004/20001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1891823004/20001
Message was sent while issue was closed.
Committed patchset #2 (id:20001)
Message was sent while issue was closed.
Description was changed from ========== MacViews: Fix BubbleDialogDelegateTest.CloseMethods BubbleDialogDelegateTest.CloseMethods, added in http://crrev.com/1809933003 (r382045) fails on MacViews because bubbles cannot be shown unless their parents are already visible. This is expected behaviour, so fix it by showing the parent anchor widget immediately after creation (which affects all tests calling CreateTestWidget()). Note this test passes on Aura because of http://crbug.com/590957. BUG=590957, 600921 ========== to ========== MacViews: Fix BubbleDialogDelegateTest.CloseMethods BubbleDialogDelegateTest.CloseMethods, added in http://crrev.com/1809933003 (r382045) fails on MacViews because bubbles cannot be shown unless their parents are already visible. This is expected behaviour, so fix it by showing the parent anchor widget immediately after creation (which affects all tests calling CreateTestWidget()). Note this test passes on Aura because of http://crbug.com/590957. BUG=590957, 600921 Committed: https://crrev.com/bfe263607137b4c4af5cb42fcf9a2d6cb7cc4183 Cr-Commit-Position: refs/heads/master@{#388720} ==========
Message was sent while issue was closed.
Patchset 2 (id:??) landed as https://crrev.com/bfe263607137b4c4af5cb42fcf9a2d6cb7cc4183 Cr-Commit-Position: refs/heads/master@{#388720} |
