Add web shell matcher for address field.

Add web shell matcher for address field.

Create shared utility to verify the URL (or other text) in the
address field of the web shell.

BUG=609622
Committed: https://crrev.com/95d6dcd5160bc4aca48df76898092f73c9b09c36
Cr-Commit-Position: refs/heads/master@{#392420}

[baxley, 2016-05-09 15:32:18]

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

[baxley, 2016-05-09 15:32:18]

One question in the comments about creating a new gyp target.

https://codereview.chromium.org/1961063002/diff/1/ios/testing/earl_grey/earl_...
File ios/testing/earl_grey/earl_grey_support.gyp (right):

https://codereview.chromium.org/1961063002/diff/1/ios/testing/earl_grey/earl_...
ios/testing/earl_grey/earl_grey_support.gyp:11: #GN ...
I'll add gn for this CL. I just want to make sure adding a new target makes
sense.

I could have skipped the gyp and just included the file where it is used, but
felt weird directly including a file from a more general location, when a lot of
these cases have a target for the more general code. I suspect we'll add other
utilities here, basically for anything that is EarlGrey specific, but will be
needed by the web shell and Chrome.

Let me know if this is okay, or you'd prefer ios/testing/earl_grey_support.h
with no target, then having to include ../testing/earl_grey in the web shell
test support target.

[Eugene But (OOO till 7-30), 2016-05-09 15:59:08]

lgtm.

Some comments which I think should be addressed/discussed can be done in later
CLs. Those I marked with "Can be changed later.".

https://codereview.chromium.org/1961063002/diff/1/ios/testing/earl_grey/earl_...
File ios/testing/earl_grey/earl_grey_support.gyp (right):

https://codereview.chromium.org/1961063002/diff/1/ios/testing/earl_grey/earl_...
ios/testing/earl_grey/earl_grey_support.gyp:11: #GN ...
On 2016/05/09 15:32:18, baxley wrote:
> I'll add gn for this CL. I just want to make sure adding a new target makes
> sense.
> 
> I could have skipped the gyp and just included the file where it is used, but
> felt weird directly including a file from a more general location, when a lot
of
> these cases have a target for the more general code. I suspect we'll add other
> utilities here, basically for anything that is EarlGrey specific, but will be
> needed by the web shell and Chrome.
> 
> Let me know if this is okay, or you'd prefer ios/testing/earl_grey_support.h
> with no target, then having to include ../testing/earl_grey in the web shell
> test support target.
This way is fine with me. Later we can reiterate on this again.

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

https://codereview.chromium.org/1961063002/diff/1/ios/testing/earl_grey/wait_...
ios/testing/earl_grey/wait_util.h:5: namespace earl_grey_support {
Can be changed later.

Everything which lives in ios/web is namespaced as web::, everything that lives
in ios/net is namespaced as net::.

With that I feel that the namespace should be testing. WDYT?

https://codereview.chromium.org/1961063002/diff/1/ios/testing/earl_grey/wait_...
ios/testing/earl_grey/wait_util.h:8: const NSTimeInterval kSpinDelaySeconds =
0.01;
These should be extern. No?

https://codereview.chromium.org/1961063002/diff/1/ios/testing/earl_grey/wait_...
ios/testing/earl_grey/wait_util.h:10: // Constant for timeout in seconds while
waiting for web element.
Can be changed later.

Sounds like this should be a part of ios/web/test, not ios/testing

https://codereview.chromium.org/1961063002/diff/1/ios/testing/earl_grey/wait_...
ios/testing/earl_grey/wait_util.h:11: const NSTimeInterval
kWaitForElementTimeout = 4.0;
kWaitForWebElementTimeout

https://codereview.chromium.org/1961063002/diff/1/ios/web/shell/test/earl_gre...
File ios/web/shell/test/earl_grey/shell_matchers.h (right):

https://codereview.chromium.org/1961063002/diff/1/ios/web/shell/test/earl_gre...
ios/web/shell/test/earl_grey/shell_matchers.h:32: +
(id<GREYMatcher>)matcherForAddressFieldTextEqualTo:(NSString*)text;
s/matcherForAddressFieldTextEqualTo/matcherForAddressFieldContainingText:

1.) Word before argument should describe the argument
2.) For consistency with the method above

