Add option for url_fetch_test to wait for a JS expression...

Add option for url_fetch_test to wait for a JS expression

This option extends url_fetch_test to support waiting for a JavaScript
expression to evaluate to true. This is used in the Chrome OS
desktopui_GmailLatency test. The browser goes to the test URL and then runs
the test to completion.

Contributed by sjg@chromium.org

BUG=chromiumos-10869
TEST=test on my workstation on Ubuntu, x86-mario, qemu using a web page which
sets a JS value after a period.

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=71408

[sjg, 2011-01-13 18:48:03]

Will, I have created an official change for Chrome here - my concern was only
that since this feature is only used by Chrome OS people might complain about
adding code when no one knows who uses it. Also this change may not be needed by
Chrome OS long term.

But let's do it this way and see how we go.

Also, re the gtest timeout, I have added the comment, but I'm not sure of the
mechanism that limits the test time. Are you referring to
UITestBase::set_command_execution_timeout_ms or something higher level?

[tfarina, 2011-01-13 18:57:50]

http://codereview.chromium.org/6249003/diff/3001/chrome/test/url_fetch_test/u...
File chrome/test/url_fetch_test/url_fetch_test.cc (right):

http://codereview.chromium.org/6249003/diff/3001/chrome/test/url_fetch_test/u...
chrome/test/url_fetch_test/url_fetch_test.cc:1: // Copyright (c) 2010 The
Chromium Authors. All rights reserved.
Please, update the copyright year.

http://codereview.chromium.org/6249003/diff/3001/chrome/test/url_fetch_test/u...
chrome/test/url_fetch_test/url_fetch_test.cc:60: 
I think you removed this line for readability, but I don't think it's necessary.

http://codereview.chromium.org/6249003/diff/3001/chrome/test/url_fetch_test/u...
chrome/test/url_fetch_test/url_fetch_test.cc:128: //
As you did with the other switches, I think you can remove this blank line.

http://codereview.chromium.org/6249003/diff/3001/chrome/test/url_fetch_test/u...
chrome/test/url_fetch_test/url_fetch_test.cc:162: if
(!js_timeout_ms_str.empty()) {
Please, remove these {}, they are not necessary.

http://codereview.chromium.org/6249003/diff/3001/chrome/test/url_fetch_test/u...
chrome/test/url_fetch_test/url_fetch_test.cc:171: js_expr, js_frame_xpath,
js_timeout_ms, &result);
A minor style issue, although I don't think it's extremely necessary, I think
you could write one arg per line. It already looks a mess these lines.

[sjg, 2011-01-13 20:46:56]

Udpated as per comments from tfarina, thanks.

[asargent_no_longer_on_chrome, 2011-01-13 21:03:59]

lgtm

[sjg, 2011-01-13 22:11:03]

On 2011/01/13 21:03:59, Antony Sargent wrote:
> lgtm

Thanks, as I am not a committer for Chrome, can you please commit for me?

[tfarina, 2011-01-13 22:44:55]

On 2011/01/13 22:11:03, sjg wrote:
> On 2011/01/13 21:03:59, Antony Sargent wrote:
> > lgtm
> 
> Thanks, as I am not a committer for Chrome, can you please commit for me?

LGTM! I will do this for you.

[sjg1, 2011-01-13 22:54:19]

Thank you

On Thu, Jan 13, 2011 at 2:44 PM, <tfarina@chromium.org> wrote:

> On 2011/01/13 22:11:03, sjg wrote:
>
>> On 2011/01/13 21:03:59, Antony Sargent wrote:
>> > lgtm
>>
>
>  Thanks, as I am not a committer for Chrome, can you please commit for me?
>>
>
> LGTM! I will do this for you.
>
>
> http://codereview.chromium.org/6249003/
>

[tfarina, 2011-01-13 22:56:15]

On 2011/01/13 22:54:19, sjg_google.com wrote:
> Thank you
> 
I'll be landing it here: http://codereview.chromium.org/6288003/
when it passes the trybots.

[tfarina, 2011-01-14 02:18:38]

On 2011/01/13 22:56:15, tfarina wrote:
> On 2011/01/13 22:54:19, http://sjg_google.com wrote:
> > Thank you
> > 
> I'll be landing it here: http://codereview.chromium.org/6288003/
> when it passes the trybots.

Landed in http://src.chromium.org/viewvc/chrome?view=rev&revision=71408

[sjg, 2011-01-14 02:55:14]

Thanks very much with your help with this!

On Thu, Jan 13, 2011 at 6:18 PM, <tfarina@chromium.org> wrote:

> On 2011/01/13 22:56:15, tfarina wrote:
>
>  On 2011/01/13 22:54:19, http://sjg_google.com wrote:
>> > Thank you
>> >
>> I'll be landing it here: http://codereview.chromium.org/6288003/
>> when it passes the trybots.
>>
>
> Landed in http://src.chromium.org/viewvc/chrome?view=rev&revision=71408
>
>
> http://codereview.chromium.org/6249003/
>

[sjg, 2011-01-14 02:56:42]

Thanks very much with your help with this!

On Thu, Jan 13, 2011 at 6:18 PM, <tfarina@chromium.org> wrote:

> On 2011/01/13 22:56:15, tfarina wrote:
>
>  On 2011/01/13 22:54:19, http://sjg_google.com wrote:
>> > Thank you
>> >
>> I'll be landing it here: http://codereview.chromium.org/6288003/
>> when it passes the trybots.
>>
>
> Landed in http://src.chromium.org/viewvc/chrome?view=rev&revision=71408
>
>
> http://codereview.chromium.org/6249003/
>

[willchan no longer on Chromium, 2011-01-14 18:20:29]

http://codereview.chromium.org/6249003/diff/8001/chrome/test/url_fetch_test/u...
File chrome/test/url_fetch_test/url_fetch_test.cc (right):

http://codereview.chromium.org/6249003/diff/8001/chrome/test/url_fetch_test/u...
chrome/test/url_fetch_test/url_fetch_test.cc:75: wait_js_frame_xpath,
We're trying to limit the amount of wstring usage in our code.  Please pass
std::string everywhere and then convert it to wstring at the last place that you
have to, which is probably right here at the WaitUntilJavaScriptCondition()
call.  That way, we minimize the amount of places we use wstring.

[sjg, 2011-01-20 20:41:17]

This change just deals with Will's request to reduce use of std::wstring. I have
re-opened this issue until it is resolved.

Hopefully it is still OK to use std::string

[tfarina, 2011-01-21 00:40:36]

On 2011/01/20 20:41:17, sjg wrote:
> This change just deals with Will's request to reduce use of std::wstring. I
have
> re-opened this issue until it is resolved.
>
No, don't do that. Instead of reopening a issue that was already landed. Please
create a new one. Using an existing one will causes some confusion in the
history.

> Hopefully it is still OK to use std::string
Probably will be OK, but open a new issue for this change.

[sjg, 2011-01-21 16:59:18]

On 2011/01/21 00:40:36, tfarina wrote:
> On 2011/01/20 20:41:17, sjg wrote:
> > This change just deals with Will's request to reduce use of std::wstring. I
> have
> > re-opened this issue until it is resolved.
> >
> No, don't do that. Instead of reopening a issue that was already landed.
Please
> create a new one. Using an existing one will causes some confusion in the
> history.
> 
> > Hopefully it is still OK to use std::string
> Probably will be OK, but open a new issue for this change.

Closed in favor of:

http://codereview.chromium.org/6243010