Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2706403006:
Check that tab switcher is active/inactive before visibility check. (Closed)
|
Created:
3 years, 10 months ago by kkhorimoto Modified:
3 years, 9 months ago Reviewers:
Eugene But (OOO till 7-30) CC:
chromium-reviews, marq+watch_chromium.org, pkl (ping after 24h if needed), noyau+watch_chromium.org, sdefresne+watch_chromium.org Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionCheck that tab switcher is active/inactive before visibility check.
As shown in the referenced bug, testCloseAllTabs was failing because the
stack view visibility check was checking for a nil stack view. This is
likely occurring because the visibility check was occurring before the
touch event that should show the tab switcher is handled. Adding this
condition will wait for the touch event to be handled before checking
for visibility.
BUG=693517
Review-Url: https://codereview.chromium.org/2706403006
Cr-Commit-Position: refs/heads/master@{#455293}
Committed: https://chromium.googlesource.com/chromium/src/+/389418d676c37f1c98ac289da1b57b68888bd701
Patch Set 1 #
Total comments: 8
Patch Set 2 : separate visibility vs. active checks #
Total comments: 6
Patch Set 3 : moved comment #Patch Set 4 : GREYAssert #Patch Set 5 : fix gn #
Messages
Total messages: 21 (8 generated)
kkhorimoto@chromium.org changed reviewers: + baxley@chromium.org
kkhorimoto@chromium.org changed reviewers: + eugenebut@chromium.org - baxley@chromium.org
https://codereview.chromium.org/2706403006/diff/1/ios/chrome/browser/ui/stack... File ios/chrome/browser/ui/stack_view/stack_view_egtest.mm (right): https://codereview.chromium.org/2706403006/diff/1/ios/chrome/browser/ui/stack... ios/chrome/browser/ui/stack_view/stack_view_egtest.mm:58: visible ? @"active" : @"inactive"]; I'm confused about mixing is_visible and is_active concepts. Do you want to pick one? https://codereview.chromium.org/2706403006/diff/1/ios/chrome/browser/ui/stack... ios/chrome/browser/ui/stack_view/stack_view_egtest.mm:60: () = ^BOOL { Is this done by clang format? If so, could you please file a bug against it. https://codereview.chromium.org/2706403006/diff/1/ios/chrome/browser/ui/stack... ios/chrome/browser/ui/stack_view/stack_view_egtest.mm:61: BOOL isActive = chrome_test_util::GetStackViewController() != nil && nit: no need for |!= nil| https://codereview.chromium.org/2706403006/diff/1/ios/chrome/browser/ui/stack... ios/chrome/browser/ui/stack_view/stack_view_egtest.mm:68: [activeTabSwitcherCondition waitWithTimeout:5.0]; Do you want to use kWaitForUIElementTimeout instead of 5.0?
https://codereview.chromium.org/2706403006/diff/1/ios/chrome/browser/ui/stack... File ios/chrome/browser/ui/stack_view/stack_view_egtest.mm (right): https://codereview.chromium.org/2706403006/diff/1/ios/chrome/browser/ui/stack... ios/chrome/browser/ui/stack_view/stack_view_egtest.mm:58: visible ? @"active" : @"inactive"]; On 2017/02/23 19:11:25, Eugene But wrote: > I'm confused about mixing is_visible and is_active concepts. Do you want to pick > one? separated out into two different checks. https://codereview.chromium.org/2706403006/diff/1/ios/chrome/browser/ui/stack... ios/chrome/browser/ui/stack_view/stack_view_egtest.mm:60: () = ^BOOL { On 2017/02/23 19:11:25, Eugene But wrote: > Is this done by clang format? If so, could you please file a bug against it. Done. https://codereview.chromium.org/2706403006/diff/1/ios/chrome/browser/ui/stack... ios/chrome/browser/ui/stack_view/stack_view_egtest.mm:61: BOOL isActive = chrome_test_util::GetStackViewController() != nil && On 2017/02/23 19:11:25, Eugene But wrote: > nit: no need for |!= nil| Done. https://codereview.chromium.org/2706403006/diff/1/ios/chrome/browser/ui/stack... ios/chrome/browser/ui/stack_view/stack_view_egtest.mm:68: [activeTabSwitcherCondition waitWithTimeout:5.0]; On 2017/02/23 19:11:25, Eugene But wrote: > Do you want to use kWaitForUIElementTimeout instead of 5.0? Done.
https://codereview.chromium.org/2706403006/diff/20001/ios/chrome/browser/ui/s... File ios/chrome/browser/ui/stack_view/stack_view_egtest.mm (right): https://codereview.chromium.org/2706403006/diff/20001/ios/chrome/browser/ui/s... ios/chrome/browser/ui/stack_view/stack_view_egtest.mm:55: void WaitForStackViewActive(bool active) { Do you want to return bool from this method and assert that result is true? https://codereview.chromium.org/2706403006/diff/20001/ios/chrome/browser/ui/s... ios/chrome/browser/ui/stack_view/stack_view_egtest.mm:75: // Verify the visibility of the stack view. Could you please promote this comment to a function comment. Current function comment is not correct as it does not wait.
https://codereview.chromium.org/2706403006/diff/20001/ios/chrome/browser/ui/s... File ios/chrome/browser/ui/stack_view/stack_view_egtest.mm (right): https://codereview.chromium.org/2706403006/diff/20001/ios/chrome/browser/ui/s... ios/chrome/browser/ui/stack_view/stack_view_egtest.mm:55: void WaitForStackViewActive(bool active) { On 2017/03/07 16:08:15, Eugene But wrote: > Do you want to return bool from this method and assert that result is true? The condition we're checking below is dependent on |active|, so we'd need to have two separate functions for active or inactive. Moreover, we'd need to implement the GREYConditions at the callsites instead of having it encapsulated by this function. https://codereview.chromium.org/2706403006/diff/20001/ios/chrome/browser/ui/s... ios/chrome/browser/ui/stack_view/stack_view_egtest.mm:75: // Verify the visibility of the stack view. On 2017/03/07 16:08:15, Eugene But wrote: > Could you please promote this comment to a function comment. Current function > comment is not correct as it does not wait. Done.
https://codereview.chromium.org/2706403006/diff/20001/ios/chrome/browser/ui/s... File ios/chrome/browser/ui/stack_view/stack_view_egtest.mm (right): https://codereview.chromium.org/2706403006/diff/20001/ios/chrome/browser/ui/s... ios/chrome/browser/ui/stack_view/stack_view_egtest.mm:55: void WaitForStackViewActive(bool active) { On 2017/03/07 19:05:03, kkhorimoto_ wrote: > On 2017/03/07 16:08:15, Eugene But wrote: > > Do you want to return bool from this method and assert that result is true? > > The condition we're checking below is dependent on |active|, so we'd need to > have two separate functions for active or inactive. Moreover, we'd need to > implement the GREYConditions at the callsites instead of having it encapsulated > by this function. I did a search for WaitFor methods in EG tests and most of them assert on timeout, which sounds like a reasonable thing to do in this case as well. WaitForStackViewActive should not be not-op if condition is not met. Doing so causes bugs like this: https://bugs.chromium.org/p/chromium/issues/detail?id=685439
https://codereview.chromium.org/2706403006/diff/20001/ios/chrome/browser/ui/s... File ios/chrome/browser/ui/stack_view/stack_view_egtest.mm (right): https://codereview.chromium.org/2706403006/diff/20001/ios/chrome/browser/ui/s... ios/chrome/browser/ui/stack_view/stack_view_egtest.mm:55: void WaitForStackViewActive(bool active) { On 2017/03/07 19:21:12, Eugene But wrote: > On 2017/03/07 19:05:03, kkhorimoto_ wrote: > > On 2017/03/07 16:08:15, Eugene But wrote: > > > Do you want to return bool from this method and assert that result is true? > > > > The condition we're checking below is dependent on |active|, so we'd need to > > have two separate functions for active or inactive. Moreover, we'd need to > > implement the GREYConditions at the callsites instead of having it > encapsulated > > by this function. > I did a search for WaitFor methods in EG tests and most of them assert on > timeout, which sounds like a reasonable thing to do in this case as well. > WaitForStackViewActive should not be not-op if condition is not met. Doing so > causes bugs like this: > https://bugs.chromium.org/p/chromium/issues/detail?id=685439 Fixed
Thank you! Sorry for back and forth. lgtm
The CQ bit was checked by kkhorimoto@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-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-xcode-...) 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 kkhorimoto@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from eugenebut@chromium.org Link to the patchset: https://codereview.chromium.org/2706403006/#ps80001 (title: "fix gn")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 80001, "attempt_start_ts": 1488924425753100,
"parent_rev": "8db99b6d494c3b357108a3ba82c9ad52689615e5", "commit_rev":
"389418d676c37f1c98ac289da1b57b68888bd701"}
Message was sent while issue was closed.
Description was changed from ========== Check that tab switcher is active/inactive before visibility check. As shown in the referenced bug, testCloseAllTabs was failing because the stack view visibility check was checking for a nil stack view. This is likely occurring because the visibility check was occurring before the touch event that should show the tab switcher is handled. Adding this condition will wait for the touch event to be handled before checking for visibility. BUG=693517 ========== to ========== Check that tab switcher is active/inactive before visibility check. As shown in the referenced bug, testCloseAllTabs was failing because the stack view visibility check was checking for a nil stack view. This is likely occurring because the visibility check was occurring before the touch event that should show the tab switcher is handled. Adding this condition will wait for the touch event to be handled before checking for visibility. BUG=693517 Review-Url: https://codereview.chromium.org/2706403006 Cr-Commit-Position: refs/heads/master@{#455293} Committed: https://chromium.googlesource.com/chromium/src/+/389418d676c37f1c98ac289da1b5... ==========
Message was sent while issue was closed.
Committed patchset #5 (id:80001) as https://chromium.googlesource.com/chromium/src/+/389418d676c37f1c98ac289da1b5... |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