https://codereview.chromium.org/1961063002/diff/1/ios/web/shell/test/earl_gre...
File ios/web/shell/test/earl_grey/shell_matchers.mm (right):

https://codereview.chromium.org/1961063002/diff/1/ios/web/shell/test/earl_gre...
ios/web/shell/test/earl_grey/shell_matchers.mm:54: UITextField* textField =
static_cast<UITextField*>(view);
base::mac::ObjCCastStrict

https://codereview.chromium.org/1961063002/diff/1/ios/web/shell/test/earl_gre...
ios/web/shell/test/earl_grey/shell_matchers.mm:57:
dateWithTimeIntervalSinceNow:earl_grey_support::kWaitForElementTimeout];
Can be changed later.

This is repetitive code, which probably should be factored to wait utils.

https://codereview.chromium.org/1961063002/diff/1/ios/web/shell/test/earl_gre...
ios/web/shell/test/earl_grey/shell_matchers.mm:69: [description
Optional NIT: 

[description appendText:@"web view containing "];
[description appendText:text];

https://codereview.chromium.org/1961063002/diff/1/ios/web/shell/test/web_shel...
File ios/web/shell/test/web_shell_navigation_egtest.mm (right):

https://codereview.chromium.org/1961063002/diff/1/ios/web/shell/test/web_shel...
ios/web/shell/test/web_shell_navigation_egtest.mm:70: [[EarlGrey
selectElementWithMatcher:web::addressFieldText(URL1Text)]
Can be changed later.

Sounds like your matcher should take GURL instead of NSString. Do you see any
cases where it actually needs NSString?

[baxley, 2016-05-09 17:11:43]

PTAL, thanks!

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

https://codereview.chromium.org/1961063002/diff/1/ios/testing/earl_grey/wait_...
ios/testing/earl_grey/wait_util.h:5: namespace earl_grey_support {
On 2016/05/09 15:59:08, Eugene But wrote:
> Can be changed later.
> 
> Everything which lives in ios/web is namespaced as web::, everything that
lives
> in ios/net is namespaced as net::.
> 
> With that I feel that the namespace should be testing. WDYT?

That's reasonable. Done.

https://codereview.chromium.org/1961063002/diff/1/ios/testing/earl_grey/wait_...
ios/testing/earl_grey/wait_util.h:8: const NSTimeInterval kSpinDelaySeconds =
0.01;
On 2016/05/09 15:59:08, Eugene But wrote:
> These should be extern. No?

Done.

https://codereview.chromium.org/1961063002/diff/1/ios/testing/earl_grey/wait_...
ios/testing/earl_grey/wait_util.h:10: // Constant for timeout in seconds while
waiting for web element.
On 2016/05/09 15:59:08, Eugene But wrote:
> Can be changed later.
> 
> Sounds like this should be a part of ios/web/test, not ios/testing

The code matched my intention, but the comment is wrong. I left the code as is,
and updated the comment. My intention is for these delays to not be web
specific. For example, shell_matchers is waiting for the address text to be
updated, and nothing in the web. The web does affect this, but it's a matcher
for non web UI. This could also be used by Chrome, as the default UI wait when
we need to poll.

If the core reason for any delay is the web, I'm fine with moving it to
web/public/test. But I don't know  if we can guarantee web stuff is the only
thing that will need this. What do you think?

https://codereview.chromium.org/1961063002/diff/1/ios/testing/earl_grey/wait_...
ios/testing/earl_grey/wait_util.h:11: const NSTimeInterval
kWaitForElementTimeout = 4.0;
On 2016/05/09 15:59:08, Eugene But wrote:
> kWaitForWebElementTimeout

see previous comment.

https://codereview.chromium.org/1961063002/diff/1/ios/web/shell/test/earl_gre...
File ios/web/shell/test/earl_grey/shell_matchers.h (right):

https://codereview.chromium.org/1961063002/diff/1/ios/web/shell/test/earl_gre...
ios/web/shell/test/earl_grey/shell_matchers.h:32: +
(id<GREYMatcher>)matcherForAddressFieldTextEqualTo:(NSString*)text;
On 2016/05/09 15:59:08, Eugene But wrote:
> s/matcherForAddressFieldTextEqualTo/matcherForAddressFieldContainingText:
> 
> 1.) Word before argument should describe the argument
> 2.) For consistency with the method above

I changed to matcherForAddressFieldEqualToText to address your first comment.

Regarding the second point, these APIs don't do the same thing. The first checks
if |text| is anywhere in the web view text. This API checks that |text| is equal
to the text property of the address field.

I think for now it's good to have and equalTo method to be strict on URLs.

https://codereview.chromium.org/1961063002/diff/1/ios/web/shell/test/earl_gre...
File ios/web/shell/test/earl_grey/shell_matchers.mm (right):

https://codereview.chromium.org/1961063002/diff/1/ios/web/shell/test/earl_gre...
ios/web/shell/test/earl_grey/shell_matchers.mm:54: UITextField* textField =
static_cast<UITextField*>(view);
On 2016/05/09 15:59:08, Eugene But wrote:
> base::mac::ObjCCastStrict

Done.

https://codereview.chromium.org/1961063002/diff/1/ios/web/shell/test/earl_gre...
ios/web/shell/test/earl_grey/shell_matchers.mm:57:
dateWithTimeIntervalSinceNow:earl_grey_support::kWaitForElementTimeout];
On 2016/05/09 15:59:08, Eugene But wrote:
> Can be changed later.
> 
> This is repetitive code, which probably should be factored to wait utils.

