Web shell test to go back and forward.

Web shell test to go back and forward.

Includes utilities for the simple local html server, and to verify text
within a WKWebView. Tests and utilities are written in Earl Grey.

BUG=604049, 604050
Committed: https://crrev.com/46f30edfad269c5e85aa22c3ff067df2548a1022
Cr-Commit-Position: refs/heads/master@{#389157}

[baxley, 2016-04-15 23:57:47]

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

[baxley, 2016-04-15 23:57:48]

I added a few comments to patch 1 for clarification, but here's the summary:
- html_response_provider is being moved upstream so the web shell can use this
code.
- This patch exposes WebController, which we don't want to do. We'll expose a
web API to do what this test needs before landing this.


Feel free to do a high-level pass first, to discuss the naming/location of
utilities as this will likely set a precedent. I don't think we'll get it
perfect, and it would be better to move quickly, so we should just make a best
effort right now. Once this CL is finalized I'll put it into a doc.

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

https://codereview.chromium.org/1890333002/diff/1/ios/web/ios_web.gyp#newcode394
ios/web/ios_web.gyp:394: '../../base/base.gyp:test_support_base',
The two additions to this section were needed when using ios_web_test_support,
so I added them as dependencies here.

https://codereview.chromium.org/1890333002/diff/1/ios/web/public/test/respons...
File ios/web/public/test/response_providers/html_response_provider.h (right):

https://codereview.chromium.org/1890333002/diff/1/ios/web/public/test/respons...
ios/web/public/test/response_providers/html_response_provider.h:6: #define
IOS_WEB_PUBLIC_TEST_RESPONSE_PROVIDERS_HTML_RESPONSE_PROVIDER_H_
This file is being moved upstream so it can be used by the web shell tests.

https://codereview.chromium.org/1890333002/diff/1/ios/web/public/test/respons...
File ios/web/public/test/response_providers/html_response_provider.mm (right):

https://codereview.chromium.org/1890333002/diff/1/ios/web/public/test/respons...
ios/web/public/test/response_providers/html_response_provider.mm:3: // found in
the LICENSE file.
This file is being moved upstream so it can be used by the web shell tests.

https://codereview.chromium.org/1890333002/diff/1/ios/web/shell/test/utils/eg...
File ios/web/shell/test/utils/eg_util.h (right):

https://codereview.chromium.org/1890333002/diff/1/ios/web/shell/test/utils/eg...
ios/web/shell/test/utils/eg_util.h:13: CRWWebController*
GetCurrentWebController();
eg_util.h|mm will go away before this lands. We will expose some web APIs to do
what is needed (execute JS for this CL).

https://codereview.chromium.org/1890333002/diff/1/ios/web/shell/view_controll...
File ios/web/shell/view_controller.mm (right):

https://codereview.chromium.org/1890333002/diff/1/ios/web/shell/view_controll...
ios/web/shell/view_controller.mm:66: - (CRWWebController*)webController {
this should go away before this lands.

[Eugene But (OOO till 7-30), 2016-04-16 00:38:41]

https://codereview.chromium.org/1890333002/diff/20001/ios/web/public/test/htt...
File ios/web/public/test/http_server_util.h (right):

https://codereview.chromium.org/1890333002/diff/20001/ios/web/public/test/htt...
ios/web/public/test/http_server_util.h:10: #import
"ios/web/public/test/http_server.h"
These 2 includes are unused.

https://codereview.chromium.org/1890333002/diff/20001/ios/web/public/test/htt...
ios/web/public/test/http_server_util.h:15: namespace http_server_util {
I don't think we need 3rd level of namespaces.

https://codereview.chromium.org/1890333002/diff/20001/ios/web/public/test/htt...
File ios/web/public/test/http_server_util.mm (right):

https://codereview.chromium.org/1890333002/diff/20001/ios/web/public/test/htt...
ios/web/public/test/http_server_util.mm:21: }  // namespace web
Order of closing namespaces should be backwards

https://codereview.chromium.org/1890333002/diff/20001/ios/web/shell/test/util...
File ios/web/shell/test/utils/eg_util.h (right):

https://codereview.chromium.org/1890333002/diff/20001/ios/web/shell/test/util...
ios/web/shell/test/utils/eg_util.h:10: namespace earl_grey_util {
This is very generic name, which does not provide any restrictions of what
should be put here. What other API do you plan to have in this file? Maybe name
it web_state_test_util?

https://codereview.chromium.org/1890333002/diff/20001/ios/web/shell/test/util...
ios/web/shell/test/utils/eg_util.h:13: CRWWebController*
GetCurrentWebController();
WebController is private part of web. This should return WebState instead.

https://codereview.chromium.org/1890333002/diff/20001/ios/web/shell/test/util...
File ios/web/shell/test/utils/eg_util.mm (right):

https://codereview.chromium.org/1890333002/diff/20001/ios/web/shell/test/util...
ios/web/shell/test/utils/eg_util.mm:12: ViewController* vc = (ViewController*)[
From Style Guide:

Readers should be able to infer the variable type from the name, but do not use
Hungarian notation for syntactic attributes, such as the static type of a
variable (int or pointer). Give as descriptive a name as possible, within
reason.

Please use more descriptive name than vc.

https://codereview.chromium.org/1890333002/diff/20001/ios/web/shell/test/util...
ios/web/shell/test/utils/eg_util.mm:15: CRWWebController* wc = [vc
webController];
NIT: Drop this variable

https://codereview.chromium.org/1890333002/diff/20001/ios/web/shell/test/util...
File ios/web/shell/test/utils/web_view_egutil.mm (right):

https://codereview.chromium.org/1890333002/diff/20001/ios/web/shell/test/util...
ios/web/shell/test/utils/web_view_egutil.mm:37: __block id script = nil;
This is not |script|...

https://codereview.chromium.org/1890333002/diff/20001/ios/web/shell/test/util...
ios/web/shell/test/utils/web_view_egutil.mm:42: script = ^(NSString*
evaluationResult, NSError* error) {
Please log |error| if any.

https://codereview.chromium.org/1890333002/diff/20001/ios/web/shell/test/util...
ios/web/shell/test/utils/web_view_egutil.mm:70:
base::scoped_nsobject<GREYElementMatcherBlock> matcherBlock(
base::mac::ScopedBlock

https://codereview.chromium.org/1890333002/diff/20001/ios/web/shell/test/util...
ios/web/shell/test/utils/web_view_egutil.mm:72: descriptionBlock:describe]
retain]);
Blocks should be copied, not retained.

https://codereview.chromium.org/1890333002/diff/20001/ios/web/shell/test/util...
ios/web/shell/test/utils/web_view_egutil.mm:72: descriptionBlock:describe]
retain]);
This is a memory leak

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

https://codereview.chromium.org/1890333002/diff/20001/ios/web/shell/test/web_...
ios/web/shell/test/web_shell_navigation_egtest.mm:7: #import <WebKit/WebKit.h>
Please sort includes alphabetically

https://codereview.chromium.org/1890333002/diff/20001/ios/web/shell/test/web_...
ios/web/shell/test/web_shell_navigation_egtest.mm:54: const GURL secondGURL =
web::test::HttpServer::MakeUrl("http://secondURL");
Optional NIT: s/secondGURL/secondURL

https://codereview.chromium.org/1890333002/diff/20001/ios/web/shell/test/web_...
ios/web/shell/test/web_shell_navigation_egtest.mm:55: NSString* secondURL =
base::SysUTF8ToNSString(secondGURL.spec().c_str());
Optional NIT: s/secondURL/secondURLSpec

https://codereview.chromium.org/1890333002/diff/20001/ios/web/shell/test/web_...
ios/web/shell/test/web_shell_navigation_egtest.mm:55: NSString* secondURL =
base::SysUTF8ToNSString(secondGURL.spec().c_str());
Drop |.c_str()|. SysUTF8ToNSString takes std::string

