Remove matcher to wait for static HTML view.

Remove matcher to wait for static HTML view.

EarlGrey matchers should not wait. This adds a method
waitForStaticHTMLViewContainingText, which should be used instead.

BUG=700067
Review-Url: https://codereview.chromium.org/2784943002
Cr-Commit-Position: refs/heads/master@{#461529}
Committed: https://chromium.googlesource.com/chromium/src/+/7f5396b89be45513d1a2f9a97c4416be04b57a2e

[baxley, 2017-03-31 20:59:21]

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

[baxley, 2017-03-31 20:59:22]

Would you mind doing a high-level review first?

I put in a few comments where I have questions. You're welcome to review
anything, but if you have high-level questions we should resolve those before
getting to deep into the code.

Thanks!

https://codereview.chromium.org/2784943002/diff/120001/ios/chrome/browser/web...
File ios/chrome/browser/web/navigation_egtest.mm (left):

https://codereview.chromium.org/2784943002/diff/120001/ios/chrome/browser/web...
ios/chrome/browser/web/navigation_egtest.mm:112:
l10n_util::GetNSString(IDS_ERRORPAGES_HEADING_INTERNET_DISCONNECTED);
I chatted with kkhorimoto@ and he thought it was reasonable to remove the check
for IDS_ERROR_PAGES_HEADING_INTERNET_DISCONNECTED, when looking for an error
page. The thought was the test should be hitting *HEADING_NOT_AVAILABLE, and if
it's getting a WiFi problem then it should be handled differently.

https://codereview.chromium.org/2784943002/diff/120001/ios/chrome/test/app/st...
File ios/chrome/test/app/static_html_view_test_util.h (right):

https://codereview.chromium.org/2784943002/diff/120001/ios/chrome/test/app/st...
ios/chrome/test/app/static_html_view_test_util.h:15: bool
StaticHtmlViewContainingText(web::WebState* web_state, std::string text);
I know you had a concern about having some methods that return bool, some that
assert, some that wait, and some that don't.

I think it's reasonable to have ios/chrome/test/app methods that do no waiting
and just check a state. Then ChromeEarlGrey can do any waiting that is
necessary.

https://codereview.chromium.org/2784943002/diff/120001/ios/chrome/test/earl_g...
File ios/chrome/test/earl_grey/chrome_earl_grey.h (right):

https://codereview.chromium.org/2784943002/diff/120001/ios/chrome/test/earl_g...
ios/chrome/test/earl_grey/chrome_earl_grey.h:62: isTextPresent:(BOOL)present;
Let me know what you think about this API. The previous behavior was the
"contains" case was there is a static html view that contains |text|. The
negative of this was there is either no static html view, or the one that exists
doesn't contain the given text.

Let me know if this doesn't sound right, or you think the name/comment could be
better.

[Eugene But (OOO till 7-30), 2017-03-31 22:19:17]

Even if this CL was not intended as a final CL, I did not mind reviewing. And
this is actually lgtm.

https://codereview.chromium.org/2784943002/diff/120001/ios/chrome/test/app/st...
File ios/chrome/test/app/static_html_view_test_util.h (right):

https://codereview.chromium.org/2784943002/diff/120001/ios/chrome/test/app/st...
ios/chrome/test/app/static_html_view_test_util.h:15: bool
StaticHtmlViewContainingText(web::WebState* web_state, std::string text);
On 2017/03/31 20:59:22, baxley wrote:
> I know you had a concern about having some methods that return bool, some that
> assert, some that wait, and some that don't.
> 
> I think it's reasonable to have ios/chrome/test/app methods that do no waiting
> and just check a state. Then ChromeEarlGrey can do any waiting that is
> necessary.
I think this is fine. AFAIK, methods inside chrome_test_util are not suppose to
assert.

https://codereview.chromium.org/2784943002/diff/120001/ios/chrome/test/app/st...
File ios/chrome/test/app/static_html_view_test_util.mm (right):

https://codereview.chromium.org/2784943002/diff/120001/ios/chrome/test/app/st...
ios/chrome/test/app/static_html_view_test_util.mm:27: web::JavaScriptResultBlock
completion_handler =
Optional nit: do you want to fold this block into a call?

https://codereview.chromium.org/2784943002/diff/120001/ios/chrome/test/app/st...
ios/chrome/test/app/static_html_view_test_util.mm:29: result = [script_result
copy];
This looks like a leak to me

https://codereview.chromium.org/2784943002/diff/120001/ios/chrome/test/app/st...
ios/chrome/test/app/static_html_view_test_util.mm:55:
base::mac::ObjCCast<WKWebView>(web_state_view.subviews[0]);
Do you want to do web_state_view.subviews.forstObject, to avoid out of bounds
exception?

https://codereview.chromium.org/2784943002/diff/120001/ios/chrome/test/app/st...
ios/chrome/test/app/static_html_view_test_util.mm:56: StaticHtmlViewController*
html_view_controller = nil;
I think the following code will work correctly for all the cases:
return
base::mac::ObjCCast<StaticHtmlViewController>(web_view.navigationDelegate);

https://codereview.chromium.org/2784943002/diff/120001/ios/chrome/test/app/st...
ios/chrome/test/app/static_html_view_test_util.mm:73: if ([result
isKindOfClass:[NSString class]]) {
You can simplify this code as follows:

id body = ExecuteScriptInStaticController(html_view_controller, @"document.body
? document.body.textContent : ''");
return [body containsString:base::SysUTF8ToNSString(text)];

https://codereview.chromium.org/2784943002/diff/120001/ios/chrome/test/earl_g...
File ios/chrome/test/earl_grey/chrome_earl_grey.h (right):

https://codereview.chromium.org/2784943002/diff/120001/ios/chrome/test/earl_g...
ios/chrome/test/earl_grey/chrome_earl_grey.h:62: isTextPresent:(BOOL)present;
On 2017/03/31 20:59:22, baxley wrote:
> Let me know what you think about this API. The previous behavior was the
> "contains" case was there is a static html view that contains |text|. The
> negative of this was there is either no static html view, or the one that
exists
> doesn't contain the given text.
> 
> Let me know if this doesn't sound right, or you think the name/comment could
be
> better.
It does not sounds like a canonical Objective-C name to me, which should read as
a sentence. I would create 2 public APIs instead:
+ (void)waitForStaticHTMLViewContainingText:(NSString*)text;
+ (void)waitForStaticHTMLViewNotContainingText:(NSString*)text;

Even if it means to have more code in the implementation.

[baxley, 2017-04-03 18:10:41]

comments addressed. Thanks for the detailed review and suggestions.

https://codereview.chromium.org/2784943002/diff/120001/ios/chrome/test/app/st...
File ios/chrome/test/app/static_html_view_test_util.h (right):

https://codereview.chromium.org/2784943002/diff/120001/ios/chrome/test/app/st...
ios/chrome/test/app/static_html_view_test_util.h:15: bool
StaticHtmlViewContainingText(web::WebState* web_state, std::string text);
On 2017/03/31 22:19:17, Eugene But wrote:
> On 2017/03/31 20:59:22, baxley wrote:
> > I know you had a concern about having some methods that return bool, some
that
> > assert, some that wait, and some that don't.
> > 
> > I think it's reasonable to have ios/chrome/test/app methods that do no
waiting
> > and just check a state. Then ChromeEarlGrey can do any waiting that is
> > necessary.
> I think this is fine. AFAIK, methods inside chrome_test_util are not suppose
to
> assert.

Yes, this comment was in response to an offline conversation about it being
confusing to have some that wait, some that don't, some that assert, some that
return bool.

I think that didn't have the context of this, where it fits fine.

https://codereview.chromium.org/2784943002/diff/120001/ios/chrome/test/app/st...
File ios/chrome/test/app/static_html_view_test_util.mm (right):

https://codereview.chromium.org/2784943002/diff/120001/ios/chrome/test/app/st...
ios/chrome/test/app/static_html_view_test_util.mm:27: web::JavaScriptResultBlock
completion_handler =
On 2017/03/31 22:19:17, Eugene But wrote:
> Optional nit: do you want to fold this block into a call?

Done.

https://codereview.chromium.org/2784943002/diff/120001/ios/chrome/test/app/st...
ios/chrome/test/app/static_html_view_test_util.mm:29: result = [script_result
copy];
On 2017/03/31 22:19:17, Eugene But wrote:
> This looks like a leak to me

Done. Nice Catch!

https://codereview.chromium.org/2784943002/diff/120001/ios/chrome/test/app/st...
ios/chrome/test/app/static_html_view_test_util.mm:55:
base::mac::ObjCCast<WKWebView>(web_state_view.subviews[0]);
On 2017/03/31 22:19:17, Eugene But wrote:
> Do you want to do web_state_view.subviews.forstObject, to avoid out of bounds
> exception? 

I had the if condition above to prevent that, but firstObject is cleaner and
safer. Done.

https://codereview.chromium.org/2784943002/diff/120001/ios/chrome/test/app/st...
ios/chrome/test/app/static_html_view_test_util.mm:56: StaticHtmlViewController*
html_view_controller = nil;
On 2017/03/31 22:19:17, Eugene But wrote:
> I think the following code will work correctly for all the cases:
> return
> base::mac::ObjCCast<StaticHtmlViewController>(web_view.navigationDelegate);
> 

Done.

https://codereview.chromium.org/2784943002/diff/120001/ios/chrome/test/app/st...
ios/chrome/test/app/static_html_view_test_util.mm:73: if ([result
isKindOfClass:[NSString class]]) {
On 2017/03/31 22:19:17, Eugene But wrote:
> You can simplify this code as follows:
> 
> id body = ExecuteScriptInStaticController(html_view_controller,
@"document.body
> ? document.body.textContent : ''");
> return [body containsString:base::SysUTF8ToNSString(text)];

Done.

https://codereview.chromium.org/2784943002/diff/120001/ios/chrome/test/earl_g...
File ios/chrome/test/earl_grey/chrome_earl_grey.h (right):

https://codereview.chromium.org/2784943002/diff/120001/ios/chrome/test/earl_g...
ios/chrome/test/earl_grey/chrome_earl_grey.h:62: isTextPresent:(BOOL)present;
On 2017/03/31 22:19:17, Eugene But wrote:
> On 2017/03/31 20:59:22, baxley wrote:
> > Let me know what you think about this API. The previous behavior was the
> > "contains" case was there is a static html view that contains |text|. The
> > negative of this was there is either no static html view, or the one that
> exists
> > doesn't contain the given text.
> > 
> > Let me know if this doesn't sound right, or you think the name/comment could
> be
> > better.
> It does not sounds like a canonical Objective-C name to me, which should read
as
> a sentence. I would create 2 public APIs instead:
> + (void)waitForStaticHTMLViewContainingText:(NSString*)text;
> + (void)waitForStaticHTMLViewNotContainingText:(NSString*)text;
> 
> Even if it means to have more code in the implementation.
> 

Done.

[baxley, 2017-04-03 18:14:50]

The CQ bit was checked by baxley@chromium.org

[baxley, 2017-04-03 18:14:50]

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/2784943002/#ps200001
(title: "typo")

[commit-bot: I haz the power, 2017-04-03 18:16:19]

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

[commit-bot: I haz the power, 2017-04-03 20:52:32]

CQ is committing da patch.

Bot data: {"patchset_id": 200001, "attempt_start_ts": 1491243290251570,
"parent_rev": "36cf5528db3069f2232a9af0897bf4fb2da8fe62", "commit_rev":
"7f5396b89be45513d1a2f9a97c4416be04b57a2e"}

[commit-bot: I haz the power, 2017-04-03 20:53:24]

Description was changed from

==========
Remove matcher to wait for static HTML view.

EarlGrey matchers should not wait. This adds a method
waitForStaticHTMLViewContainingText, which should be used instead.

BUG=700067
==========

to

==========
Remove matcher to wait for static HTML view.

EarlGrey matchers should not wait. This adds a method
waitForStaticHTMLViewContainingText, which should be used instead.

BUG=700067

Review-Url: https://codereview.chromium.org/2784943002
Cr-Commit-Position: refs/heads/master@{#461529}
Committed:
https://chromium.googlesource.com/chromium/src/+/7f5396b89be45513d1a2f9a97c44...
==========

[commit-bot: I haz the power, 2017-04-03 20:53:25]

Committed patchset #11 (id:200001) as
https://chromium.googlesource.com/chromium/src/+/7f5396b89be45513d1a2f9a97c44...

[lpromero, 2017-04-19 11:35:09]

lpromero@chromium.org changed reviewers:
+ lpromero@chromium.org

[lpromero, 2017-04-19 11:35:10]

https://codereview.chromium.org/2784943002/diff/200001/ios/chrome/test/earl_g...
File ios/chrome/test/earl_grey/chrome_earl_grey.mm (right):

https://codereview.chromium.org/2784943002/diff/200001/ios/chrome/test/earl_g...
ios/chrome/test/earl_grey/chrome_earl_grey.mm:134: NSString* const
kInternetDisconnectedError =
The error is not about internet disconnection, but about page being unavailable.

[baxley, 2017-04-19 14:39:49]

https://codereview.chromium.org/2784943002/diff/200001/ios/chrome/test/earl_g...
File ios/chrome/test/earl_grey/chrome_earl_grey.mm (right):

https://codereview.chromium.org/2784943002/diff/200001/ios/chrome/test/earl_g...
ios/chrome/test/earl_grey/chrome_earl_grey.mm:134: NSString* const
kInternetDisconnectedError =
On 2017/04/19 11:35:10, lpromero wrote:
> The error is not about internet disconnection, but about page being
unavailable.

Thanks for pointing out the inconsistency. Do you think the API make sense, but
should maybe have a more clear comment/method name, and definitely a better
variable name here? Or do you think it should be doing something different?

[lpromero, 2017-04-19 14:44:52]

https://codereview.chromium.org/2784943002/diff/200001/ios/chrome/test/earl_g...
File ios/chrome/test/earl_grey/chrome_earl_grey.mm (right):

https://codereview.chromium.org/2784943002/diff/200001/ios/chrome/test/earl_g...
ios/chrome/test/earl_grey/chrome_earl_grey.mm:134: NSString* const
kInternetDisconnectedError =
On 2017/04/19 14:39:49, baxley wrote:
> On 2017/04/19 11:35:10, lpromero wrote:
> > The error is not about internet disconnection, but about page being
> unavailable.
> 
> Thanks for pointing out the inconsistency. Do you think the API make sense,
but
> should maybe have a more clear comment/method name, and definitely a better
> variable name here? Or do you think it should be doing something different?

The variable name might be sufficient, but since the error being waited upon is
specific, maybe a more specific method name could be worth it? I didn't feel the
need for it, I only thought of raising the issue for the variable.