Acknowledged.

https://codereview.chromium.org/1961063002/diff/1/ios/web/shell/test/earl_gre...
ios/web/shell/test/earl_grey/shell_matchers.mm:69: [description
On 2016/05/09 15:59:08, Eugene But wrote:
> Optional NIT: 
> 
> [description appendText:@"web view containing "];
> [description appendText:text];

Done.

https://codereview.chromium.org/1961063002/diff/1/ios/web/shell/test/web_shel...
File ios/web/shell/test/web_shell_navigation_egtest.mm (right):

https://codereview.chromium.org/1961063002/diff/1/ios/web/shell/test/web_shel...
ios/web/shell/test/web_shell_navigation_egtest.mm:70: [[EarlGrey
selectElementWithMatcher:web::addressFieldText(URL1Text)]
On 2016/05/09 15:59:08, Eugene But wrote:
> Can be changed later.
> 
> Sounds like your matcher should take GURL instead of NSString. Do you see any
> cases where it actually needs NSString?

Okay, I'll wait. I don't know if there's a case where there may be a search
query that isn't a GURL. It also is more tightly scoped to what the method needs
(it needs the text and nothing else).

https://codereview.chromium.org/1961063002/diff/20001/ios/testing/earl_grey/B...
File ios/testing/earl_grey/BUILD.gn (right):

https://codereview.chromium.org/1961063002/diff/20001/ios/testing/earl_grey/B...
ios/testing/earl_grey/BUILD.gn:4: 
I based this on ios/third_party/fishhook/BUILD.gn, which is of similar
complexity (or un-complexity :) ).

[Eugene But (OOO till 7-30), 2016-05-09 17:58:35]

still lgtm

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

https://codereview.chromium.org/1961063002/diff/1/ios/testing/earl_grey/wait_...
ios/testing/earl_grey/wait_util.h:10: // Constant for timeout in seconds while
waiting for web element.
On 2016/05/09 17:11:43, baxley wrote:
> On 2016/05/09 15:59:08, Eugene But wrote:
> > Can be changed later.
> > 
> > Sounds like this should be a part of ios/web/test, not ios/testing
> 
> The code matched my intention, but the comment is wrong. I left the code as
is,
> and updated the comment. My intention is for these delays to not be web
> specific. For example, shell_matchers is waiting for the address text to be
> updated, and nothing in the web. The web does affect this, but it's a matcher
> for non web UI. This could also be used by Chrome, as the default UI wait when
> we need to poll.
> 
> If the core reason for any delay is the web, I'm fine with moving it to
> web/public/test. But I don't know  if we can guarantee web stuff is the only
> thing that will need this. What do you think?
I see. Then kWaitForUIElementTimeout :)