https://codereview.chromium.org/1890333002/diff/20001/ios/web/shell/test/web_...
ios/web/shell/test/web_shell_navigation_egtest.mm:61: [[EarlGrey
Can we group these methods into a single "enter URL" method? This seems to be a
common thing to do. Even in this test.

https://codereview.chromium.org/1890333002/diff/20001/ios/web/shell/test/web_...
ios/web/shell/test/web_shell_navigation_egtest.mm:62:
selectElementWithMatcher:grey_accessibilityLabel(
NIT: Can you move matcher to a separate variable to make this more readable?

https://codereview.chromium.org/1890333002/diff/20001/ios/web/shell/test/web_...
ios/web/shell/test/web_shell_navigation_egtest.mm:65: [[EarlGrey
selectElementWithMatcher:grey_accessibilityLabel(@"Clear text")]
Please make constant for "Clear Text"

https://codereview.chromium.org/1890333002/diff/20001/ios/web/shell/test/web_...
ios/web/shell/test/web_shell_navigation_egtest.mm:68:
selectElementWithMatcher:grey_accessibilityLabel(
NIT: Separate variable for matcher?

https://codereview.chromium.org/1890333002/diff/20001/ios/web/shell/view_cont...
File ios/web/shell/view_controller.h (right):

https://codereview.chromium.org/1890333002/diff/20001/ios/web/shell/view_cont...
ios/web/shell/view_controller.h:36: @interface ViewController
(ExposedForTesting)
I don't see any issues with making WebState a public property. WebController
should not be exposed, more over it will be completely removed from WebShell

[Eugene But (OOO till 7-30), 2016-04-16 02:00:51]

https://codereview.chromium.org/1890333002/diff/20001/ios/web/shell/test/util...
File ios/web/shell/test/utils/web_view_egutil.mm (right):

https://codereview.chromium.org/1890333002/diff/20001/ios/web/shell/test/util...
ios/web/shell/test/utils/web_view_egutil.mm:70:
base::scoped_nsobject<GREYElementMatcherBlock> matcherBlock(
On 2016/04/16 00:38:40, Eugene But wrote:
> base::mac::ScopedBlock
Oh, this is not a block... nevermind....

https://codereview.chromium.org/1890333002/diff/20001/ios/web/shell/test/util...
ios/web/shell/test/utils/web_view_egutil.mm:72: descriptionBlock:describe]
retain]);
On 2016/04/16 00:38:40, Eugene But wrote:
> Blocks should be copied, not retained.
Ignore this as well :)

EG is so confusing. They use same name for completely different things....
What's more interesting those different things are a part of the same method :(

[rohitrao (ping after 24h), 2016-04-18 11:44:25]

My high-level takeaway is that I have no idea what's going on here =)  I may
grab you for a half hour today so that you can teach me how EG works.

https://codereview.chromium.org/1890333002/diff/20001/ios/web/shell/test/util...
File ios/web/shell/test/utils/web_view_egutil.mm (right):

https://codereview.chromium.org/1890333002/diff/20001/ios/web/shell/test/util...
ios/web/shell/test/utils/web_view_egutil.mm:72: descriptionBlock:describe]
retain]);
Isn't this still a leak?  Alloc/init will return a retained object, so there
should be no need to retain it again before putting it in a scoped_nsobject?

[baxley, 2016-04-18 13:50:55]

On 2016/04/18 11:44:25, rohitrao wrote:
> My high-level takeaway is that I have no idea what's going on here =)  I may
> grab you for a half hour today so that you can teach me how EG works.
That makes sense. Let me finish going through Eugene's comments, since some of
them may also be part of the discussion. I'll ping you offline when I get there.

> 
>
https://codereview.chromium.org/1890333002/diff/20001/ios/web/shell/test/util...
> File ios/web/shell/test/utils/web_view_egutil.mm (right):
> 
>
https://codereview.chromium.org/1890333002/diff/20001/ios/web/shell/test/util...
> ios/web/shell/test/utils/web_view_egutil.mm:72: descriptionBlock:describe]
> retain]);
> Isn't this still a leak?  Alloc/init will return a retained object, so there
> should be no need to retain it again before putting it in a scoped_nsobject?
I'm looking at this.

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

https://codereview.chromium.org/1890333002/diff/20001/ios/web/shell/test/util...
File ios/web/shell/test/utils/web_view_egutil.mm (right):

https://codereview.chromium.org/1890333002/diff/20001/ios/web/shell/test/util...
ios/web/shell/test/utils/web_view_egutil.mm:72: descriptionBlock:describe]
retain]);
On 2016/04/18 11:44:25, rohitrao wrote:
> Isn't this still a leak?  Alloc/init will return a retained object, so there
> should be no need to retain it again before putting it in a scoped_nsobject?
"Ignore this as well" was related to copy vs. retain (though I would file a bug
against EG, because the API is very confusing). It is still a leak.

[baxley, 2016-04-21 16:09:18]

I made quite a few changes, here is a summary of a few changes, to aid the
review...

Added the method to LoadURL via the NavigationManager, which greatly simplified
the test code, and a lot of our concerns and ideas to create complicated
matchers went away. We'll see as we add more tests if anything needs to be done.

Switched to WebState::ExecuteJavaScript to get the text in the webview.

Exposed webState in the ViewController as a property.

Split up html_response_provider into two separate files for the two classes
(html_response_provider and html_response_provider_impl). I also had to add a
copy constructor to one of the structs. Other than that, it's a copy of the
original code.

Moved eg_util to navigation_egutil. I think this will eventually get turned into
classes that can be shared across Chrome and the web shell, for which I have a
bug (crbug.com/604902). I'm concerned doing that design work now would add too
much complexity to this CL.


PTAL!

https://codereview.chromium.org/1890333002/diff/20001/ios/web/public/test/htt...
File ios/web/public/test/http_server_util.h (right):

https://codereview.chromium.org/1890333002/diff/20001/ios/web/public/test/htt...
ios/web/public/test/http_server_util.h:10: #import
"ios/web/public/test/http_server.h"
On 2016/04/16 00:38:40, Eugene But wrote:
> These 2 includes are unused.

Done.

https://codereview.chromium.org/1890333002/diff/20001/ios/web/public/test/htt...
ios/web/public/test/http_server_util.h:15: namespace http_server_util {
On 2016/04/16 00:38:40, Eugene But wrote:
> I don't think we need 3rd level of namespaces.

Done.

https://codereview.chromium.org/1890333002/diff/20001/ios/web/public/test/htt...
File ios/web/public/test/http_server_util.mm (right):

https://codereview.chromium.org/1890333002/diff/20001/ios/web/public/test/htt...
ios/web/public/test/http_server_util.mm:21: }  // namespace web
On 2016/04/16 00:38:40, Eugene But wrote:
> Order of closing namespaces should be backwards

Done.

https://codereview.chromium.org/1890333002/diff/20001/ios/web/shell/test/util...
File ios/web/shell/test/utils/eg_util.h (right):

