Re-enable testContextMenuOpenInNewTabFromTallPage on device.

Re-enable testContextMenuOpenInNewTabFromTallPage on device.

Earl Grey is not synchronizing with WKWebView properly. So adding a wait until
condition to wait for context menu to disappear before proceeding.

BUG=681130
Review-Url: https://codereview.chromium.org/2640783005
Cr-Commit-Position: refs/heads/master@{#444901}
Committed: https://chromium.googlesource.com/chromium/src/+/0b662a9cf9d75b3223e9fad83228e8f6d515cfe4

[liaoyuke, 2017-01-19 22:25:11]

liaoyuke@chromium.org changed reviewers:
+ baxley@chromium.org

[liaoyuke, 2017-01-19 22:25:12]

Hi Mike,

Please take a look.

Thank you very much!

[baxley, 2017-01-19 22:27:40]

https://codereview.chromium.org/2640783005/diff/20001/ios/chrome/browser/cont...
File ios/chrome/browser/context_menu/context_menu_egtest.mm (right):

https://codereview.chromium.org/2640783005/diff/20001/ios/chrome/browser/cont...
ios/chrome/browser/context_menu/context_menu_egtest.mm:61: void
WaitForMatcherBecomesNil(id<GREYMatcher> matcher) {
Can we call it "WaitForMatcher"? I think becoming nil applies to the error
argument, rather than the matcher itself, and I think it's an implementation
detail the caller doesnt' need.

https://codereview.chromium.org/2640783005/diff/20001/ios/chrome/browser/cont...
ios/chrome/browser/context_menu/context_menu_egtest.mm:90:
WaitForMatcherBecomesNil(contextMenuItemButton);
Were you able to verify that this fixes the problem? or is it a speculative fix?

https://codereview.chromium.org/2640783005/diff/20001/ios/chrome/browser/cont...
ios/chrome/browser/context_menu/context_menu_egtest.mm:193: -
(void)testContextMenuOpenInNewTabFromTallPage {
Did you have to modify anything to fix this method? Or is it just not flaky
anymore?

[liaoyuke, 2017-01-19 22:47:44]

PTAL.

Thanks!

https://codereview.chromium.org/2640783005/diff/20001/ios/chrome/browser/cont...
File ios/chrome/browser/context_menu/context_menu_egtest.mm (right):

https://codereview.chromium.org/2640783005/diff/20001/ios/chrome/browser/cont...
ios/chrome/browser/context_menu/context_menu_egtest.mm:61: void
WaitForMatcherBecomesNil(id<GREYMatcher> matcher) {
On 2017/01/19 22:27:40, baxley wrote:
> Can we call it "WaitForMatcher"? I think becoming nil applies to the error
> argument, rather than the matcher itself, and I think it's an implementation
> detail the caller doesnt' need.

Acknowledged.

https://codereview.chromium.org/2640783005/diff/20001/ios/chrome/browser/cont...
ios/chrome/browser/context_menu/context_menu_egtest.mm:90:
WaitForMatcherBecomesNil(contextMenuItemButton);
On 2017/01/19 22:27:40, baxley wrote:
> Were you able to verify that this fixes the problem? or is it a speculative
fix?

Speculative. I'll keep an eye on it and once it fails again, I'll revert the CL.

https://codereview.chromium.org/2640783005/diff/20001/ios/chrome/browser/cont...
ios/chrome/browser/context_menu/context_menu_egtest.mm:193: -
(void)testContextMenuOpenInNewTabFromTallPage {
On 2017/01/19 22:27:40, baxley wrote:
> Did you have to modify anything to fix this method? Or is it just not flaky
> anymore?

Acknowledged.

[liaoyuke, 2017-01-19 22:48:19]

The CQ bit was checked by liaoyuke@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-01-19 22:49:01]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[baxley, 2017-01-19 23:00:15]

LGTM

Based on past flake it was ~1/day, which isn't *that* flaky, especially for
devices. Please keep an eye on it, and maybe let the sheriffs know. Thanks!

[liaoyuke, 2017-01-19 23:01:33]

On 2017/01/19 23:00:15, baxley wrote:
> LGTM
> 
> Based on past flake it was ~1/day, which isn't *that* flaky, especially for
> devices. Please keep an eye on it, and maybe let the sheriffs know. Thanks!

Sure! Thank you for reviewing!

[commit-bot: I haz the power, 2017-01-19 23:07:26]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-01-19 23:07:27]

Dry run: This issue passed the CQ dry run.

[liaoyuke, 2017-01-19 23:09:09]

Description was changed from

==========
Re-enable testContextMenuOpenInNewTabFromTallPage on device.

Earl Grey is not synchronizing with WKWebView properly. So adding a wait until
condition to wait for context menu to disappear before proceeding.

BUG=681130
==========

to

==========
Re-enable testContextMenuOpenInNewTabFromTallPage on device.

Earl Grey is not synchronizing with WKWebView properly. So adding a wait until
condition to wait for context menu to disappear before proceeding.

BUG=681130
==========

[liaoyuke, 2017-01-19 23:09:09]

liaoyuke@chromium.org changed reviewers:
+ eugenebut@chromium.org

[liaoyuke, 2017-01-19 23:09:37]

Hi Eugene,

Do you mind taking a quick look for owner's approval?

Thank you very much!

[Eugene But (OOO till 7-30), 2017-01-19 23:19:07]

lgtm

https://codereview.chromium.org/2640783005/diff/40001/ios/chrome/browser/cont...
File ios/chrome/browser/context_menu/context_menu_egtest.mm (right):

https://codereview.chromium.org/2640783005/diff/40001/ios/chrome/browser/cont...
ios/chrome/browser/context_menu/context_menu_egtest.mm:74: // Earl Grey cannot
preperly synchronize with WKWebView, so adding a
I would avoid mentioning WKWebView here. How about this comment for a function?:

Waits for the context menu to disappear. TODO(crbug.com/): Remove this once
EarlGrey is synchronized with context menu.

https://codereview.chromium.org/2640783005/diff/40001/ios/chrome/browser/cont...
ios/chrome/browser/context_menu/context_menu_egtest.mm:76: ConditionBlock
condition = ^{
Could you please move this to WaitForContextMenuDisappeared function.

[liaoyuke, 2017-01-19 23:46:49]

Thank you for the quick response!

https://codereview.chromium.org/2640783005/diff/40001/ios/chrome/browser/cont...
File ios/chrome/browser/context_menu/context_menu_egtest.mm (right):

https://codereview.chromium.org/2640783005/diff/40001/ios/chrome/browser/cont...
ios/chrome/browser/context_menu/context_menu_egtest.mm:74: // Earl Grey cannot
preperly synchronize with WKWebView, so adding a
On 2017/01/19 23:19:07, Eugene But wrote:
> I would avoid mentioning WKWebView here. How about this comment for a
function?:
> 
> Waits for the context menu to disappear. TODO(crbug.com/): Remove this once
> EarlGrey is synchronized with context menu.

Done.

https://codereview.chromium.org/2640783005/diff/40001/ios/chrome/browser/cont...
ios/chrome/browser/context_menu/context_menu_egtest.mm:76: ConditionBlock
condition = ^{
On 2017/01/19 23:19:07, Eugene But wrote:
> Could you please move this to WaitForContextMenuDisappeared function.

Done.

[liaoyuke, 2017-01-19 23:47:08]

The CQ bit was checked by liaoyuke@chromium.org

[liaoyuke, 2017-01-19 23:47:08]

The patchset sent to the CQ was uploaded after l-g-t-m from baxley@chromium.org,
eugenebut@chromium.org
Link to the patchset: https://codereview.chromium.org/2640783005/#ps60001
(title: "Addressed feedback")

[commit-bot: I haz the power, 2017-01-19 23:48:14]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Eugene But (OOO till 7-30), 2017-01-20 00:00:14]

https://codereview.chromium.org/2640783005/diff/60001/ios/chrome/browser/cont...
File ios/chrome/browser/context_menu/context_menu_egtest.mm (right):

https://codereview.chromium.org/2640783005/diff/60001/ios/chrome/browser/cont...
ios/chrome/browser/context_menu/context_menu_egtest.mm:61: // once EarlGrey is
synchronized with context menu.
Could you please document |matcher| argument. Sorry, did not realize that your
function will need it.

[liaoyuke, 2017-01-20 00:00:51]

The CQ bit was unchecked by liaoyuke@chromium.org

[liaoyuke, 2017-01-20 00:15:38]

Patchset #5 (id:80001) has been deleted

[liaoyuke, 2017-01-20 00:16:22]

Changed function name to WaitForContextMenuItemDisappeared.

https://codereview.chromium.org/2640783005/diff/60001/ios/chrome/browser/cont...
File ios/chrome/browser/context_menu/context_menu_egtest.mm (right):

https://codereview.chromium.org/2640783005/diff/60001/ios/chrome/browser/cont...
ios/chrome/browser/context_menu/context_menu_egtest.mm:61: // once EarlGrey is
synchronized with context menu.
On 2017/01/20 00:00:14, Eugene But wrote:
> Could you please document |matcher| argument. Sorry, did not realize that your
> function will need it.

Done.

[liaoyuke, 2017-01-20 00:26:43]

The CQ bit was checked by liaoyuke@chromium.org

[liaoyuke, 2017-01-20 00:26:43]

The patchset sent to the CQ was uploaded after l-g-t-m from baxley@chromium.org,
eugenebut@chromium.org
Link to the patchset: https://codereview.chromium.org/2640783005/#ps100001
(title: "Update function name")

[commit-bot: I haz the power, 2017-01-20 00:27:59]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-01-20 00:54:48]

CQ is committing da patch.

Bot data: {"patchset_id": 100001, "attempt_start_ts": 1484872003284050,
"parent_rev": "a2fd53c7b0a8b9f0c657e94257059879c8fe7a27", "commit_rev":
"0b662a9cf9d75b3223e9fad83228e8f6d515cfe4"}

[commit-bot: I haz the power, 2017-01-20 00:55:22]

Description was changed from

==========
Re-enable testContextMenuOpenInNewTabFromTallPage on device.

Earl Grey is not synchronizing with WKWebView properly. So adding a wait until
condition to wait for context menu to disappear before proceeding.

BUG=681130
==========

to

==========
Re-enable testContextMenuOpenInNewTabFromTallPage on device.

Earl Grey is not synchronizing with WKWebView properly. So adding a wait until
condition to wait for context menu to disappear before proceeding.

BUG=681130

Review-Url: https://codereview.chromium.org/2640783005
Cr-Commit-Position: refs/heads/master@{#444901}
Committed:
https://chromium.googlesource.com/chromium/src/+/0b662a9cf9d75b3223e9fad83228...
==========

[commit-bot: I haz the power, 2017-01-20 00:55:23]

Committed patchset #5 (id:100001) as
https://chromium.googlesource.com/chromium/src/+/0b662a9cf9d75b3223e9fad83228...