https://codereview.chromium.org/1961063002/diff/60001/ios/testing/earl_grey/w...
File ios/testing/earl_grey/wait_util.h (right):

https://codereview.chromium.org/1961063002/diff/60001/ios/testing/earl_grey/w...
ios/testing/earl_grey/wait_util.h:5: #ifndef
__IOS_TESTING_EARL_GREY_WAIT_UTIL_H__
IOS_TESTING_EARL_GREY_WAIT_UTIL_H_

https://codereview.chromium.org/1961063002/diff/60001/ios/web/ios_web_shell_t...
File ios/web/ios_web_shell_tests.gyp (right):

https://codereview.chromium.org/1961063002/diff/60001/ios/web/ios_web_shell_t...
ios/web/ios_web_shell_tests.gyp:92:
'../testing/earl_grey/earl_grey_support.gyp:earl_grey_support',
Any gn files which needs to be updated?

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

https://codereview.chromium.org/1961063002/diff/60001/ios/web/shell/test/earl...
ios/web/shell/test/earl_grey/shell_matchers.mm:70: [description
[description appendText:@"web view containing "];

[baxley, 2016-05-09 18:42:27]

comments addressed.

https://codereview.chromium.org/1961063002/diff/60001/ios/testing/earl_grey/w...
File ios/testing/earl_grey/wait_util.h (right):

https://codereview.chromium.org/1961063002/diff/60001/ios/testing/earl_grey/w...
ios/testing/earl_grey/wait_util.h:5: #ifndef
__IOS_TESTING_EARL_GREY_WAIT_UTIL_H__
On 2016/05/09 17:58:35, Eugene But wrote:
> IOS_TESTING_EARL_GREY_WAIT_UTIL_H_

Done.

https://codereview.chromium.org/1961063002/diff/60001/ios/web/ios_web_shell_t...
File ios/web/ios_web_shell_tests.gyp (right):

https://codereview.chromium.org/1961063002/diff/60001/ios/web/ios_web_shell_t...
ios/web/ios_web_shell_tests.gyp:92:
'../testing/earl_grey/earl_grey_support.gyp:earl_grey_support',
On 2016/05/09 17:58:35, Eugene But wrote:
> Any gn files which needs to be updated?

Not yet.

I chatted with Sylvain, and he is waiting for all the gyp stuff to be complete
for XCTests, then he can port it to GN. The only target that is dependent upon
this is ios_web_shell_earl_grey_test_support, which hasn't been created in GN
yet.

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

https://codereview.chromium.org/1961063002/diff/60001/ios/web/shell/test/earl...
ios/web/shell/test/earl_grey/shell_matchers.mm:70: [description
On 2016/05/09 17:58:35, Eugene But wrote:
> [description appendText:@"web view containing "];

Gah. Feel free to preface these comments with "Hey, Moron:" :)

Done.

[baxley, 2016-05-09 18:42:32]

The CQ bit was checked by baxley@chromium.org

[baxley, 2016-05-09 18:42:33]

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/1961063002/#ps80001
(title: "s/Element/UIElement")

[commit-bot: I haz the power, 2016-05-09 18:43:02]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1961063002/80001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1961063002/80001

[commit-bot: I haz the power, 2016-05-09 20:59:41]

Committed patchset #5 (id:80001)

[commit-bot: I haz the power, 2016-05-09 21:01:44]

Description was changed from

==========
Add web shell matcher for address field.

Create shared utility to verify the URL (or other text) in the
address field of the web shell.

BUG=609622
==========

to

==========
Add web shell matcher for address field.

Create shared utility to verify the URL (or other text) in the
address field of the web shell.

BUG=609622

Committed: https://crrev.com/95d6dcd5160bc4aca48df76898092f73c9b09c36
Cr-Commit-Position: refs/heads/master@{#392420}
==========

[commit-bot: I haz the power, 2016-05-09 21:01:45]

Patchset 5 (id:??) landed as
https://crrev.com/95d6dcd5160bc4aca48df76898092f73c9b09c36
Cr-Commit-Position: refs/heads/master@{#392420}