https://codereview.chromium.org/1890333002/diff/20001/ios/web/shell/test/util...
ios/web/shell/test/utils/eg_util.h:10: namespace earl_grey_util {
On 2016/04/16 00:38:40, Eugene But wrote:
> This is very generic name, which does not provide any restrictions of what
> should be put here. What other API do you plan to have in this file? Maybe
name
> it web_state_test_util?

moved to navigation_egutil

https://codereview.chromium.org/1890333002/diff/20001/ios/web/shell/test/util...
ios/web/shell/test/utils/eg_util.h:13: CRWWebController*
GetCurrentWebController();
On 2016/04/16 00:38:40, Eugene But wrote:
> WebController is private part of web. This should return WebState instead.

Done.

https://codereview.chromium.org/1890333002/diff/20001/ios/web/shell/test/util...
File ios/web/shell/test/utils/eg_util.mm (right):

https://codereview.chromium.org/1890333002/diff/20001/ios/web/shell/test/util...
ios/web/shell/test/utils/eg_util.mm:12: ViewController* vc = (ViewController*)[
On 2016/04/16 00:38:40, Eugene But wrote:
> From Style Guide:
> 
> Readers should be able to infer the variable type from the name, but do not
use
> Hungarian notation for syntactic attributes, such as the static type of a
> variable (int or pointer). Give as descriptive a name as possible, within
> reason.
> 
> Please use more descriptive name than vc.

Done.

https://codereview.chromium.org/1890333002/diff/20001/ios/web/shell/test/util...
ios/web/shell/test/utils/eg_util.mm:15: CRWWebController* wc = [vc
webController];
On 2016/04/16 00:38:40, Eugene But wrote:
> NIT: Drop this variable

Acknowledged.

https://codereview.chromium.org/1890333002/diff/20001/ios/web/shell/test/util...
File ios/web/shell/test/utils/web_view_egutil.mm (right):

https://codereview.chromium.org/1890333002/diff/20001/ios/web/shell/test/util...
ios/web/shell/test/utils/web_view_egutil.mm:37: __block id script = nil;
On 2016/04/16 00:38:40, Eugene But wrote:
> This is not |script|...

A lot of this logic went away as I switched to WebState::ExecuteJavaScript

https://codereview.chromium.org/1890333002/diff/20001/ios/web/shell/test/util...
ios/web/shell/test/utils/web_view_egutil.mm:42: script = ^(NSString*
evaluationResult, NSError* error) {
On 2016/04/16 00:38:40, Eugene But wrote:
> Please log |error| if any.

N/A with new API

https://codereview.chromium.org/1890333002/diff/20001/ios/web/shell/test/util...
ios/web/shell/test/utils/web_view_egutil.mm:72: descriptionBlock:describe]
retain]);
On 2016/04/18 11:44:25, rohitrao wrote:
> Isn't this still a leak?  Alloc/init will return a retained object, so there
> should be no need to retain it again before putting it in a scoped_nsobject?

Done.

https://codereview.chromium.org/1890333002/diff/20001/ios/web/shell/test/util...
ios/web/shell/test/utils/web_view_egutil.mm:72: descriptionBlock:describe]
retain]);
On 2016/04/16 00:38:40, Eugene But wrote:
> This is a memory leak

Done.

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

https://codereview.chromium.org/1890333002/diff/20001/ios/web/shell/test/web_...
ios/web/shell/test/web_shell_navigation_egtest.mm:7: #import <WebKit/WebKit.h>
On 2016/04/16 00:38:40, Eugene But wrote:
> Please sort includes alphabetically

Done.

https://codereview.chromium.org/1890333002/diff/20001/ios/web/shell/test/web_...
ios/web/shell/test/web_shell_navigation_egtest.mm:54: const GURL secondGURL =
web::test::HttpServer::MakeUrl("http://secondURL");
On 2016/04/16 00:38:40, Eugene But wrote:
> Optional NIT: s/secondGURL/secondURL

Done.

https://codereview.chromium.org/1890333002/diff/20001/ios/web/shell/test/web_...
ios/web/shell/test/web_shell_navigation_egtest.mm:55: NSString* secondURL =
base::SysUTF8ToNSString(secondGURL.spec().c_str());
On 2016/04/16 00:38:40, Eugene But wrote:
> Optional NIT: s/secondURL/secondURLSpec

Acknowledged.

https://codereview.chromium.org/1890333002/diff/20001/ios/web/shell/test/web_...
ios/web/shell/test/web_shell_navigation_egtest.mm:55: NSString* secondURL =
base::SysUTF8ToNSString(secondGURL.spec().c_str());
On 2016/04/16 00:38:41, Eugene But wrote:
> Drop |.c_str()|. SysUTF8ToNSString takes std::string

Acknowledged.

