Refactor wait_util so it isn't coupled to EarlGrey.

Refactor wait_util so it isn't coupled to EarlGrey.

Create utility to wait for condition, else return false. This allows
test code not dependent upon EarlGrey to use wait_util. Also,
update web shell tests to use this utility.

BUG=657886
Committed: https://crrev.com/e598fd1fd26e086580ca81c0a7e682045dd58ffb
Cr-Commit-Position: refs/heads/master@{#427728}

[baxley, 2016-10-25 00:00:52]

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

[baxley, 2016-10-25 00:00:53]

Could you review ios/testing/wait_util first, and let me know if you have
concerns about the overall approach? Thanks!

https://codereview.chromium.org/2448753002/diff/1/ios/testing/earl_grey/wait_...
File ios/testing/earl_grey/wait_util.h (right):

https://codereview.chromium.org/2448753002/diff/1/ios/testing/earl_grey/wait_...
ios/testing/earl_grey/wait_util.h:4: 
I plan to remove this file, after updating Chrome tests to stop using this API
(and use the non-EarlGrey method in this CL) crbug.com/658859

https://codereview.chromium.org/2448753002/diff/1/ios/web/public/test/web_vie...
File ios/web/public/test/web_view_interaction_test_util.mm (right):

https://codereview.chromium.org/2448753002/diff/1/ios/web/public/test/web_vie...
ios/web/public/test/web_view_interaction_test_util.mm:44: return nullptr;
The old implementation issued a GREYAssert, which I don't think is good in
non-EarlGrey code. Let me know if you have a better thing to return from here on
failure.

[Eugene But (OOO till 7-30), 2016-10-25 14:43:39]

Thank you for cleanup!

https://codereview.chromium.org/2448753002/diff/20001/ios/testing/wait_util.h
File ios/testing/wait_util.h (right):

https://codereview.chromium.org/2448753002/diff/20001/ios/testing/wait_util.h...
ios/testing/wait_util.h:7: 
This is now third wait_util file. And WaitUntilConditionOrTimeout conceptually
is not very different from functions inside base/test/ios/wait_util. Should the
function be in base instead and constants in a separate file?

https://codereview.chromium.org/2448753002/diff/20001/ios/testing/wait_util.h...
ios/testing/wait_util.h:26: bool WaitUntilConditionOrTimeout(NSTimeInterval
timeout,
Should we use Chromium types instead (base::TimeDelta)?

https://codereview.chromium.org/2448753002/diff/20001/ios/testing/wait_util.h...
ios/testing/wait_util.h:27: bool (^condition)(void));
Should this be ConditionBlock from block_types.h?

https://codereview.chromium.org/2448753002/diff/20001/ios/testing/wait_util.mm
File ios/testing/wait_util.mm (right):

https://codereview.chromium.org/2448753002/diff/20001/ios/testing/wait_util.m...
ios/testing/wait_util.mm:5: #include "ios/testing/wait_util.h"
s/include/import

https://codereview.chromium.org/2448753002/diff/20001/ios/web/public/test/ear...
File ios/web/public/test/earl_grey/web_view_actions.mm (right):

https://codereview.chromium.org/2448753002/diff/20001/ios/web/public/test/ear...
ios/web/public/test/earl_grey/web_view_actions.mm:67:
testing::kWaitForUIElementTimeout, ^bool() {
Optional nit: You don't need |bool() |

https://codereview.chromium.org/2448753002/diff/20001/ios/web/public/test/web...
File ios/web/public/test/web_view_interaction_test_util.mm (right):

https://codereview.chromium.org/2448753002/diff/20001/ios/web/public/test/web...
ios/web/public/test/web_view_interaction_test_util.mm:82:
testing::kWaitForUIElementTimeout, ^bool() {
Optional nit: You don't need |bool() | here.

https://codereview.chromium.org/2448753002/diff/20001/ios/web/shell/test/earl...
File ios/web/shell/test/earl_grey/shell_matchers.mm (right):

https://codereview.chromium.org/2448753002/diff/20001/ios/web/shell/test/earl...
ios/web/shell/test/earl_grey/shell_matchers.mm:46: NSString* errorMessage =
[NSString
s/errorMessage/error_message

https://codereview.chromium.org/2448753002/diff/20001/ios/web/shell/test/earl...
ios/web/shell/test/earl_grey/shell_matchers.mm:52: ^bool() {
Optional nit (seems to be better formatting):

^{
  return text == base::SysNSStringToUTF8(text_field.text);
}

https://codereview.chromium.org/2448753002/diff/20001/ios/web/shell/test/pdf_...
File ios/web/shell/test/pdf_egtest.mm (right):

https://codereview.chromium.org/2448753002/diff/20001/ios/web/shell/test/pdf_...
ios/web/shell/test/pdf_egtest.mm:30: testing::kWaitForUIElementTimeout, ^bool()
{
Optional nit: You don't need |bool() |

[baxley, 2016-10-25 18:50:05]

Comments addressed (some with questions). PTAL!

https://codereview.chromium.org/2448753002/diff/20001/ios/testing/wait_util.h
File ios/testing/wait_util.h (right):

https://codereview.chromium.org/2448753002/diff/20001/ios/testing/wait_util.h...
ios/testing/wait_util.h:7: 
On 2016/10/25 14:43:38, Eugene But wrote:
> This is now third wait_util file. And WaitUntilConditionOrTimeout conceptually
> is not very different from functions inside base/test/ios/wait_util. Should
the
> function be in base instead and constants in a separate file?

I have a bug to delete the one in ios/testing/earlgrey, which will happen after
this lands and we remove its use downstream

We'd rather not add this code to base, since we don't want to encourage using
WaitUntilCondition. :)

I filed crbug.com/659208, as a follow up where we should fix this. Hopefully we
can remove the waits, but if not we can then consolidate them. WDYT?

https://codereview.chromium.org/2448753002/diff/20001/ios/testing/wait_util.h...
ios/testing/wait_util.h:26: bool WaitUntilConditionOrTimeout(NSTimeInterval
timeout,
On 2016/10/25 14:43:38, Eugene But wrote:
> Should we use Chromium types instead (base::TimeDelta)?

This adds complexity, since the caller has to create and pass in an object, when
all cases just want to pass in a time interval. I think it would be better to
change, if we moved it to base.

If you feel strongly, I can keep the API as is, but use a timer and create
TimeDelta based on the value passed in.

https://codereview.chromium.org/2448753002/diff/20001/ios/testing/wait_util.h...
ios/testing/wait_util.h:27: bool (^condition)(void));
On 2016/10/25 14:43:38, Eugene But wrote:
> Should this be ConditionBlock from block_types.h?

Done.

https://codereview.chromium.org/2448753002/diff/20001/ios/testing/wait_util.mm
File ios/testing/wait_util.mm (right):

https://codereview.chromium.org/2448753002/diff/20001/ios/testing/wait_util.m...
ios/testing/wait_util.mm:5: #include "ios/testing/wait_util.h"
On 2016/10/25 14:43:39, Eugene But wrote:
> s/include/import

Done.

https://codereview.chromium.org/2448753002/diff/20001/ios/web/public/test/ear...
File ios/web/public/test/earl_grey/web_view_actions.mm (right):

https://codereview.chromium.org/2448753002/diff/20001/ios/web/public/test/ear...
ios/web/public/test/earl_grey/web_view_actions.mm:67:
testing::kWaitForUIElementTimeout, ^bool() {
On 2016/10/25 14:43:39, Eugene But wrote:
> Optional nit: You don't need |bool() |

Done.

https://codereview.chromium.org/2448753002/diff/20001/ios/web/public/test/web...
File ios/web/public/test/web_view_interaction_test_util.mm (right):

https://codereview.chromium.org/2448753002/diff/20001/ios/web/public/test/web...
ios/web/public/test/web_view_interaction_test_util.mm:82:
testing::kWaitForUIElementTimeout, ^bool() {
On 2016/10/25 14:43:39, Eugene But wrote:
> Optional nit: You don't need |bool() | here.

Done.

https://codereview.chromium.org/2448753002/diff/20001/ios/web/shell/test/earl...
File ios/web/shell/test/earl_grey/shell_matchers.mm (right):

https://codereview.chromium.org/2448753002/diff/20001/ios/web/shell/test/earl...
ios/web/shell/test/earl_grey/shell_matchers.mm:46: NSString* errorMessage =
[NSString
On 2016/10/25 14:43:39, Eugene But wrote:
> s/errorMessage/error_message

Done.

https://codereview.chromium.org/2448753002/diff/20001/ios/web/shell/test/earl...
ios/web/shell/test/earl_grey/shell_matchers.mm:52: ^bool() {
On 2016/10/25 14:43:39, Eugene But wrote:
> Optional nit (seems to be better formatting):
> 
> ^{
>   return text == base::SysNSStringToUTF8(text_field.text);
> }

This is what git cl format did. Would you rather I create a variable, or add
using testing::kWaitFor... to see if that helps?

https://codereview.chromium.org/2448753002/diff/20001/ios/web/shell/test/pdf_...
File ios/web/shell/test/pdf_egtest.mm (right):

https://codereview.chromium.org/2448753002/diff/20001/ios/web/shell/test/pdf_...
ios/web/shell/test/pdf_egtest.mm:30: testing::kWaitForUIElementTimeout, ^bool()
{
On 2016/10/25 14:43:39, Eugene But wrote:
> Optional nit: You don't need |bool() |

Done.

[Eugene But (OOO till 7-30), 2016-10-25 22:04:18]

lgtm

[baxley, 2016-10-25 22:07:06]

The CQ bit was checked by baxley@chromium.org

[commit-bot: I haz the power, 2016-10-25 22:07:26]

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, 2016-10-25 22:13:30]

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

[commit-bot: I haz the power, 2016-10-25 22:13:31]

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...)

[baxley, 2016-10-26 15:29:04]

The CQ bit was checked by baxley@chromium.org

[baxley, 2016-10-26 15:29:04]

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/2448753002/#ps120001
(title: "compile")

[commit-bot: I haz the power, 2016-10-26 15:29:29]

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, 2016-10-26 17:13:31]

Committed patchset #7 (id:120001)

[commit-bot: I haz the power, 2016-10-26 17:50:47]

Description was changed from

==========
Refactor wait_util so it isn't coupled to EarlGrey.

Create utility to wait for condition, else return false. This allows
test code not dependent upon EarlGrey to use wait_util. Also,
update web shell tests to use this utility.

BUG=657886
==========

to

==========
Refactor wait_util so it isn't coupled to EarlGrey.

Create utility to wait for condition, else return false. This allows
test code not dependent upon EarlGrey to use wait_util. Also,
update web shell tests to use this utility.

BUG=657886

Committed: https://crrev.com/e598fd1fd26e086580ca81c0a7e682045dd58ffb
Cr-Commit-Position: refs/heads/master@{#427728}
==========

[commit-bot: I haz the power, 2016-10-26 17:50:48]

Patchset 7 (id:??) landed as
https://crrev.com/e598fd1fd26e086580ca81c0a7e682045dd58ffb
Cr-Commit-Position: refs/heads/master@{#427728}