Create ChromeEarlGrey waitForWebViewContainingText.

Create ChromeEarlGrey waitForWebViewContainingText.

Matchers should not wait, so this is a replacement. This replaces
most uses of chrome_test_util::WebViewContainingText. There are
a few left over that are dependent upon having a method that
waits for the absence of a test.

BUG=707009
Review-Url: https://codereview.chromium.org/2798773002
Cr-Commit-Position: refs/heads/master@{#478687}
Committed: https://chromium.googlesource.com/chromium/src/+/ee4557b858b9b19eeab13da386f15e31e27718e6

[baxley, 2017-04-21 18:09:54]

Description was changed from

==========
Create ChromeEarlGrey waitForWebViewContainingText.

Matchers should not wait, so this is a replacement.

BUG=
==========

to

==========
Create ChromeEarlGrey waitForWebViewContainingText.

Matchers should not wait, so this is a replacement. This replaces
most uses of chrome_test_util::WebViewContainingText. There are
a few left over that are dependent upon having a method that
waits for the absence of a test.

BUG=
==========

[baxley, 2017-04-21 18:13:52]

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

[baxley, 2017-04-21 18:13:53]

If you don't have time, we can wait a week on this.

Please look at at chrome_earl_grey, shell_earl_grey, and
web_view_content_test_util first. Everything else should be callsites that are
using these three files.

In the callsites I sometimes added using directives to help aide formatting, but
I tried to only do it when it would help in more than one or two places.

In reading_list_egtest I modified the logic a little based on new APIs.

There were a few tests I did not update since I think there may be some logic
improvements that need to be made with the new API and I wanted to isolate it
from this CL.

I'm also running it through a bunch of trybots to help make sure I didn't make
any typos.

Thanks!

[Eugene But (OOO till 7-30), 2017-04-21 18:49:21]

Thank you for doing this!

https://codereview.chromium.org/2798773002/diff/140001/ios/chrome/browser/ui/...
File ios/chrome/browser/ui/dialogs/javascript_dialog_egtest.mm (right):

https://codereview.chromium.org/2798773002/diff/140001/ios/chrome/browser/ui/...
ios/chrome/browser/ui/dialogs/javascript_dialog_egtest.mm:167: NSString* URL =
base::SysUTF8ToNSString(
nit: s/URL/spec

https://codereview.chromium.org/2798773002/diff/140001/ios/chrome/browser/ui/...
ios/chrome/browser/ui/dialogs/javascript_dialog_egtest.mm:170: [NSString
stringWithFormat:kPageWithLinkResponseFormat, @(kLinkID), URL,
Is there a reason for converting everything to NSString and %@? %s was working
and needed less boilerplate code.

https://codereview.chromium.org/2798773002/diff/140001/ios/chrome/browser/ui/...
ios/chrome/browser/ui/dialogs/javascript_dialog_egtest.mm:170: [NSString
stringWithFormat:kPageWithLinkResponseFormat, @(kLinkID), URL,
Isn't @(char[]) just a wrapper for initWithUTF8String:, which we decided not to
use? I think in Chrome we always use base::SysUTF8ToNSString to avoid problems
with null pointers.

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

https://codereview.chromium.org/2798773002/diff/140001/ios/chrome/test/earl_g...
ios/chrome/test/earl_grey/chrome_earl_grey.h:85: +
(void)waitForWebViewContainingText:(NSString*)text;
Is there a reason why this takes NSString and not std::string? I looked at
callers and they either have std::string/char[] (which they have to convert) or
a literal.

NSString* arguments forces multiple callers to use base::SysUTF8ToNSString which
makes code less readable.

https://codereview.chromium.org/2798773002/diff/140001/ios/web/public/test/we...
File ios/web/public/test/web_view_content_test_util.h (right):

https://codereview.chromium.org/2798773002/diff/140001/ios/web/public/test/we...
ios/web/public/test/web_view_content_test_util.h:14: // Otherwise, returns
false.
Do you want to mention anything about "timeout"?

[baxley, 2017-04-21 20:09:01]

A few follow up questions on some of the comments...

https://codereview.chromium.org/2798773002/diff/140001/ios/chrome/browser/ui/...
File ios/chrome/browser/ui/dialogs/javascript_dialog_egtest.mm (right):

https://codereview.chromium.org/2798773002/diff/140001/ios/chrome/browser/ui/...
ios/chrome/browser/ui/dialogs/javascript_dialog_egtest.mm:170: [NSString
stringWithFormat:kPageWithLinkResponseFormat, @(kLinkID), URL,
On 2017/04/21 18:49:21, Eugene But wrote:
> Isn't @(char[]) just a wrapper for initWithUTF8String:, which we decided not
to
> use? I think in Chrome we always use base::SysUTF8ToNSString to avoid problems
> with null pointers.

Acknowledged.

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

https://codereview.chromium.org/2798773002/diff/140001/ios/chrome/test/earl_g...
ios/chrome/test/earl_grey/chrome_earl_grey.h:85: +
(void)waitForWebViewContainingText:(NSString*)text;
On 2017/04/21 18:49:21, Eugene But wrote:
> Is there a reason why this takes NSString and not std::string? I looked at
> callers and they either have std::string/char[] (which they have to convert)
or
> a literal.
> 
> NSString* arguments forces multiple callers to use base::SysUTF8ToNSString
which
> makes code less readable.

I went back and forth on this. All the methods in this file are Obj-C, and take
NSString, so I opted for consistency with the APIs here. A handful of tests had
to do a decent amount of conversions, but a lot of tests were negligibly
affected. Some of the simple ones fit a little better with NSString, but as you
pointed out these are mostly dealing with literals.

If you feel strongly that the tests deal in string, and have one API in
ChromeEarlGrey that takes string amongst others that take NSString, I'm happy to
make the change.

https://codereview.chromium.org/2798773002/diff/140001/ios/web/public/test/we...
File ios/web/public/test/web_view_content_test_util.h (right):

https://codereview.chromium.org/2798773002/diff/140001/ios/web/public/test/we...
ios/web/public/test/web_view_content_test_util.h:14: // Otherwise, returns
false.
On 2017/04/21 18:49:21, Eugene But wrote:
> Do you want to mention anything about "timeout"?

Are you suggesting to say that this method doesn't timeout?

This method doesn't wait for a timeout. It waits for JS execution to complete,
but when it's done it simply returns the result. The callers (in ChromeEarlGrey
and ShellEarlGrey) do the waiting and talk about timeout.

[Eugene But (OOO till 7-30), 2017-04-21 21:37:46]

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

https://codereview.chromium.org/2798773002/diff/140001/ios/chrome/test/earl_g...
ios/chrome/test/earl_grey/chrome_earl_grey.h:85: +
(void)waitForWebViewContainingText:(NSString*)text;
On 2017/04/21 20:09:01, baxley wrote:
> On 2017/04/21 18:49:21, Eugene But wrote:
> > Is there a reason why this takes NSString and not std::string? I looked at
> > callers and they either have std::string/char[] (which they have to convert)
> or
> > a literal.
> > 
> > NSString* arguments forces multiple callers to use base::SysUTF8ToNSString
> which
> > makes code less readable.
> 
> I went back and forth on this. All the methods in this file are Obj-C, and
take
> NSString, so I opted for consistency with the APIs here. A handful of tests
had
> to do a decent amount of conversions, but a lot of tests were negligibly
> affected. Some of the simple ones fit a little better with NSString, but as
you
> pointed out these are mostly dealing with literals.
> 
> If you feel strongly that the tests deal in string, and have one API in
> ChromeEarlGrey that takes string amongst others that take NSString, I'm happy
to
> make the change.
|loadURL:| takes GURL, instead of NSURL, so using C++ objects does not seem to
be a big deal. I don't want to make you change bunch of code, but I think it is
better to optimize API for callers, even if it makes it inconsistent. Also I
looked at other API that take NSString* and look like they also could take
std::string instead (clients pass either literal or have to convert std::string
to NSString*). Thoughts?

https://codereview.chromium.org/2798773002/diff/140001/ios/web/public/test/we...
File ios/web/public/test/web_view_content_test_util.h (right):

https://codereview.chromium.org/2798773002/diff/140001/ios/web/public/test/we...
ios/web/public/test/web_view_content_test_util.h:14: // Otherwise, returns
false.
On 2017/04/21 20:09:01, baxley wrote:
> On 2017/04/21 18:49:21, Eugene But wrote:
> > Do you want to mention anything about "timeout"?
> 
> Are you suggesting to say that this method doesn't timeout?
> 
> This method doesn't wait for a timeout. It waits for JS execution to complete,
> but when it's done it simply returns the result. The callers (in
ChromeEarlGrey
> and ShellEarlGrey) do the waiting and talk about timeout.
This method does "timeout". JS execution can "timeout", not sure if that's
useful to mention.

[baxley, 2017-04-21 22:41:58]

Thanks for the comments.

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

https://codereview.chromium.org/2798773002/diff/140001/ios/chrome/test/earl_g...
ios/chrome/test/earl_grey/chrome_earl_grey.h:85: +
(void)waitForWebViewContainingText:(NSString*)text;
On 2017/04/21 21:37:45, Eugene But wrote:
> On 2017/04/21 20:09:01, baxley wrote:
> > On 2017/04/21 18:49:21, Eugene But wrote:
> > > Is there a reason why this takes NSString and not std::string? I looked at
> > > callers and they either have std::string/char[] (which they have to
convert)
> > or
> > > a literal.
> > > 
> > > NSString* arguments forces multiple callers to use base::SysUTF8ToNSString
> > which
> > > makes code less readable.
> > 
> > I went back and forth on this. All the methods in this file are Obj-C, and
> take
> > NSString, so I opted for consistency with the APIs here. A handful of tests
> had
> > to do a decent amount of conversions, but a lot of tests were negligibly
> > affected. Some of the simple ones fit a little better with NSString, but as
> you
> > pointed out these are mostly dealing with literals.
> > 
> > If you feel strongly that the tests deal in string, and have one API in
> > ChromeEarlGrey that takes string amongst others that take NSString, I'm
happy
> to
> > make the change.
> |loadURL:| takes GURL, instead of NSURL, so using C++ objects does not seem to
> be a big deal. I don't want to make you change bunch of code, but I think it
is
> better to optimize API for callers, even if it makes it inconsistent. Also I
> looked at other API that take NSString* and look like they also could take
> std::string instead (clients pass either literal or have to convert
std::string
> to NSString*). Thoughts?

I'll look at it a little more and think about it. When I first looked, neither
looked great. I just want to think about it before spending time to do it, and
realizing it may not be much better.

https://codereview.chromium.org/2798773002/diff/140001/ios/web/public/test/we...
File ios/web/public/test/web_view_content_test_util.h (right):

https://codereview.chromium.org/2798773002/diff/140001/ios/web/public/test/we...
ios/web/public/test/web_view_content_test_util.h:14: // Otherwise, returns
false.
On 2017/04/21 21:37:46, Eugene But wrote:
> On 2017/04/21 20:09:01, baxley wrote:
> > On 2017/04/21 18:49:21, Eugene But wrote:
> > > Do you want to mention anything about "timeout"?
> > 
> > Are you suggesting to say that this method doesn't timeout?
> > 
> > This method doesn't wait for a timeout. It waits for JS execution to
complete,
> > but when it's done it simply returns the result. The callers (in
> ChromeEarlGrey
> > and ShellEarlGrey) do the waiting and talk about timeout.
> This method does "timeout". JS execution can "timeout", not sure if that's
> useful to mention.

I don't consider it a timeout in the same sense to the user. These timeouts are
implementation details in making JS call synchronous. I know ExecuteJavaScript
uses a timeout in its implementation.

I feel like timeout has more meaning in the methods where the user is waiting
for a condition. This signifies that it didn't reach the condition within a
timeout, rather than reaching a state in which we know the condition will never
be met, or waiting indefinitely.

Anyway, I don't think we should say it in this comment, like we haven't for
other similar comments. It still returns true if it contains the text, and false
if it doesn't contain the text (regardless if there was an error or not). Any
objections?

[Eugene But (OOO till 7-30), 2017-04-21 22:46:15]

https://codereview.chromium.org/2798773002/diff/140001/ios/web/public/test/we...
File ios/web/public/test/web_view_content_test_util.h (right):

https://codereview.chromium.org/2798773002/diff/140001/ios/web/public/test/we...
ios/web/public/test/web_view_content_test_util.h:14: // Otherwise, returns
false.
On 2017/04/21 22:41:58, baxley wrote:
> On 2017/04/21 21:37:46, Eugene But wrote:
> > On 2017/04/21 20:09:01, baxley wrote:
> > > On 2017/04/21 18:49:21, Eugene But wrote:
> > > > Do you want to mention anything about "timeout"?
> > > 
> > > Are you suggesting to say that this method doesn't timeout?
> > > 
> > > This method doesn't wait for a timeout. It waits for JS execution to
> complete,
> > > but when it's done it simply returns the result. The callers (in
> > ChromeEarlGrey
> > > and ShellEarlGrey) do the waiting and talk about timeout.
> > This method does "timeout". JS execution can "timeout", not sure if that's
> > useful to mention.
> 
> I don't consider it a timeout in the same sense to the user. These timeouts
are
> implementation details in making JS call synchronous. I know ExecuteJavaScript
> uses a timeout in its implementation.
> 
> I feel like timeout has more meaning in the methods where the user is waiting
> for a condition. This signifies that it didn't reach the condition within a
> timeout, rather than reaching a state in which we know the condition will
never
> be met, or waiting indefinitely.
> 
> Anyway, I don't think we should say it in this comment, like we haven't for
> other similar comments. It still returns true if it contains the text, and
false
> if it doesn't contain the text (regardless if there was an error or not). Any
> objections?
No objections.

[baxley, 2017-06-09 20:30:16]

I finally got back to cleaning this up!

Unfortunately, it's probably easiest to review it against head.

[Eugene But (OOO till 7-30), 2017-06-12 03:47:12]

lgtm

https://codereview.chromium.org/2798773002/diff/200002/ios/chrome/browser/ui/...
File ios/chrome/browser/ui/dialogs/javascript_dialog_egtest.mm (right):

https://codereview.chromium.org/2798773002/diff/200002/ios/chrome/browser/ui/...
ios/chrome/browser/ui/dialogs/javascript_dialog_egtest.mm:321: [ChromeEarlGrey
waitForWebViewContainingText:""];
nit: s/""std::string()

https://codereview.chromium.org/2798773002/diff/200002/ios/chrome/browser/ui/...
File ios/chrome/browser/ui/history/history_ui_egtest.mm (right):

https://codereview.chromium.org/2798773002/diff/200002/ios/chrome/browser/ui/...
ios/chrome/browser/ui/history/history_ui_egtest.mm:61: NSString* url_spec_text =
SysUTF8ToNSString(url.spec());
nit: Is this really necessary?

https://codereview.chromium.org/2798773002/diff/200002/ios/chrome/browser/ui/...
File ios/chrome/browser/ui/reading_list/reading_list_egtest.mm (right):

https://codereview.chromium.org/2798773002/diff/200002/ios/chrome/browser/ui/...
ios/chrome/browser/ui/reading_list/reading_list_egtest.mm:321:
GREYAssert(testing::WaitUntilConditionOrTimeout(
Do you want to explain in the comments why waitForWebViewContainingText: would
not work here?

https://codereview.chromium.org/2798773002/diff/200002/ios/chrome/browser/web...
File ios/chrome/browser/web/visible_url_egtest.mm (right):

https://codereview.chromium.org/2798773002/diff/200002/ios/chrome/browser/web...
ios/chrome/browser/web/visible_url_egtest.mm:296: [ChromeEarlGrey
tapWebViewElementWithID:SysUTF8ToNSString(kGoBackLink)];
nit: Is this really necessary? Same question for other places.

https://codereview.chromium.org/2798773002/diff/200002/ios/web/public/test/we...
File ios/web/public/test/web_view_content_test_util.h (right):

https://codereview.chromium.org/2798773002/diff/200002/ios/web/public/test/we...
ios/web/public/test/web_view_content_test_util.h:15: bool
WebViewContainingText(web::WebState* web_state, const std::string& text);
Do you want to prefix this method with |Is|?

[baxley, 2017-06-12 16:41:18]

Comments addressed. Thanks for the thorough review!

https://codereview.chromium.org/2798773002/diff/200002/ios/chrome/browser/ui/...
File ios/chrome/browser/ui/dialogs/javascript_dialog_egtest.mm (right):

https://codereview.chromium.org/2798773002/diff/200002/ios/chrome/browser/ui/...
ios/chrome/browser/ui/dialogs/javascript_dialog_egtest.mm:321: [ChromeEarlGrey
waitForWebViewContainingText:""];
On 2017/06/12 03:47:11, Eugene But (OOO till June 11) wrote:
> nit: s/""std::string()

Done.

https://codereview.chromium.org/2798773002/diff/200002/ios/chrome/browser/ui/...
File ios/chrome/browser/ui/history/history_ui_egtest.mm (right):

https://codereview.chromium.org/2798773002/diff/200002/ios/chrome/browser/ui/...
ios/chrome/browser/ui/history/history_ui_egtest.mm:61: NSString* url_spec_text =
SysUTF8ToNSString(url.spec());
On 2017/06/12 03:47:11, Eugene But (OOO till June 11) wrote:
> nit: Is this really necessary?

Nope. This was there from when I had the API take NSString, and we had to do
more conversions. Nice catch!

https://codereview.chromium.org/2798773002/diff/200002/ios/chrome/browser/ui/...
File ios/chrome/browser/ui/reading_list/reading_list_egtest.mm (right):

https://codereview.chromium.org/2798773002/diff/200002/ios/chrome/browser/ui/...
ios/chrome/browser/ui/reading_list/reading_list_egtest.mm:321:
GREYAssert(testing::WaitUntilConditionOrTimeout(
On 2017/06/12 03:47:11, Eugene But (OOO till June 11) wrote:
> Do you want to explain in the comments why waitForWebViewContainingText: would
> not work here?

Done.

https://codereview.chromium.org/2798773002/diff/200002/ios/chrome/browser/web...
File ios/chrome/browser/web/visible_url_egtest.mm (right):

https://codereview.chromium.org/2798773002/diff/200002/ios/chrome/browser/web...
ios/chrome/browser/web/visible_url_egtest.mm:296: [ChromeEarlGrey
tapWebViewElementWithID:SysUTF8ToNSString(kGoBackLink)];
On 2017/06/12 03:47:11, Eugene But (OOO till June 11) wrote:
> nit: Is this really necessary? Same question for other places.

Done.

https://codereview.chromium.org/2798773002/diff/200002/ios/web/public/test/we...
File ios/web/public/test/web_view_content_test_util.h (right):

https://codereview.chromium.org/2798773002/diff/200002/ios/web/public/test/we...
ios/web/public/test/web_view_content_test_util.h:15: bool
WebViewContainingText(web::WebState* web_state, const std::string& text);
On 2017/06/12 03:47:12, Eugene But (OOO till June 11) wrote:
> Do you want to prefix this method with |Is|?

Done.

[baxley, 2017-06-12 16:42:14]

Description was changed from

==========
Create ChromeEarlGrey waitForWebViewContainingText.

Matchers should not wait, so this is a replacement. This replaces
most uses of chrome_test_util::WebViewContainingText. There are
a few left over that are dependent upon having a method that
waits for the absence of a test.

BUG=
==========

to

==========
Create ChromeEarlGrey waitForWebViewContainingText.

Matchers should not wait, so this is a replacement. This replaces
most uses of chrome_test_util::WebViewContainingText. There are
a few left over that are dependent upon having a method that
waits for the absence of a test.

BUG=707009
==========

[baxley, 2017-06-12 16:42:21]

The CQ bit was checked by baxley@chromium.org

[baxley, 2017-06-12 16:42:21]

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/2798773002/#ps230001
(title: "review comments")

[commit-bot: I haz the power, 2017-06-12 16:42:45]

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-06-12 18:21:50]

CQ is committing da patch.

Bot data: {"patchset_id": 230001, "attempt_start_ts": 1497285741193390,
"parent_rev": "2eacda6204994f9639585a7adcce3d04a2b83bed", "commit_rev":
"ee4557b858b9b19eeab13da386f15e31e27718e6"}

[commit-bot: I haz the power, 2017-06-12 18:22:10]

Description was changed from

==========
Create ChromeEarlGrey waitForWebViewContainingText.

Matchers should not wait, so this is a replacement. This replaces
most uses of chrome_test_util::WebViewContainingText. There are
a few left over that are dependent upon having a method that
waits for the absence of a test.

BUG=707009
==========

to

==========
Create ChromeEarlGrey waitForWebViewContainingText.

Matchers should not wait, so this is a replacement. This replaces
most uses of chrome_test_util::WebViewContainingText. There are
a few left over that are dependent upon having a method that
waits for the absence of a test.

BUG=707009

Review-Url: https://codereview.chromium.org/2798773002
Cr-Commit-Position: refs/heads/master@{#478687}
Committed:
https://chromium.googlesource.com/chromium/src/+/ee4557b858b9b19eeab13da386f1...
==========

[commit-bot: I haz the power, 2017-06-12 18:22:13]

Committed patchset #13 (id:230001) as
https://chromium.googlesource.com/chromium/src/+/ee4557b858b9b19eeab13da386f1...