https://codereview.chromium.org/1890333002/diff/20001/ios/web/shell/test/web_...
ios/web/shell/test/web_shell_navigation_egtest.mm:61: [[EarlGrey
On 2016/04/16 00:38:40, Eugene But wrote:
> Can we group these methods into a single "enter URL" method? This seems to be
a
> common thing to do. Even in this test.

I think that's a reasonable approach, but isn't needed now that I call LoadURL

https://codereview.chromium.org/1890333002/diff/20001/ios/web/shell/test/web_...
ios/web/shell/test/web_shell_navigation_egtest.mm:62:
selectElementWithMatcher:grey_accessibilityLabel(
On 2016/04/16 00:38:40, Eugene But wrote:
> NIT: Can you move matcher to a separate variable to make this more readable?

n/a

https://codereview.chromium.org/1890333002/diff/20001/ios/web/shell/test/web_...
ios/web/shell/test/web_shell_navigation_egtest.mm:65: [[EarlGrey
selectElementWithMatcher:grey_accessibilityLabel(@"Clear text")]
On 2016/04/16 00:38:41, Eugene But wrote:
> Please make constant for "Clear Text"

n/a

https://codereview.chromium.org/1890333002/diff/20001/ios/web/shell/test/web_...
ios/web/shell/test/web_shell_navigation_egtest.mm:68:
selectElementWithMatcher:grey_accessibilityLabel(
On 2016/04/16 00:38:40, Eugene But wrote:
> NIT: Separate variable for matcher?

n/a for this CL, but good idea for the future.

https://codereview.chromium.org/1890333002/diff/20001/ios/web/shell/view_cont...
File ios/web/shell/view_controller.h (right):

https://codereview.chromium.org/1890333002/diff/20001/ios/web/shell/view_cont...
ios/web/shell/view_controller.h:36: @interface ViewController
(ExposedForTesting)
On 2016/04/16 00:38:41, Eugene But wrote:
> I don't see any issues with making WebState a public property. WebController
> should not be exposed, more over it will be completely removed from WebShell

Done.

https://codereview.chromium.org/1890333002/diff/40001/ios/web/shell/test/util...
File ios/web/shell/test/utils/navigation_egutil.mm (right):

https://codereview.chromium.org/1890333002/diff/40001/ios/web/shell/test/util...
ios/web/shell/test/utils/navigation_egutil.mm:16: namespace navigation_egutil {
I'm working on a better design for the utilities, so I have it like this just to
get things rolling.

[Eugene But (OOO till 7-30), 2016-04-21 17:07:56]

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

https://codereview.chromium.org/1890333002/diff/60001/ios/web/ios_web_shell_t...
ios/web/ios_web_shell_tests.gyp:30: 'shell/test/utils/navigation_egutil.h',
Utils is very generic directory name and it's totally unclear what should go
there and what should not. For now I would leave all util files right in
shell/test/. Later we can figure out the directory structure, but I do think
that name should be that generic. Thought?

https://codereview.chromium.org/1890333002/diff/60001/ios/web/public/test/res...
File ios/web/public/test/response_providers/html_response_provider.h (right):

https://codereview.chromium.org/1890333002/diff/60001/ios/web/public/test/res...
ios/web/public/test/response_providers/html_response_provider.h:22: // Note:
This works with the web::test::HttpServer and hence works on UIWebView
Please drop this comment. We don't support UIWebView anymore.

https://codereview.chromium.org/1890333002/diff/60001/ios/web/shell/test/util...
File ios/web/shell/test/utils/navigation_egutil.h (right):

https://codereview.chromium.org/1890333002/diff/60001/ios/web/shell/test/util...
ios/web/shell/test/utils/navigation_egutil.h:8: #import
"ios/web/shell/view_controller.h"
Drop this

https://codereview.chromium.org/1890333002/diff/60001/ios/web/shell/test/util...
ios/web/shell/test/utils/navigation_egutil.h:12: namespace navigation_egutil {
This should also be in web namespace

https://codereview.chromium.org/1890333002/diff/60001/ios/web/shell/test/util...
ios/web/shell/test/utils/navigation_egutil.h:12: namespace navigation_egutil {
There is nothing here related to Earl Grey. How about navigation_test_util ?

https://codereview.chromium.org/1890333002/diff/60001/ios/web/shell/test/util...
ios/web/shell/test/utils/navigation_egutil.h:14: // Load |url| in current
webState.
s/Load/Loads

https://codereview.chromium.org/1890333002/diff/60001/ios/web/shell/test/util...
ios/web/shell/test/utils/navigation_egutil.h:14: // Load |url| in current
webState.
Please document that ui::PAGE_TRANSITION_TYPED transition is used.

https://codereview.chromium.org/1890333002/diff/60001/ios/web/shell/test/util...
ios/web/shell/test/utils/navigation_egutil.h:15: void LoadURL(GURL url);
According to C++ Style guide this should be  LoadUrl

https://codereview.chromium.org/1890333002/diff/60001/ios/web/shell/test/util...
File ios/web/shell/test/utils/navigation_egutil.mm (right):

https://codereview.chromium.org/1890333002/diff/60001/ios/web/shell/test/util...
ios/web/shell/test/utils/navigation_egutil.mm:10: #include
"ios/web/web_state/web_state_impl.h"
You can use only ios/web/public, please import web_state.h instead.

https://codereview.chromium.org/1890333002/diff/60001/ios/web/shell/test/util...
ios/web/shell/test/utils/navigation_egutil.mm:25: DCHECK(navigationManager);
NIT: In upstream Chromium we don't add DCHECKs before pointers dereferencing as
it will crash anyways.

https://codereview.chromium.org/1890333002/diff/60001/ios/web/shell/test/util...
File ios/web/shell/test/utils/web_view_egutil.h (right):

https://codereview.chromium.org/1890333002/diff/60001/ios/web/shell/test/util...
ios/web/shell/test/utils/web_view_egutil.h:1: // Copyright 2016 The Chromium
Authors. All rights reserved.
This file will contain matchers, right? So I think we should be more specific
with name (f.e. web_view_matchers)

https://codereview.chromium.org/1890333002/diff/60001/ios/web/shell/test/util...
File ios/web/shell/test/utils/web_view_egutil.mm (right):

https://codereview.chromium.org/1890333002/diff/60001/ios/web/shell/test/util...
ios/web/shell/test/utils/web_view_egutil.mm:11: #import
"base/test/ios/wait_util.h"
s/import/include

https://codereview.chromium.org/1890333002/diff/60001/ios/web/shell/test/util...
ios/web/shell/test/utils/web_view_egutil.mm:15: #include
"ios/web/web_state/web_state_impl.h"
Import only public API. Also this is import, not include

https://codereview.chromium.org/1890333002/diff/60001/ios/web/shell/test/util...
ios/web/shell/test/utils/web_view_egutil.mm:20: const NSTimeInterval
kSpinDelaySeconds = 0.01;
Can we move this to a common place? Seems like something that will be used in
other places.

https://codereview.chromium.org/1890333002/diff/60001/ios/web/shell/test/util...
ios/web/shell/test/utils/web_view_egutil.mm:26: std::string
kGetDocumentBodyJavaScript =
Global objects are not allowed per C++ Style guide. Please use char[]

https://codereview.chromium.org/1890333002/diff/60001/ios/web/shell/test/util...
ios/web/shell/test/utils/web_view_egutil.mm:40: rootViewController];
Can we have move finding view controller to a common single place?

https://codereview.chromium.org/1890333002/diff/60001/ios/web/shell/test/util...
ios/web/shell/test/utils/web_view_egutil.mm:43: DCHECK(webState);
NIT: No need for this DCHECK, code will crash anyway.

https://codereview.chromium.org/1890333002/diff/60001/ios/web/shell/test/util...
ios/web/shell/test/utils/web_view_egutil.mm:46: void (^searchForTextBlock)(const
base::Value* value) =
Optional NIT: Is it possible to move block inline? Local variables for block
makes the code less readable because you reading the code in reverse order
comparing to the actual program flow.

https://codereview.chromium.org/1890333002/diff/60001/ios/web/shell/test/util...
ios/web/shell/test/utils/web_view_egutil.mm:49: if (value &&
value->IsType(base::Value::TYPE_STRING)) {
if (value && value->IsType(base::Value::TYPE_STRING) &&
value->GetAsString(&response)) {

https://codereview.chromium.org/1890333002/diff/60001/ios/web/shell/test/util...
ios/web/shell/test/utils/web_view_egutil.mm:51: if (response.find([text
UTF8String]) == std::string::npos) {
Please use sys_string_conversions instead of UTF8String

https://codereview.chromium.org/1890333002/diff/60001/ios/web/shell/test/util...
ios/web/shell/test/utils/web_view_egutil.mm:53: didSucceed = YES;
didSucceed = response.find([text UTF8String]) != std::string::npos;

https://codereview.chromium.org/1890333002/diff/60001/ios/web/shell/test/util...
ios/web/shell/test/utils/web_view_egutil.mm:59: while (!([[NSDate date]
compare:deadline] == NSOrderedDescending) &&
[[NSDate date] compare:deadline] != NSOrderedDescending

https://codereview.chromium.org/1890333002/diff/60001/ios/web/shell/test/util...
ios/web/shell/test/utils/web_view_egutil.mm:69: DescribeToBlock describe =
^void(id<GREYDescription> description) {
Optional NIT: Drop void for consistency with existing code.

https://codereview.chromium.org/1890333002/diff/60001/ios/web/shell/test/util...
ios/web/shell/test/utils/web_view_egutil.mm:76: autorelease];
Optional NIT: Should we add a convenience function to avoid this ugly
formatting:

matcherWithMatchesBlock:descriptionBlock: ?

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

https://codereview.chromium.org/1890333002/diff/60001/ios/web/shell/test/web_...
ios/web/shell/test/web_shell_navigation_egtest.mm:18: @interface
CRWWebShellNavigationTest : XCTestCase
From Style Guide: Every non-trivial interface, public and private, should have
an accompanying comment describing its purpose and how it fits into the larger
picture. Please describe which tests should be here.

https://codereview.chromium.org/1890333002/diff/60001/ios/web/shell/test/web_...
ios/web/shell/test/web_shell_navigation_egtest.mm:76:
selectElementWithMatcher:grey_accessibilityLabel(
This formatting does not look readable. How about adding a matcher:
shell_backButton()?

https://codereview.chromium.org/1890333002/diff/60001/ios/web/shell/test/web_...
ios/web/shell/test/web_shell_navigation_egtest.mm:83:
selectElementWithMatcher:grey_accessibilityLabel(
ditto

[baxley, 2016-04-21 21:52:52]

Thanks for the review. Comments addressed, I also moved a few files around so it
may affect the diffs/comments. PTAL!

A few things that moved...
shell/test/utils/navigation_egutil -> shell/test/navigation_test_util
shell/test/utils/web_view_egutil -> shell/test/web_view_matchers

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

https://codereview.chromium.org/1890333002/diff/60001/ios/web/ios_web_shell_t...
ios/web/ios_web_shell_tests.gyp:30: 'shell/test/utils/navigation_egutil.h',
On 2016/04/21 17:07:51, Eugene But wrote:
> Utils is very generic directory name and it's totally unclear what should go
> there and what should not. For now I would leave all util files right in
> shell/test/. Later we can figure out the directory structure, but I do think
> that name should be that generic. Thought?

Sounds good. Let's just leave it all in ios/web/shell/test. Then when we
finalize the design we can figure out what to do.

https://codereview.chromium.org/1890333002/diff/60001/ios/web/public/test/res...
File ios/web/public/test/response_providers/html_response_provider.h (right):

https://codereview.chromium.org/1890333002/diff/60001/ios/web/public/test/res...
ios/web/public/test/response_providers/html_response_provider.h:22: // Note:
This works with the web::test::HttpServer and hence works on UIWebView
On 2016/04/21 17:07:51, Eugene But wrote:
> Please drop this comment. We don't support UIWebView anymore.

Done.

https://codereview.chromium.org/1890333002/diff/60001/ios/web/shell/test/util...
File ios/web/shell/test/utils/navigation_egutil.h (right):

https://codereview.chromium.org/1890333002/diff/60001/ios/web/shell/test/util...
ios/web/shell/test/utils/navigation_egutil.h:6: #define
IOS_WEB_SHELL_TEST_NAVIGATION_EGUTIL_H_
Note: this file was moved to navigation_test_util.h

https://codereview.chromium.org/1890333002/diff/60001/ios/web/shell/test/util...
ios/web/shell/test/utils/navigation_egutil.h:8: #import
"ios/web/shell/view_controller.h"
On 2016/04/21 17:07:52, Eugene But wrote:
> Drop this

Done.

https://codereview.chromium.org/1890333002/diff/60001/ios/web/shell/test/util...
ios/web/shell/test/utils/navigation_egutil.h:12: namespace navigation_egutil {
On 2016/04/21 17:07:52, Eugene But wrote:
> This should also be in web namespace

Done.

https://codereview.chromium.org/1890333002/diff/60001/ios/web/shell/test/util...
ios/web/shell/test/utils/navigation_egutil.h:12: namespace navigation_egutil {
On 2016/04/21 17:07:52, Eugene But wrote:
> There is nothing here related to Earl Grey. How about navigation_test_util ?

Done.

https://codereview.chromium.org/1890333002/diff/60001/ios/web/shell/test/util...
ios/web/shell/test/utils/navigation_egutil.h:14: // Load |url| in current
webState.
On 2016/04/21 17:07:52, Eugene But wrote:
> Please document that ui::PAGE_TRANSITION_TYPED transition is used.

Done.

https://codereview.chromium.org/1890333002/diff/60001/ios/web/shell/test/util...
ios/web/shell/test/utils/navigation_egutil.h:14: // Load |url| in current
webState.
On 2016/04/21 17:07:52, Eugene But wrote:
> s/Load/Loads

Done.

https://codereview.chromium.org/1890333002/diff/60001/ios/web/shell/test/util...
ios/web/shell/test/utils/navigation_egutil.h:15: void LoadURL(GURL url);
On 2016/04/21 17:07:52, Eugene But wrote:
> According to C++ Style guide this should be  LoadUrl

Done.

https://codereview.chromium.org/1890333002/diff/60001/ios/web/shell/test/util...
File ios/web/shell/test/utils/navigation_egutil.mm (right):

https://codereview.chromium.org/1890333002/diff/60001/ios/web/shell/test/util...
ios/web/shell/test/utils/navigation_egutil.mm:10: #include
"ios/web/web_state/web_state_impl.h"
On 2016/04/21 17:07:53, Eugene But wrote:
> You can use only ios/web/public, please import web_state.h instead.

Done.

https://codereview.chromium.org/1890333002/diff/60001/ios/web/shell/test/util...
ios/web/shell/test/utils/navigation_egutil.mm:25: DCHECK(navigationManager);
On 2016/04/21 17:07:53, Eugene But wrote:
> NIT: In upstream Chromium we don't add DCHECKs before pointers dereferencing
as
> it will crash anyways.

Done.

https://codereview.chromium.org/1890333002/diff/60001/ios/web/shell/test/util...
File ios/web/shell/test/utils/web_view_egutil.h (right):

https://codereview.chromium.org/1890333002/diff/60001/ios/web/shell/test/util...
ios/web/shell/test/utils/web_view_egutil.h:1: // Copyright 2016 The Chromium
Authors. All rights reserved.
On 2016/04/21 17:07:53, Eugene But wrote:
> This file will contain matchers, right? So I think we should be more specific
> with name (f.e. web_view_matchers)

Done.

https://codereview.chromium.org/1890333002/diff/60001/ios/web/shell/test/util...
File ios/web/shell/test/utils/web_view_egutil.mm (right):

https://codereview.chromium.org/1890333002/diff/60001/ios/web/shell/test/util...
ios/web/shell/test/utils/web_view_egutil.mm:11: #import
"base/test/ios/wait_util.h"
On 2016/04/21 17:07:54, Eugene But wrote:
> s/import/include

Done.

https://codereview.chromium.org/1890333002/diff/60001/ios/web/shell/test/util...
ios/web/shell/test/utils/web_view_egutil.mm:15: #include
"ios/web/web_state/web_state_impl.h"
On 2016/04/21 17:07:54, Eugene But wrote:
> Import only public API. Also this is import, not include

Done.

https://codereview.chromium.org/1890333002/diff/60001/ios/web/shell/test/util...
ios/web/shell/test/utils/web_view_egutil.mm:20: const NSTimeInterval
kSpinDelaySeconds = 0.01;
On 2016/04/21 17:07:54, Eugene But wrote:
> Can we move this to a common place? Seems like something that will be used in
> other places.

After discussion, okay to leave here for now.

https://codereview.chromium.org/1890333002/diff/60001/ios/web/shell/test/util...
ios/web/shell/test/utils/web_view_egutil.mm:26: std::string
kGetDocumentBodyJavaScript =
On 2016/04/21 17:07:56, Eugene But wrote:
> Global objects are not allowed per C++ Style guide. Please use char[]

Done.

https://codereview.chromium.org/1890333002/diff/60001/ios/web/shell/test/util...
ios/web/shell/test/utils/web_view_egutil.mm:40: rootViewController];
On 2016/04/21 17:07:54, Eugene But wrote:
> Can we have move finding view controller to a common single place?

Done.

https://codereview.chromium.org/1890333002/diff/60001/ios/web/shell/test/util...
ios/web/shell/test/utils/web_view_egutil.mm:43: DCHECK(webState);
On 2016/04/21 17:07:54, Eugene But wrote:
> NIT: No need for this DCHECK, code will crash anyway.

Done.

https://codereview.chromium.org/1890333002/diff/60001/ios/web/shell/test/util...
ios/web/shell/test/utils/web_view_egutil.mm:46: void (^searchForTextBlock)(const
base::Value* value) =
On 2016/04/21 17:07:54, Eugene But wrote:
> Optional NIT: Is it possible to move block inline? Local variables for block
> makes the code less readable because you reading the code in reverse order
> comparing to the actual program flow.

Done.

https://codereview.chromium.org/1890333002/diff/60001/ios/web/shell/test/util...
ios/web/shell/test/utils/web_view_egutil.mm:49: if (value &&
value->IsType(base::Value::TYPE_STRING)) {
On 2016/04/21 17:07:55, Eugene But wrote:
> if (value && value->IsType(base::Value::TYPE_STRING) &&
> value->GetAsString(&response)) {

Done.

https://codereview.chromium.org/1890333002/diff/60001/ios/web/shell/test/util...
ios/web/shell/test/utils/web_view_egutil.mm:51: if (response.find([text
UTF8String]) == std::string::npos) {
On 2016/04/21 17:07:54, Eugene But wrote:
> Please use sys_string_conversions instead of UTF8String

Done.

https://codereview.chromium.org/1890333002/diff/60001/ios/web/shell/test/util...
ios/web/shell/test/utils/web_view_egutil.mm:53: didSucceed = YES;
On 2016/04/21 17:07:56, Eugene But wrote:
> didSucceed = response.find([text UTF8String]) != std::string::npos;

Done. I don't know what I was doing with this if block.

https://codereview.chromium.org/1890333002/diff/60001/ios/web/shell/test/util...
ios/web/shell/test/utils/web_view_egutil.mm:59: while (!([[NSDate date]
compare:deadline] == NSOrderedDescending) &&
On 2016/04/21 17:07:54, Eugene But wrote:
> [[NSDate date] compare:deadline] != NSOrderedDescending

Done.

https://codereview.chromium.org/1890333002/diff/60001/ios/web/shell/test/util...
ios/web/shell/test/utils/web_view_egutil.mm:69: DescribeToBlock describe =
^void(id<GREYDescription> description) {
On 2016/04/21 17:07:54, Eugene But wrote:
> Optional NIT: Drop void for consistency with existing code.

Done.

https://codereview.chromium.org/1890333002/diff/60001/ios/web/shell/test/util...
ios/web/shell/test/utils/web_view_egutil.mm:76: autorelease];
On 2016/04/21 17:07:54, Eugene But wrote:
> Optional NIT: Should we add a convenience function to avoid this ugly
> formatting:
> 
> matcherWithMatchesBlock:descriptionBlock: ?

After offline discussion, leave as is for now.

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

https://codereview.chromium.org/1890333002/diff/60001/ios/web/shell/test/web_...
ios/web/shell/test/web_shell_navigation_egtest.mm:18: @interface
CRWWebShellNavigationTest : XCTestCase
On 2016/04/21 17:07:56, Eugene But wrote:
> From Style Guide: Every non-trivial interface, public and private, should have
> an accompanying comment describing its purpose and how it fits into the larger
> picture. Please describe which tests should be here.

Done.

https://codereview.chromium.org/1890333002/diff/60001/ios/web/shell/test/web_...
ios/web/shell/test/web_shell_navigation_egtest.mm:76:
selectElementWithMatcher:grey_accessibilityLabel(
On 2016/04/21 17:07:56, Eugene But wrote:
> This formatting does not look readable. How about adding a matcher:
> shell_backButton()?

Done.

https://codereview.chromium.org/1890333002/diff/60001/ios/web/shell/test/web_...
ios/web/shell/test/web_shell_navigation_egtest.mm:83:
selectElementWithMatcher:grey_accessibilityLabel(
On 2016/04/21 17:07:56, Eugene But wrote:
> ditto

Done.

[Eugene But (OOO till 7-30), 2016-04-21 22:53:08]

Thanks for addressing the comments. LGTM

https://codereview.chromium.org/1890333002/diff/100001/ios/web/shell/test/nav...
File ios/web/shell/test/navigation_test_util.mm (right):

https://codereview.chromium.org/1890333002/diff/100001/ios/web/shell/test/nav...
ios/web/shell/test/navigation_test_util.mm:7: #import <EarlGrey/EarlGrey.h>
Drop this

https://codereview.chromium.org/1890333002/diff/100001/ios/web/shell/test/nav...
ios/web/shell/test/navigation_test_util.mm:21: ViewController* viewController =
s/viewController/view_controller

https://codereview.chromium.org/1890333002/diff/100001/ios/web/shell/test/nav...
ios/web/shell/test/navigation_test_util.mm:24: NavigationManager*
navigationManager =
s/navigationManager/navigation_manager

https://codereview.chromium.org/1890333002/diff/100001/ios/web/shell/test/nav...
ios/web/shell/test/navigation_test_util.mm:31: }  // namespace earl_grey_util
s/namespace earl_grey_util/navigation_test_util

https://codereview.chromium.org/1890333002/diff/100001/ios/web/shell/test/she...
File ios/web/shell/test/shell_matchers.h (right):

https://codereview.chromium.org/1890333002/diff/100001/ios/web/shell/test/she...
ios/web/shell/test/shell_matchers.h:10: + (id<GREYMatcher>)matcherForBackButton;
Optional NIT: Maybe matcherForShellBackButton ?

https://codereview.chromium.org/1890333002/diff/100001/ios/web/shell/test/she...
ios/web/shell/test/shell_matchers.h:22: // Shorthand for
GREYMatchers::matcherForBackButton
Optional NIT: Full stop at the end for consistency? Here and in other 2
functions.

https://codereview.chromium.org/1890333002/diff/100001/ios/web/shell/test/she...
ios/web/shell/test/shell_matchers.h:23: GREY_EXPORT id<GREYMatcher>
shell_backButton();
NIT: I don't think we want to use GREY_EXPORT macro, since this is not a part of
EG framework. Just export sounds more appropriate.

https://codereview.chromium.org/1890333002/diff/100001/ios/web/shell/test/web...
File ios/web/shell/test/web_shell_navigation_egtest.mm (right):

https://codereview.chromium.org/1890333002/diff/100001/ios/web/shell/test/web...
ios/web/shell/test/web_shell_navigation_egtest.mm:68:
selectElementWithMatcher:shell_webViewContainingText(firstResponse)]
Optional NIT: I would name things response1/URL1 just to improve the formatting.

https://codereview.chromium.org/1890333002/diff/100001/ios/web/shell/test/web...
File ios/web/shell/test/web_shell_test_util.h (right):

https://codereview.chromium.org/1890333002/diff/100001/ios/web/shell/test/web...
ios/web/shell/test/web_shell_test_util.h:9: #include "url/gurl.h"
Drop this

https://codereview.chromium.org/1890333002/diff/100001/ios/web/shell/test/web...
File ios/web/shell/test/web_shell_test_util.mm (right):

https://codereview.chromium.org/1890333002/diff/100001/ios/web/shell/test/web...
ios/web/shell/test/web_shell_test_util.mm:5: #include
"ios/web/shell/test/web_shell_test_util.h"
s/include/import

https://codereview.chromium.org/1890333002/diff/100001/ios/web/shell/test/web...
ios/web/shell/test/web_shell_test_util.mm:6: 
NIT: import UIKit since it's used here.

https://codereview.chromium.org/1890333002/diff/100001/ios/web/shell/test/web...
File ios/web/shell/test/web_view_matchers.h (right):

https://codereview.chromium.org/1890333002/diff/100001/ios/web/shell/test/web...
ios/web/shell/test/web_view_matchers.h:14: // Shorthand for
GREYMatchers::matcherForWebViewContainingText
Optional NIT: Full stop at the end for consistency.

[baxley, 2016-04-22 00:39:40]

On 2016/04/21 22:53:08, Eugene But wrote:
> Thanks for addressing the comments. LGTM
> 
>
https://codereview.chromium.org/1890333002/diff/100001/ios/web/shell/test/nav...
> File ios/web/shell/test/navigation_test_util.mm (right):
> 
>
https://codereview.chromium.org/1890333002/diff/100001/ios/web/shell/test/nav...
> ios/web/shell/test/navigation_test_util.mm:7: #import <EarlGrey/EarlGrey.h>
> Drop this
> 
>
https://codereview.chromium.org/1890333002/diff/100001/ios/web/shell/test/nav...
> ios/web/shell/test/navigation_test_util.mm:21: ViewController* viewController
=
> s/viewController/view_controller
> 
>
https://codereview.chromium.org/1890333002/diff/100001/ios/web/shell/test/nav...
> ios/web/shell/test/navigation_test_util.mm:24: NavigationManager*
> navigationManager =
> s/navigationManager/navigation_manager
> 
>
https://codereview.chromium.org/1890333002/diff/100001/ios/web/shell/test/nav...
> ios/web/shell/test/navigation_test_util.mm:31: }  // namespace earl_grey_util
> s/namespace earl_grey_util/navigation_test_util
> 
>
https://codereview.chromium.org/1890333002/diff/100001/ios/web/shell/test/she...
> File ios/web/shell/test/shell_matchers.h (right):
> 
>
https://codereview.chromium.org/1890333002/diff/100001/ios/web/shell/test/she...
> ios/web/shell/test/shell_matchers.h:10: +
(id<GREYMatcher>)matcherForBackButton;
> Optional NIT: Maybe matcherForShellBackButton ?
> 
>
https://codereview.chromium.org/1890333002/diff/100001/ios/web/shell/test/she...
> ios/web/shell/test/shell_matchers.h:22: // Shorthand for
> GREYMatchers::matcherForBackButton
> Optional NIT: Full stop at the end for consistency? Here and in other 2
> functions.
> 
>
https://codereview.chromium.org/1890333002/diff/100001/ios/web/shell/test/she...
> ios/web/shell/test/shell_matchers.h:23: GREY_EXPORT id<GREYMatcher>
> shell_backButton();
> NIT: I don't think we want to use GREY_EXPORT macro, since this is not a part
of
> EG framework. Just export sounds more appropriate.
> 
>
https://codereview.chromium.org/1890333002/diff/100001/ios/web/shell/test/web...
> File ios/web/shell/test/web_shell_navigation_egtest.mm (right):
> 
>
https://codereview.chromium.org/1890333002/diff/100001/ios/web/shell/test/web...
> ios/web/shell/test/web_shell_navigation_egtest.mm:68:
> selectElementWithMatcher:shell_webViewContainingText(firstResponse)]
> Optional NIT: I would name things response1/URL1 just to improve the
formatting.
> 
>
https://codereview.chromium.org/1890333002/diff/100001/ios/web/shell/test/web...
> File ios/web/shell/test/web_shell_test_util.h (right):
> 
>
https://codereview.chromium.org/1890333002/diff/100001/ios/web/shell/test/web...
> ios/web/shell/test/web_shell_test_util.h:9: #include "url/gurl.h"
> Drop this
> 
>
https://codereview.chromium.org/1890333002/diff/100001/ios/web/shell/test/web...
> File ios/web/shell/test/web_shell_test_util.mm (right):
> 
>
https://codereview.chromium.org/1890333002/diff/100001/ios/web/shell/test/web...
> ios/web/shell/test/web_shell_test_util.mm:5: #include
> "ios/web/shell/test/web_shell_test_util.h"
> s/include/import
> 
>
https://codereview.chromium.org/1890333002/diff/100001/ios/web/shell/test/web...
> ios/web/shell/test/web_shell_test_util.mm:6: 
> NIT: import UIKit since it's used here.
> 
>
https://codereview.chromium.org/1890333002/diff/100001/ios/web/shell/test/web...
> File ios/web/shell/test/web_view_matchers.h (right):
> 
>
https://codereview.chromium.org/1890333002/diff/100001/ios/web/shell/test/web...
> ios/web/shell/test/web_view_matchers.h:14: // Shorthand for
> GREYMatchers::matcherForWebViewContainingText
> Optional NIT: Full stop at the end for consistency.

Eugene,

I addressed the comments, but had questions on two of them. Would you mind
taking a look at those? Thanks!

[Eugene But (OOO till 7-30), 2016-04-22 01:21:28]

On 2016/04/22 00:39:40, baxley wrote:
> On 2016/04/21 22:53:08, Eugene But wrote:
> > Thanks for addressing the comments. LGTM
> > 
> >
>
https://codereview.chromium.org/1890333002/diff/100001/ios/web/shell/test/nav...
> > File ios/web/shell/test/navigation_test_util.mm (right):
> > 
> >
>
https://codereview.chromium.org/1890333002/diff/100001/ios/web/shell/test/nav...
> > ios/web/shell/test/navigation_test_util.mm:7: #import <EarlGrey/EarlGrey.h>
> > Drop this
> > 
> >
>
https://codereview.chromium.org/1890333002/diff/100001/ios/web/shell/test/nav...
> > ios/web/shell/test/navigation_test_util.mm:21: ViewController*
viewController
> =
> > s/viewController/view_controller
> > 
> >
>
https://codereview.chromium.org/1890333002/diff/100001/ios/web/shell/test/nav...
> > ios/web/shell/test/navigation_test_util.mm:24: NavigationManager*
> > navigationManager =
> > s/navigationManager/navigation_manager
> > 
> >
>
https://codereview.chromium.org/1890333002/diff/100001/ios/web/shell/test/nav...
> > ios/web/shell/test/navigation_test_util.mm:31: }  // namespace
earl_grey_util
> > s/namespace earl_grey_util/navigation_test_util
> > 
> >
>
https://codereview.chromium.org/1890333002/diff/100001/ios/web/shell/test/she...
> > File ios/web/shell/test/shell_matchers.h (right):
> > 
> >
>
https://codereview.chromium.org/1890333002/diff/100001/ios/web/shell/test/she...
> > ios/web/shell/test/shell_matchers.h:10: +
> (id<GREYMatcher>)matcherForBackButton;
> > Optional NIT: Maybe matcherForShellBackButton ?
> > 
> >
>
https://codereview.chromium.org/1890333002/diff/100001/ios/web/shell/test/she...
> > ios/web/shell/test/shell_matchers.h:22: // Shorthand for
> > GREYMatchers::matcherForBackButton
> > Optional NIT: Full stop at the end for consistency? Here and in other 2
> > functions.
> > 
> >
>
https://codereview.chromium.org/1890333002/diff/100001/ios/web/shell/test/she...
> > ios/web/shell/test/shell_matchers.h:23: GREY_EXPORT id<GREYMatcher>
> > shell_backButton();
> > NIT: I don't think we want to use GREY_EXPORT macro, since this is not a
part
> of
> > EG framework. Just export sounds more appropriate.
> > 
> >
>
https://codereview.chromium.org/1890333002/diff/100001/ios/web/shell/test/web...
> > File ios/web/shell/test/web_shell_navigation_egtest.mm (right):
> > 
> >
>
https://codereview.chromium.org/1890333002/diff/100001/ios/web/shell/test/web...
> > ios/web/shell/test/web_shell_navigation_egtest.mm:68:
> > selectElementWithMatcher:shell_webViewContainingText(firstResponse)]
> > Optional NIT: I would name things response1/URL1 just to improve the
> formatting.
> > 
> >
>
https://codereview.chromium.org/1890333002/diff/100001/ios/web/shell/test/web...
> > File ios/web/shell/test/web_shell_test_util.h (right):
> > 
> >
>
https://codereview.chromium.org/1890333002/diff/100001/ios/web/shell/test/web...
> > ios/web/shell/test/web_shell_test_util.h:9: #include "url/gurl.h"
> > Drop this
> > 
> >
>
https://codereview.chromium.org/1890333002/diff/100001/ios/web/shell/test/web...
> > File ios/web/shell/test/web_shell_test_util.mm (right):
> > 
> >
>
https://codereview.chromium.org/1890333002/diff/100001/ios/web/shell/test/web...
> > ios/web/shell/test/web_shell_test_util.mm:5: #include
> > "ios/web/shell/test/web_shell_test_util.h"
> > s/include/import
> > 
> >
>
https://codereview.chromium.org/1890333002/diff/100001/ios/web/shell/test/web...
> > ios/web/shell/test/web_shell_test_util.mm:6: 
> > NIT: import UIKit since it's used here.
> > 
> >
>
https://codereview.chromium.org/1890333002/diff/100001/ios/web/shell/test/web...
> > File ios/web/shell/test/web_view_matchers.h (right):
> > 
> >
>
https://codereview.chromium.org/1890333002/diff/100001/ios/web/shell/test/web...
> > ios/web/shell/test/web_view_matchers.h:14: // Shorthand for
> > GREYMatchers::matcherForWebViewContainingText
> > Optional NIT: Full stop at the end for consistency.
> 
> Eugene,
> 
> I addressed the comments, but had questions on two of them. Would you mind
> taking a look at those? Thanks!
Sure, but where are your questions? ;)

[baxley, 2016-04-22 02:02:59]

And now actually publishing my comments. :P

https://codereview.chromium.org/1890333002/diff/100001/ios/web/shell/test/nav...
File ios/web/shell/test/navigation_test_util.mm (right):

https://codereview.chromium.org/1890333002/diff/100001/ios/web/shell/test/nav...
ios/web/shell/test/navigation_test_util.mm:7: #import <EarlGrey/EarlGrey.h>
On 2016/04/21 22:53:08, Eugene But wrote:
> Drop this

Done.

https://codereview.chromium.org/1890333002/diff/100001/ios/web/shell/test/nav...
ios/web/shell/test/navigation_test_util.mm:21: ViewController* viewController =
On 2016/04/21 22:53:08, Eugene But wrote:
> s/viewController/view_controller

Done.

https://codereview.chromium.org/1890333002/diff/100001/ios/web/shell/test/nav...
ios/web/shell/test/navigation_test_util.mm:24: NavigationManager*
navigationManager =
On 2016/04/21 22:53:08, Eugene But wrote:
> s/navigationManager/navigation_manager

Done.

https://codereview.chromium.org/1890333002/diff/100001/ios/web/shell/test/nav...
ios/web/shell/test/navigation_test_util.mm:31: }  // namespace earl_grey_util
On 2016/04/21 22:53:08, Eugene But wrote:
> s/namespace earl_grey_util/navigation_test_util

Done.

https://codereview.chromium.org/1890333002/diff/100001/ios/web/shell/test/she...
File ios/web/shell/test/shell_matchers.h (right):

https://codereview.chromium.org/1890333002/diff/100001/ios/web/shell/test/she...
ios/web/shell/test/shell_matchers.h:10: + (id<GREYMatcher>)matcherForBackButton;
On 2016/04/21 22:53:08, Eugene But wrote:
> Optional NIT: Maybe matcherForShellBackButton ?

Done.

https://codereview.chromium.org/1890333002/diff/100001/ios/web/shell/test/she...
ios/web/shell/test/shell_matchers.h:22: // Shorthand for
GREYMatchers::matcherForBackButton
On 2016/04/21 22:53:08, Eugene But wrote:
> Optional NIT: Full stop at the end for consistency? Here and in other 2
> functions.

Done.

https://codereview.chromium.org/1890333002/diff/100001/ios/web/shell/test/she...
ios/web/shell/test/shell_matchers.h:23: GREY_EXPORT id<GREYMatcher>
shell_backButton();
On 2016/04/21 22:53:08, Eugene But wrote:
> NIT: I don't think we want to use GREY_EXPORT macro, since this is not a part
of
> EG framework. Just export sounds more appropriate.

Are you okay with just using extern "C" {? Or do you want me to put a
conditional for __cplusplus?

https://codereview.chromium.org/1890333002/diff/100001/ios/web/shell/test/web...
File ios/web/shell/test/web_shell_navigation_egtest.mm (right):

https://codereview.chromium.org/1890333002/diff/100001/ios/web/shell/test/web...
ios/web/shell/test/web_shell_navigation_egtest.mm:68:
selectElementWithMatcher:shell_webViewContainingText(firstResponse)]
On 2016/04/21 22:53:08, Eugene But wrote:
> Optional NIT: I would name things response1/URL1 just to improve the
formatting.

Done.

https://codereview.chromium.org/1890333002/diff/100001/ios/web/shell/test/web...
File ios/web/shell/test/web_shell_test_util.h (right):

https://codereview.chromium.org/1890333002/diff/100001/ios/web/shell/test/web...
ios/web/shell/test/web_shell_test_util.h:9: #include "url/gurl.h"
On 2016/04/21 22:53:08, Eugene But wrote:
> Drop this

Done.

https://codereview.chromium.org/1890333002/diff/100001/ios/web/shell/test/web...
File ios/web/shell/test/web_shell_test_util.mm (right):

https://codereview.chromium.org/1890333002/diff/100001/ios/web/shell/test/web...
ios/web/shell/test/web_shell_test_util.mm:5: #include
"ios/web/shell/test/web_shell_test_util.h"
On 2016/04/21 22:53:08, Eugene But wrote:
> s/include/import

Why should it be import? isn't web_shell_test_util.h mostly C++?

https://codereview.chromium.org/1890333002/diff/100001/ios/web/shell/test/web...
ios/web/shell/test/web_shell_test_util.mm:6: 
On 2016/04/21 22:53:08, Eugene But wrote:
> NIT: import UIKit since it's used here.

Done.

https://codereview.chromium.org/1890333002/diff/100001/ios/web/shell/test/web...
File ios/web/shell/test/web_view_matchers.h (right):

https://codereview.chromium.org/1890333002/diff/100001/ios/web/shell/test/web...
ios/web/shell/test/web_view_matchers.h:14: // Shorthand for
GREYMatchers::matcherForWebViewContainingText
On 2016/04/21 22:53:08, Eugene But wrote:
> Optional NIT: Full stop at the end for consistency.

Done.

[Eugene But (OOO till 7-30), 2016-04-22 02:33:29]

https://codereview.chromium.org/1890333002/diff/100001/ios/web/shell/test/she...
File ios/web/shell/test/shell_matchers.h (right):

https://codereview.chromium.org/1890333002/diff/100001/ios/web/shell/test/she...
ios/web/shell/test/shell_matchers.h:23: GREY_EXPORT id<GREYMatcher>
shell_backButton();
On 2016/04/22 02:02:59, baxley wrote:
> On 2016/04/21 22:53:08, Eugene But wrote:
> > NIT: I don't think we want to use GREY_EXPORT macro, since this is not a
part
> of
> > EG framework. Just export sounds more appropriate.
> 
> Are you okay with just using extern "C" {? Or do you want me to put a
> conditional for __cplusplus?
That's a good point. In Chromium we have macros like CONTENT_EXPORT and
NET_EXPORT. So it makes sense to have WEB_EXPORT. And I'm fine if you do this in
a separate CL.

https://codereview.chromium.org/1890333002/diff/100001/ios/web/shell/test/web...
File ios/web/shell/test/web_shell_test_util.mm (right):

https://codereview.chromium.org/1890333002/diff/100001/ios/web/shell/test/web...
ios/web/shell/test/web_shell_test_util.mm:5: #include
"ios/web/shell/test/web_shell_test_util.h"
On 2016/04/22 02:02:59, baxley wrote:
> On 2016/04/21 22:53:08, Eugene But wrote:
> > s/include/import
> 
> Why should it be import? isn't web_shell_test_util.h mostly C++?
Well mostly is not enough to be qualified for C++.
|#import "ios/web/shell/view_controller.h"| fully disqualifies this header as
C++ header.

[baxley, 2016-04-22 13:32:39]

Okay, I made the include->import change. I'll leave it as extern "C" for this CL
and make a follow up as soon as this lands to create a WEB_EXPORT.

https://codereview.chromium.org/1890333002/diff/100001/ios/web/shell/test/web...
File ios/web/shell/test/web_shell_test_util.mm (right):

https://codereview.chromium.org/1890333002/diff/100001/ios/web/shell/test/web...
ios/web/shell/test/web_shell_test_util.mm:5: #include
"ios/web/shell/test/web_shell_test_util.h"
On 2016/04/21 22:53:08, Eugene But wrote:
> s/include/import

Done.

[baxley, 2016-04-22 16:51:19]

The CQ bit was checked by baxley@chromium.org

[baxley, 2016-04-22 16:51:20]

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/1890333002/#ps140001
(title: "s/include/import")

[commit-bot: I haz the power, 2016-04-22 16:51:38]

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

[commit-bot: I haz the power, 2016-04-22 17:48:33]

Committed patchset #8 (id:140001)

[commit-bot: I haz the power, 2016-04-22 19:49:26]

Description was changed from

==========
Web shell test to go back and forward.

Includes utilities for the simple local html server, and to verify text
within a WKWebView. Tests and utilities are written in Earl Grey.

BUG=604049,604050
==========

to

==========
Web shell test to go back and forward.

Includes utilities for the simple local html server, and to verify text
within a WKWebView. Tests and utilities are written in Earl Grey.

BUG=604049,604050

Committed: https://crrev.com/46f30edfad269c5e85aa22c3ff067df2548a1022
Cr-Commit-Position: refs/heads/master@{#389157}
==========

[commit-bot: I haz the power, 2016-04-22 19:49:28]

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