Refactor web shell integration tests utilities.

Refactor web shell integration tests utilities.

Decouple the test code so web view test code can be shared
across the web shell and Chrome.

BUG=606815
Committed: https://crrev.com/695a50d9ba4ec6f761f02783211fc8925d29678a
Cr-Commit-Position: refs/heads/master@{#391949}

[baxley, 2016-05-05 17:01:51]

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

[baxley, 2016-05-05 17:01:52]

I'm going to patch this into Chrome and may need to update it so it works there.
Feel free to take a look now, or wait for me to verify Chrome can use it.

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

https://codereview.chromium.org/1950323002/diff/60001/ios/web/ios_web_shell_t...
ios/web/ios_web_shell_tests.gyp:87: 'target_name':
'ios_web_shell_earl_grey_test_support',
I didn't want to go crazy creating a new target for each type of test_support
(web, web matchers, web shell helpers, web shell matchers, ...), so I put all
earl_grey and web shell specific utilities in this support target, and
everything that is web only, went into ios_web.gyp:ios_web_test_support.

[Eugene But (OOO till 7-30), 2016-05-05 18:01:12]

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

https://codereview.chromium.org/1950323002/diff/60001/ios/web/ios_web_shell_t...
ios/web/ios_web_shell_tests.gyp:87: 'target_name':
'ios_web_shell_earl_grey_test_support',
On 2016/05/05 17:01:52, baxley wrote:
> I didn't want to go crazy creating a new target for each type of test_support
> (web, web matchers, web shell helpers, web shell matchers, ...), so I put all
> earl_grey and web shell specific utilities in this support target, and
> everything that is web only, went into ios_web.gyp:ios_web_test_support.
That's fine, but I think the name should be different. F.e. 'shell/test/app' is
not related to EG and 'public/test/earl_grey/' is not related to Shell.

How about ios_web_uiintegration_tests_support? Dropping ui may not be very good
as well because ios_web has integration tests, which are neither EG, nor KIF.
Thoughts?

https://codereview.chromium.org/1950323002/diff/60001/ios/web/public/test/ear...
File ios/web/public/test/earl_grey/web_view_matchers.mm (right):

https://codereview.chromium.org/1950323002/diff/60001/ios/web/public/test/ear...
ios/web/public/test/earl_grey/web_view_matchers.mm:47: 
Check that |view| is descendant of |webState->GetView()|. If it's not, return
NO.

https://codereview.chromium.org/1950323002/diff/60001/ios/web/public/test/nav...
File ios/web/public/test/navigation_test_util.mm (right):

https://codereview.chromium.org/1950323002/diff/60001/ios/web/public/test/nav...
ios/web/public/test/navigation_test_util.mm:13: // TODO(crbug.com/604902):
Refactor these utilities into objects that can be
Seems like you can now delete this comment

https://codereview.chromium.org/1950323002/diff/60001/ios/web/shell/test/app/...
File ios/web/shell/test/app/web_shell_test_util.h (right):

https://codereview.chromium.org/1950323002/diff/60001/ios/web/shell/test/app/...
ios/web/shell/test/app/web_shell_test_util.h:8: #include
"ios/web/public/web_state/web_state.h"
s/include/import

https://codereview.chromium.org/1950323002/diff/60001/ios/web/shell/test/app/...
File ios/web/shell/test/app/web_shell_test_util.mm (right):

https://codereview.chromium.org/1950323002/diff/60001/ios/web/shell/test/app/...
ios/web/shell/test/app/web_shell_test_util.mm:7: #include <UIKit/UIKit.h>
s/include/import

https://codereview.chromium.org/1950323002/diff/60001/ios/web/shell/test/app/...
ios/web/shell/test/app/web_shell_test_util.mm:15: ViewController*
view_controller = (ViewController*)[
Missed this last time. Use static_cast instead of C-Style cast.

https://codereview.chromium.org/1950323002/diff/80001/ios/web/shell/test/shel...
File ios/web/shell/test/shell_matchers.h (left):

https://codereview.chromium.org/1950323002/diff/80001/ios/web/shell/test/shel...
ios/web/shell/test/shell_matchers.h:10: id<GREYMatcher> backButton();
Why? This is useful stuff....

https://codereview.chromium.org/1950323002/diff/80001/ios/web/shell/test/web_...
File ios/web/shell/test/web_shell_navigation_egtest.mm (right):

https://codereview.chromium.org/1950323002/diff/80001/ios/web/shell/test/web_...
ios/web/shell/test/web_shell_navigation_egtest.mm:73:
selectElementWithMatcher:web::webViewContainingText(response1, webState)]
Can you keep the old web::webViewContainingText(NSString*) which is web shell
specific and use it here. Passing webState every time is inconvenient and hurts
formatting.

https://codereview.chromium.org/1950323002/diff/80001/ios/web/shell/test/web_...
ios/web/shell/test/web_shell_navigation_egtest.mm:83: [[EarlGrey
selectElementWithMatcher:grey_accessibilityLabel(@"Back")]
Why? Old code was more readable and shorter.

https://codereview.chromium.org/1950323002/diff/80001/ios/web/shell/test/web_...
File ios/web/shell/test/web_view_matchers.mm (left):

https://codereview.chromium.org/1950323002/diff/80001/ios/web/shell/test/web_...
ios/web/shell/test/web_view_matchers.mm:42: MatchesBlock matches = ^BOOL(UIView*
view) {
Why removing this? You can jus call matcherForWebViewContainingText:inWebView...

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

I had questions about some of your comments. Everything else I agree with and
will fix.

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

https://codereview.chromium.org/1950323002/diff/60001/ios/web/ios_web_shell_t...
ios/web/ios_web_shell_tests.gyp:87: 'target_name':
'ios_web_shell_earl_grey_test_support',
On 2016/05/05 18:01:11, Eugene But wrote:
> On 2016/05/05 17:01:52, baxley wrote:
> > I didn't want to go crazy creating a new target for each type of
test_support
> > (web, web matchers, web shell helpers, web shell matchers, ...), so I put
all
> > earl_grey and web shell specific utilities in this support target, and
> > everything that is web only, went into ios_web.gyp:ios_web_test_support.
> That's fine, but I think the name should be different. F.e. 'shell/test/app'
is
> not related to EG and 'public/test/earl_grey/' is not related to Shell.
> 
> How about ios_web_uiintegration_tests_support? Dropping ui may not be very
good
> as well because ios_web has integration tests, which are neither EG, nor KIF.
> Thoughts? 

I think it's a difference of what the name means. I made it
ios_web_shell_earl_grey_test_support, as it includes all the test support to run
earl grey tests on the web shell, which is why it includes unrelated code.

While I understand your suggestion, my concern is that
ios_web_uiintegration_tests_support is saying it has UI test support, when it
really has EarlGrey support.

Maybe the problem would probably be better solved if I just split it to smaller
targets, though I'm concerned we'd have too many small targets? I guess that's
not a bad thing...
ios_web_shell_test_support (contain web_shell_test_util)
ios_web.gyp:ios_web_earl_grey_test_support (contain web_view_matchers)
ios_web_shell_earl_grey_test_support (contain shell_matchers)

https://codereview.chromium.org/1950323002/diff/80001/ios/web/shell/test/shel...
File ios/web/shell/test/shell_matchers.h (left):

https://codereview.chromium.org/1950323002/diff/80001/ios/web/shell/test/shel...
ios/web/shell/test/shell_matchers.h:10: id<GREYMatcher> backButton();
On 2016/05/05 18:01:11, Eugene But wrote:
> Why? This is useful stuff....

It's still there, I moved it to ios/web/shell/test/earl_grey/shell_matchers.h.
The diff didn't figure it out.

https://codereview.chromium.org/1950323002/diff/80001/ios/web/shell/test/web_...
File ios/web/shell/test/web_shell_navigation_egtest.mm (right):

https://codereview.chromium.org/1950323002/diff/80001/ios/web/shell/test/web_...
ios/web/shell/test/web_shell_navigation_egtest.mm:73:
selectElementWithMatcher:web::webViewContainingText(response1, webState)]
On 2016/05/05 18:01:11, Eugene But wrote:
> Can you keep the old web::webViewContainingText(NSString*) which is web shell
> specific and use it here. Passing webState every time is inconvenient and
hurts
> formatting.

Just to clarify, you're suggesting to have (I shortened names for brevity):
void shellMatcher(text) in  ios/web/shell/test/[earl_grey]
void shellMatcher(text, WebState) in ios/web/public/test/earl_grey

Then the test would call the former, which would call the latter?

FWIW, I think that sounds fine, I just want to make sure I understand your
suggestion.

https://codereview.chromium.org/1950323002/diff/80001/ios/web/shell/test/web_...
ios/web/shell/test/web_shell_navigation_egtest.mm:83: [[EarlGrey
selectElementWithMatcher:grey_accessibilityLabel(@"Back")]
On 2016/05/05 18:01:11, Eugene But wrote:
> Why? Old code was more readable and shorter.

This was inadvertent, I thought I was editing a different Xcode project. :) I'll
fix it.

https://codereview.chromium.org/1950323002/diff/80001/ios/web/shell/test/web_...
File ios/web/shell/test/web_view_matchers.mm (left):

https://codereview.chromium.org/1950323002/diff/80001/ios/web/shell/test/web_...
ios/web/shell/test/web_view_matchers.mm:42: MatchesBlock matches = ^BOOL(UIView*
view) {
On 2016/05/05 18:01:11, Eugene But wrote:
> Why removing this? You can jus call
matcherForWebViewContainingText:inWebView...

Moving it to ios/web/public/test/earl_grey.

I guess what you're suggesting is to keep this method to simply find the
WebState then pass it to the method that takes the text and the WebState?

[Eugene But (OOO till 7-30), 2016-05-05 18:50:32]

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

https://codereview.chromium.org/1950323002/diff/60001/ios/web/ios_web_shell_t...
ios/web/ios_web_shell_tests.gyp:87: 'target_name':
'ios_web_shell_earl_grey_test_support',
On 2016/05/05 18:33:18, baxley wrote:
> On 2016/05/05 18:01:11, Eugene But wrote:
> > On 2016/05/05 17:01:52, baxley wrote:
> > > I didn't want to go crazy creating a new target for each type of
> test_support
> > > (web, web matchers, web shell helpers, web shell matchers, ...), so I put
> all
> > > earl_grey and web shell specific utilities in this support target, and
> > > everything that is web only, went into ios_web.gyp:ios_web_test_support.
> > That's fine, but I think the name should be different. F.e. 'shell/test/app'
> is
> > not related to EG and 'public/test/earl_grey/' is not related to Shell.
> > 
> > How about ios_web_uiintegration_tests_support? Dropping ui may not be very
> good
> > as well because ios_web has integration tests, which are neither EG, nor
KIF.
> > Thoughts? 
> 
> I think it's a difference of what the name means. I made it
> ios_web_shell_earl_grey_test_support, as it includes all the test support to
run
> earl grey tests on the web shell, which is why it includes unrelated code.
> 
> While I understand your suggestion, my concern is that
> ios_web_uiintegration_tests_support is saying it has UI test support, when it
> really has EarlGrey support.
> 
> Maybe the problem would probably be better solved if I just split it to
smaller
> targets, though I'm concerned we'd have too many small targets? I guess that's
> not a bad thing...
> ios_web_shell_test_support (contain web_shell_test_util)
> ios_web.gyp:ios_web_earl_grey_test_support (contain web_view_matchers)
> ios_web_shell_earl_grey_test_support (contain shell_matchers)
I small targets sounds like a better approach. Do you need more than 2 small
targets (one for EG and one for WebShell)?

https://codereview.chromium.org/1950323002/diff/80001/ios/web/shell/test/shel...
File ios/web/shell/test/shell_matchers.h (left):

https://codereview.chromium.org/1950323002/diff/80001/ios/web/shell/test/shel...
ios/web/shell/test/shell_matchers.h:10: id<GREYMatcher> backButton();
On 2016/05/05 18:33:18, baxley wrote:
> On 2016/05/05 18:01:11, Eugene But wrote:
> > Why? This is useful stuff....
> 
> It's still there, I moved it to ios/web/shell/test/earl_grey/shell_matchers.h.
> The diff didn't figure it out.
Sorry. I can read, I promise...

https://codereview.chromium.org/1950323002/diff/80001/ios/web/shell/test/web_...
File ios/web/shell/test/web_shell_navigation_egtest.mm (right):

https://codereview.chromium.org/1950323002/diff/80001/ios/web/shell/test/web_...
ios/web/shell/test/web_shell_navigation_egtest.mm:73:
selectElementWithMatcher:web::webViewContainingText(response1, webState)]
On 2016/05/05 18:33:18, baxley wrote:
> On 2016/05/05 18:01:11, Eugene But wrote:
> > Can you keep the old web::webViewContainingText(NSString*) which is web
shell
> > specific and use it here. Passing webState every time is inconvenient and
> hurts
> > formatting.
> 
> Just to clarify, you're suggesting to have (I shortened names for brevity):
> void shellMatcher(text) in  ios/web/shell/test/[earl_grey]
> void shellMatcher(text, WebState) in ios/web/public/test/earl_grey
> 
> Then the test would call the former, which would call the latter?
> 
> FWIW, I think that sounds fine, I just want to make sure I understand your
> suggestion.
Correct. |shellMatcher(text)| implementation will call the latter.

https://codereview.chromium.org/1950323002/diff/80001/ios/web/shell/test/web_...
File ios/web/shell/test/web_view_matchers.mm (left):

https://codereview.chromium.org/1950323002/diff/80001/ios/web/shell/test/web_...
ios/web/shell/test/web_view_matchers.mm:42: MatchesBlock matches = ^BOOL(UIView*
view) {
On 2016/05/05 18:33:18, baxley wrote:
> On 2016/05/05 18:01:11, Eugene But wrote:
> > Why removing this? You can jus call
> matcherForWebViewContainingText:inWebView...
> 
> Moving it to ios/web/public/test/earl_grey.
> 
> I guess what you're suggesting is to keep this method to simply find the
> WebState then pass it to the method that takes the text and the WebState?
Yes, sorry for being unclear.

[baxley, 2016-05-05 21:13:53]

Comments addressed. Thanks for the review!

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

https://codereview.chromium.org/1950323002/diff/60001/ios/web/ios_web_shell_t...
ios/web/ios_web_shell_tests.gyp:87: 'target_name':
'ios_web_shell_earl_grey_test_support',
On 2016/05/05 18:50:31, Eugene But wrote:
> On 2016/05/05 18:33:18, baxley wrote:
> > On 2016/05/05 18:01:11, Eugene But wrote:
> > > On 2016/05/05 17:01:52, baxley wrote:
> > > > I didn't want to go crazy creating a new target for each type of
> > test_support
> > > > (web, web matchers, web shell helpers, web shell matchers, ...), so I
put
> > all
> > > > earl_grey and web shell specific utilities in this support target, and
> > > > everything that is web only, went into ios_web.gyp:ios_web_test_support.
> > > That's fine, but I think the name should be different. F.e.
'shell/test/app'
> > is
> > > not related to EG and 'public/test/earl_grey/' is not related to Shell.
> > > 
> > > How about ios_web_uiintegration_tests_support? Dropping ui may not be very
> > good
> > > as well because ios_web has integration tests, which are neither EG, nor
> KIF.
> > > Thoughts? 
> > 
> > I think it's a difference of what the name means. I made it
> > ios_web_shell_earl_grey_test_support, as it includes all the test support to
> run
> > earl grey tests on the web shell, which is why it includes unrelated code.
> > 
> > While I understand your suggestion, my concern is that
> > ios_web_uiintegration_tests_support is saying it has UI test support, when
it
> > really has EarlGrey support.
> > 
> > Maybe the problem would probably be better solved if I just split it to
> smaller
> > targets, though I'm concerned we'd have too many small targets? I guess
that's
> > not a bad thing...
> > ios_web_shell_test_support (contain web_shell_test_util)
> > ios_web.gyp:ios_web_earl_grey_test_support (contain web_view_matchers)
> > ios_web_shell_earl_grey_test_support (contain shell_matchers)
> I small targets sounds like a better approach. Do you need more than 2 small
> targets (one for EG and one for WebShell)?

After offline discussion, I'll leave it as is. Splitting it into logical groups
would be having one set of .h/.mm per target. Better to have just one target. We
can split it up later, as necessary.

https://codereview.chromium.org/1950323002/diff/60001/ios/web/public/test/ear...
File ios/web/public/test/earl_grey/web_view_matchers.mm (right):

https://codereview.chromium.org/1950323002/diff/60001/ios/web/public/test/ear...
ios/web/public/test/earl_grey/web_view_matchers.mm:47: 
On 2016/05/05 18:01:11, Eugene But wrote:
> Check that |view| is descendant of |webState->GetView()|. If it's not, return
> NO.

Done.

https://codereview.chromium.org/1950323002/diff/60001/ios/web/public/test/nav...
File ios/web/public/test/navigation_test_util.mm (right):

https://codereview.chromium.org/1950323002/diff/60001/ios/web/public/test/nav...
ios/web/public/test/navigation_test_util.mm:13: // TODO(crbug.com/604902):
Refactor these utilities into objects that can be
On 2016/05/05 18:01:11, Eugene But wrote:
> Seems like you can now delete this comment

Done.

https://codereview.chromium.org/1950323002/diff/60001/ios/web/shell/test/app/...
File ios/web/shell/test/app/web_shell_test_util.h (right):

https://codereview.chromium.org/1950323002/diff/60001/ios/web/shell/test/app/...
ios/web/shell/test/app/web_shell_test_util.h:8: #include
"ios/web/public/web_state/web_state.h"
On 2016/05/05 18:01:11, Eugene But wrote:
> s/include/import

Done.

https://codereview.chromium.org/1950323002/diff/60001/ios/web/shell/test/app/...
File ios/web/shell/test/app/web_shell_test_util.mm (right):

https://codereview.chromium.org/1950323002/diff/60001/ios/web/shell/test/app/...
ios/web/shell/test/app/web_shell_test_util.mm:7: #include <UIKit/UIKit.h>
On 2016/05/05 18:01:11, Eugene But wrote:
> s/include/import

Done.

https://codereview.chromium.org/1950323002/diff/60001/ios/web/shell/test/app/...
ios/web/shell/test/app/web_shell_test_util.mm:15: ViewController*
view_controller = (ViewController*)[
On 2016/05/05 18:01:11, Eugene But wrote:
> Missed this last time. Use static_cast instead of C-Style cast.

Done.

https://codereview.chromium.org/1950323002/diff/80001/ios/web/shell/test/shel...
File ios/web/shell/test/shell_matchers.h (left):

https://codereview.chromium.org/1950323002/diff/80001/ios/web/shell/test/shel...
ios/web/shell/test/shell_matchers.h:10: id<GREYMatcher> backButton();
On 2016/05/05 18:50:31, Eugene But wrote:
> On 2016/05/05 18:33:18, baxley wrote:
> > On 2016/05/05 18:01:11, Eugene But wrote:
> > > Why? This is useful stuff....
> > 
> > It's still there, I moved it to
ios/web/shell/test/earl_grey/shell_matchers.h.
> > The diff didn't figure it out.
> Sorry. I can read, I promise...

:)

https://codereview.chromium.org/1950323002/diff/80001/ios/web/shell/test/web_...
File ios/web/shell/test/web_shell_navigation_egtest.mm (right):

https://codereview.chromium.org/1950323002/diff/80001/ios/web/shell/test/web_...
ios/web/shell/test/web_shell_navigation_egtest.mm:73:
selectElementWithMatcher:web::webViewContainingText(response1, webState)]
On 2016/05/05 18:50:32, Eugene But wrote:
> On 2016/05/05 18:33:18, baxley wrote:
> > On 2016/05/05 18:01:11, Eugene But wrote:
> > > Can you keep the old web::webViewContainingText(NSString*) which is web
> shell
> > > specific and use it here. Passing webState every time is inconvenient and
> > hurts
> > > formatting.
> > 
> > Just to clarify, you're suggesting to have (I shortened names for brevity):
> > void shellMatcher(text) in  ios/web/shell/test/[earl_grey]
> > void shellMatcher(text, WebState) in ios/web/public/test/earl_grey
> > 
> > Then the test would call the former, which would call the latter?
> > 
> > FWIW, I think that sounds fine, I just want to make sure I understand your
> > suggestion.
> Correct. |shellMatcher(text)| implementation will call the latter.

Done.

https://codereview.chromium.org/1950323002/diff/80001/ios/web/shell/test/web_...
ios/web/shell/test/web_shell_navigation_egtest.mm:83: [[EarlGrey
selectElementWithMatcher:grey_accessibilityLabel(@"Back")]
On 2016/05/05 18:33:18, baxley wrote:
> On 2016/05/05 18:01:11, Eugene But wrote:
> > Why? Old code was more readable and shorter.
> 
> This was inadvertent, I thought I was editing a different Xcode project. :)
I'll
> fix it.

Done.

https://codereview.chromium.org/1950323002/diff/80001/ios/web/shell/test/web_...
File ios/web/shell/test/web_view_matchers.mm (left):

https://codereview.chromium.org/1950323002/diff/80001/ios/web/shell/test/web_...
ios/web/shell/test/web_view_matchers.mm:42: MatchesBlock matches = ^BOOL(UIView*
view) {
On 2016/05/05 18:50:32, Eugene But wrote:
> On 2016/05/05 18:33:18, baxley wrote:
> > On 2016/05/05 18:01:11, Eugene But wrote:
> > > Why removing this? You can jus call
> > matcherForWebViewContainingText:inWebView...
> > 
> > Moving it to ios/web/public/test/earl_grey.
> > 
> > I guess what you're suggesting is to keep this method to simply find the
> > WebState then pass it to the method that takes the text and the WebState?
> Yes, sorry for being unclear. 

Done. Most of this method moved to ios/web/public/test/earl_grey, but I kept the
simple API in ios/web/shell/test/earl_grey/shell_matchers

[Eugene But (OOO till 7-30), 2016-05-05 21:43:41]

lgtm

https://codereview.chromium.org/1950323002/diff/120001/ios/web/ios_web.gyp
File ios/web/ios_web.gyp (right):

https://codereview.chromium.org/1950323002/diff/120001/ios/web/ios_web.gyp#ne...
ios/web/ios_web.gyp:443: 'public/test/http_server_util.h',
Please update gn files as well.

https://codereview.chromium.org/1950323002/diff/120001/ios/web/public/test/na...
File ios/web/public/test/navigation_test_util.h (right):

https://codereview.chromium.org/1950323002/diff/120001/ios/web/public/test/na...
ios/web/public/test/navigation_test_util.h:15: void LoadUrl(web::WebState*
web_state, GURL url);
const GURL&

https://codereview.chromium.org/1950323002/diff/120001/ios/web/shell/test/ear...
File ios/web/shell/test/earl_grey/shell_matchers.mm (right):

https://codereview.chromium.org/1950323002/diff/120001/ios/web/shell/test/ear...
ios/web/shell/test/earl_grey/shell_matchers.mm:9: #include
"ios/web/shell/test/app/web_shell_test_util.h"
s/include/import

https://codereview.chromium.org/1950323002/diff/120001/ios/web/shell/test/nav...
File ios/web/shell/test/navigation_test_util.h (left):

https://codereview.chromium.org/1950323002/diff/120001/ios/web/shell/test/nav...
ios/web/shell/test/navigation_test_util.h:15: void LoadUrl(GURL url);
Maybe you should keep this method and use |void LoadUrl(web::WebState*, const
GURL&);| in the implementation?
Passing web state to LoadUrl seems to be unnecessary burden for a test writer.

[baxley, 2016-05-05 22:03:26]

Comments addressed. Thanks!

https://codereview.chromium.org/1950323002/diff/120001/ios/web/ios_web.gyp
File ios/web/ios_web.gyp (right):

https://codereview.chromium.org/1950323002/diff/120001/ios/web/ios_web.gyp#ne...
ios/web/ios_web.gyp:443: 'public/test/http_server_util.h',
On 2016/05/05 21:43:41, Eugene But wrote:
> Please update gn files as well.

Done.

https://codereview.chromium.org/1950323002/diff/120001/ios/web/public/test/na...
File ios/web/public/test/navigation_test_util.h (right):

https://codereview.chromium.org/1950323002/diff/120001/ios/web/public/test/na...
ios/web/public/test/navigation_test_util.h:15: void LoadUrl(web::WebState*
web_state, GURL url);
On 2016/05/05 21:43:41, Eugene But wrote:
> const GURL&

Done.

https://codereview.chromium.org/1950323002/diff/120001/ios/web/shell/test/ear...
File ios/web/shell/test/earl_grey/shell_matchers.mm (right):

https://codereview.chromium.org/1950323002/diff/120001/ios/web/shell/test/ear...
ios/web/shell/test/earl_grey/shell_matchers.mm:9: #include
"ios/web/shell/test/app/web_shell_test_util.h"
On 2016/05/05 21:43:41, Eugene But wrote:
> s/include/import

Done.

https://codereview.chromium.org/1950323002/diff/120001/ios/web/shell/test/nav...
File ios/web/shell/test/navigation_test_util.h (left):

https://codereview.chromium.org/1950323002/diff/120001/ios/web/shell/test/nav...
ios/web/shell/test/navigation_test_util.h:15: void LoadUrl(GURL url);
On 2016/05/05 21:43:41, Eugene But wrote:
> Maybe you should keep this method and use |void LoadUrl(web::WebState*, const
> GURL&);| in the implementation?
> Passing web state to LoadUrl seems to be unnecessary burden for a test writer.

Mind if I save it for a follow up CL?

[baxley, 2016-05-05 22:03:31]

The CQ bit was checked by baxley@chromium.org

[baxley, 2016-05-05 22:03:31]

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/1950323002/#ps140001
(title: "add gn")

[commit-bot: I haz the power, 2016-05-05 22:03:50]

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

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

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

[commit-bot: I haz the power, 2016-05-05 22:18:03]

Try jobs failed on following builders:
  android_chromium_gn_compile_rel on tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_chro...)

[Eugene But (OOO till 7-30), 2016-05-05 22:22:45]

https://codereview.chromium.org/1950323002/diff/120001/ios/web/shell/test/nav...
File ios/web/shell/test/navigation_test_util.h (left):

https://codereview.chromium.org/1950323002/diff/120001/ios/web/shell/test/nav...
ios/web/shell/test/navigation_test_util.h:15: void LoadUrl(GURL url);
On 2016/05/05 22:03:25, baxley wrote:
> On 2016/05/05 21:43:41, Eugene But wrote:
> > Maybe you should keep this method and use |void LoadUrl(web::WebState*,
const
> > GURL&);| in the implementation?
> > Passing web state to LoadUrl seems to be unnecessary burden for a test
writer.
> 
> Mind if I save it for a follow up CL?

Sure.

[baxley, 2016-05-05 22:25:22]

The CQ bit was checked by baxley@chromium.org

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

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

[commit-bot: I haz the power, 2016-05-05 23:33:36]

Committed patchset #8 (id:140001)

[commit-bot: I haz the power, 2016-05-05 23:35:32]

Description was changed from

==========
Refactor web shell integration tests utilities.

Decouple the test code so web view test code can be shared
across the web shell and Chrome.

BUG=606815
==========

to

==========
Refactor web shell integration tests utilities.

Decouple the test code so web view test code can be shared
across the web shell and Chrome.

BUG=606815

Committed: https://crrev.com/695a50d9ba4ec6f761f02783211fc8925d29678a
Cr-Commit-Position: refs/heads/master@{#391949}
==========

[commit-bot: I haz the power, 2016-05-05 23:35:32]

Patchset 8 (id:??) landed as
https://crrev.com/695a50d9ba4ec6f761f02783211fc8925d29678a
Cr-Commit-Position: refs/heads/master@{#391949}