Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2409423003:
Move WidgetActivationWaiter to a common place (Closed)
|
Created:
4 years, 2 months ago by Qiang(Joe) Xu Modified:
4 years, 2 months ago Reviewers:
sky CC:
chromium-reviews, tfarina Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionMove WidgetActivationWaiter to a common place
BUG=654967
TEST=none
Committed: https://crrev.com/a2cab6d035bef91d027bbc642304eeeece680f62
Cr-Commit-Position: refs/heads/master@{#424920}
Patch Set 1 #Patch Set 2 : remove run_loop include #
Total comments: 4
Patch Set 3 : based on sky's comments #
Total comments: 6
Patch Set 4 : based on comments #
Messages
Total messages: 26 (15 generated)
The CQ bit was checked by warx@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Description was changed from ========== Move WidgetActivationWaiter to a common place BUG= ========== to ========== Move WidgetActivationWaiter to a common place BUG=654967 TEST=none ==========
warx@chromium.org changed reviewers: + sky@chromium.org
Hi Scott, please take a look, thanks!
https://codereview.chromium.org/2409423003/diff/20001/ui/views/test/widget_te... File ui/views/test/widget_test.cc (right): https://codereview.chromium.org/2409423003/diff/20001/ui/views/test/widget_te... ui/views/test/widget_test.cc:171: EXPECT_NE(active, widget->IsActive()); I think this is error prone for a general class. You should remove the ifdef above for all platforms. https://codereview.chromium.org/2409423003/diff/20001/ui/views/test/widget_te... File ui/views/test/widget_test.h (right): https://codereview.chromium.org/2409423003/diff/20001/ui/views/test/widget_te... ui/views/test/widget_test.h:164: void Wait(); Document what this does.
The CQ bit was checked by warx@chromium.org to run a CQ dry run
Updated. PTAL. Thanks! https://codereview.chromium.org/2409423003/diff/20001/ui/views/test/widget_te... File ui/views/test/widget_test.cc (right): https://codereview.chromium.org/2409423003/diff/20001/ui/views/test/widget_te... ui/views/test/widget_test.cc:171: EXPECT_NE(active, widget->IsActive()); On 2016/10/12 16:05:43, sky wrote: > I think this is error prone for a general class. You should remove the ifdef > above for all platforms. Done. https://codereview.chromium.org/2409423003/diff/20001/ui/views/test/widget_te... File ui/views/test/widget_test.h (right): https://codereview.chromium.org/2409423003/diff/20001/ui/views/test/widget_te... ui/views/test/widget_test.h:164: void Wait(); On 2016/10/12 16:05:43, sky wrote: > Document what this does. Done.
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2409423003/diff/40001/ui/views/test/widget_te... File ui/views/test/widget_test.cc (right): https://codereview.chromium.org/2409423003/diff/40001/ui/views/test/widget_te... ui/views/test/widget_test.cc:159: EXPECT_NE(active, widget->IsActive()); There is no point in this call now. https://codereview.chromium.org/2409423003/diff/40001/ui/views/test/widget_te... File ui/views/test/widget_test.h (right): https://codereview.chromium.org/2409423003/diff/40001/ui/views/test/widget_te... ui/views/test/widget_test.h:158: // Spins a run loop until a Widget's activation reaches the desired state. How about: // Use in tests to wait until a Widget's activation change to a particular value. To use create and call Wait(). https://codereview.chromium.org/2409423003/diff/40001/ui/views/test/widget_te... ui/views/test/widget_test.h:164: // Called to spin the run loop until widget activation changed is observed. How about: Returns when the active status matches that supplied to the constructor. If the active status does not match that of the constructor a RunLoop is used until the active status matches, otherwise this returns immediately.
Hi Scott, thanks for the comments. New patch is uploaded. https://codereview.chromium.org/2409423003/diff/40001/ui/views/test/widget_te... File ui/views/test/widget_test.cc (right): https://codereview.chromium.org/2409423003/diff/40001/ui/views/test/widget_te... ui/views/test/widget_test.cc:159: EXPECT_NE(active, widget->IsActive()); On 2016/10/12 19:23:53, sky wrote: > There is no point in this call now. Done. https://codereview.chromium.org/2409423003/diff/40001/ui/views/test/widget_te... File ui/views/test/widget_test.h (right): https://codereview.chromium.org/2409423003/diff/40001/ui/views/test/widget_te... ui/views/test/widget_test.h:158: // Spins a run loop until a Widget's activation reaches the desired state. On 2016/10/12 19:23:53, sky wrote: > How about: > // Use in tests to wait until a Widget's activation change to a particular > value. To use create and call Wait(). Done. https://codereview.chromium.org/2409423003/diff/40001/ui/views/test/widget_te... ui/views/test/widget_test.h:164: // Called to spin the run loop until widget activation changed is observed. On 2016/10/12 19:23:53, sky wrote: > How about: > Returns when the active status matches that supplied to the constructor. If the > active status does not match that of the constructor a RunLoop is used until the > active status matches, otherwise this returns immediately. done. tks.
LGTM - thanks!
The CQ bit was checked by warx@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: ios-device on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...) ios-simulator on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
The CQ bit was checked by warx@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Description was changed from ========== Move WidgetActivationWaiter to a common place BUG=654967 TEST=none ========== to ========== Move WidgetActivationWaiter to a common place BUG=654967 TEST=none ==========
Message was sent while issue was closed.
Committed patchset #4 (id:60001)
Message was sent while issue was closed.
Description was changed from ========== Move WidgetActivationWaiter to a common place BUG=654967 TEST=none ========== to ========== Move WidgetActivationWaiter to a common place BUG=654967 TEST=none Committed: https://crrev.com/a2cab6d035bef91d027bbc642304eeeece680f62 Cr-Commit-Position: refs/heads/master@{#424920} ==========
Message was sent while issue was closed.
Patchset 4 (id:??) landed as https://crrev.com/a2cab6d035bef91d027bbc642304eeeece680f62 Cr-Commit-Position: refs/heads/master@{#424920} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
