Update uses of TimeDelta in chrome_frame/*.

Update uses of TimeDelta in chrome_frame/*.  Also update a bit of TimeDelta use in webkit npapi code.

Adding robertshield for his thoughts on this.


R=ananta@chromium.org
BUG=108171
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=133395

[Ted Vessenes, 2012-04-13 17:02:44]

Just poking in on this.  Do you have time to look at this, or should I look for
another reviewer?

[ananta, 2012-04-13 17:50:21]

Just some nits and one question.

https://chromiumcodereview.appspot.com/10019015/diff/1/chrome_frame/test/auto...
File chrome_frame/test/automation_client_mock.cc (right):

https://chromiumcodereview.appspot.com/10019015/diff/1/chrome_frame/test/auto...
chrome_frame/test/automation_client_mock.cc:49:
base::TimeDelta::FromMilliseconds(params->launch_timeout()) / 2);
Would this work correctly?. The launch timeout appears to be specified in
seconds above.

https://chromiumcodereview.appspot.com/10019015/diff/1/chrome_frame/test/mock...
File chrome_frame/test/mock_ie_event_sink_actions.h (right):

https://chromiumcodereview.appspot.com/10019015/diff/1/chrome_frame/test/mock...
chrome_frame/test/mock_ie_event_sink_actions.h:314:
base::TimeDelta::FromMilliseconds(delay));
Please verify whether the tests specify this timeout in milliseconds. From what
I remember the chrome frame tests specify the timeouts in seconds.

https://chromiumcodereview.appspot.com/10019015/diff/1/chrome_frame/test/perf...
File chrome_frame/test/perf/chrome_frame_perftest.cc (right):

https://chromiumcodereview.appspot.com/10019015/diff/1/chrome_frame/test/perf...
chrome_frame/test/perf/chrome_frame_perftest.cc:343:
base::PlatformThread::Sleep(base::TimeDelta::FromMilliseconds(50));
Are there plans to deprecate the other versions of Sleep like the one which
accepts the values in ms?

https://chromiumcodereview.appspot.com/10019015/diff/1/chrome_frame/test/test...
File chrome_frame/test/test_server.cc (right):

https://chromiumcodereview.appspot.com/10019015/diff/1/chrome_frame/test/test...
chrome_frame/test/test_server.cc:341:
base::TimeDelta::FromMilliseconds(options_.timeout_));
Please check here whether the timeout is specified in ms.

[Ted Vessenes, 2012-04-15 22:25:23]

I looked into this and all calls to PostDelayedTask(..., int) expect
milliseconds.  And I even traced this through things like the
params->launch_timeout() function, which records the timeout in milliseconds. 
Arguably the parameter should internally store a TimeDelta as well, but this has
been a system-wide refactoring and I'm trying to keep the patches from getting
too complicated or interdependent.

The long term plan is to deprecate all versions of Sleep and PostDelayedTask
that specify time in integers instead of TimeDelta.

[ananta, 2012-04-17 00:13:32]

Robert, please take a look at this CL.

[ananta, 2012-04-17 00:13:57]

LGTM from me. Please wait for Robert Shields LGTM.

[grt (UTC plus 2), 2012-04-17 14:13:32]

LG, nits below.

https://chromiumcodereview.appspot.com/10019015/diff/7001/chrome_frame/delete...
File chrome_frame/delete_chrome_history.cc (right):

https://chromiumcodereview.appspot.com/10019015/diff/7001/chrome_frame/delete...
chrome_frame/delete_chrome_history.cc:1: // Copyright (c) 2011 The Chromium
Authors. All rights reserved.
2012

https://chromiumcodereview.appspot.com/10019015/diff/7001/chrome_frame/test/c...
File chrome_frame/test/chrome_frame_automation_mock.cc (right):

https://chromiumcodereview.appspot.com/10019015/diff/7001/chrome_frame/test/c...
chrome_frame/test/chrome_frame_automation_mock.cc:9: const int kLongWaitTimeout
= 25 * 1000;
why not:
const base::TimeDelta kLongWaitTimeout =  base::TimeDelta::FromSeconds(25);
there's no need to worry about static ctors here in the test code

https://chromiumcodereview.appspot.com/10019015/diff/7001/chrome_frame/test/c...
chrome_frame/test/chrome_frame_automation_mock.cc:13: AutomationMockLaunch
mock_launch(&loop, kLongWaitTimeout);
and this would become kLongWaitTimeout.ToMilliseconds(), which makes it more
clear anyway.

https://chromiumcodereview.appspot.com/10019015/diff/7001/webkit/plugins/npap...
File webkit/plugins/npapi/webplugin_delegate_impl_win.cc (right):

https://chromiumcodereview.appspot.com/10019015/diff/7001/webkit/plugins/npap...
webkit/plugins/npapi/webplugin_delegate_impl_win.cc:1: // Copyright (c) 2012 The
Chromium Authors. All rights reserved.
please remove this file from the CL since it's unrelated to chrome_frame

[Ted Vessenes, 2012-04-18 01:42:36]

I like the idea of storing the delay directly, but I know that creating any
static global class instance variables will cause most builds to fail (since
their order of construction isn't well defined).  I could define the delay in
seconds though, and change the function bodies to covert to TimeDelta
immediately and then use .InMilliseconds() to do the multiplication by 1000. 
That's a bit wordier but it feels more correct.  Would you like me to do that?

I'll cut out the webkit stuff.  My review assignment script probably lopped it
into this CL because it could be reviewed by the same person.  (Last block of
work was separated into 22 separate CLs for review, so I've been juggling a lot
of balls.)  I'll put it into a separate review.

[grt (UTC plus 2), 2012-04-18 02:04:24]

https://chromiumcodereview.appspot.com/10019015/diff/7001/chrome_frame/test/c...
File chrome_frame/test/chrome_frame_automation_mock.cc (right):

https://chromiumcodereview.appspot.com/10019015/diff/7001/chrome_frame/test/c...
chrome_frame/test/chrome_frame_automation_mock.cc:9: const int kLongWaitTimeout
= 25 * 1000;
On 2012/04/17 14:13:32, grt wrote:
> why not:
> const base::TimeDelta kLongWaitTimeout =  base::TimeDelta::FromSeconds(25);
> there's no need to worry about static ctors here in the test code

On 2012/04/18 01:42:36, tedv wrote:
> I like the idea of storing the delay directly, but I know that creating any
> static global class instance variables will cause most builds to fail (since
> their order of construction isn't well defined).

I had assumed that the ban on static initializers didn't apply to test code. 
The ones I added in https://chromiumcodereview.appspot.com/10053004/ haven't
been problematic so far, but maybe I broke the rules.  If so, oops.

If you'd like to keep this as an int to be on the safe side, how about just
changing the name to kLongWaitTimeoutMs so that the units are clear at the
points of use?

https://chromiumcodereview.appspot.com/10019015/diff/7001/webkit/plugins/npap...
File webkit/plugins/npapi/webplugin_delegate_impl_win.cc (right):

https://chromiumcodereview.appspot.com/10019015/diff/7001/webkit/plugins/npap...
webkit/plugins/npapi/webplugin_delegate_impl_win.cc:1: // Copyright (c) 2012 The
Chromium Authors. All rights reserved.
> I'll cut out the webkit stuff.  My review assignment script probably lopped it
> into this CL because it could be reviewed by the same person.  (Last block of
> work was separated into 22 separate CLs for review, so I've been juggling a
lot
> of balls.)  I'll put it into a separate review.

I have no major objection to it being in this one if it makes your life simpler.
 Please update the description if you do, though.

[Ted Vessenes, 2012-04-18 02:14:28]

I'll try using static initializers here as you first suggested and sent it to
the try bots first.  If that's clean, I'll submit it as is.  Description will be
updated.

[robertshield, 2012-04-18 02:23:09]

https://chromiumcodereview.appspot.com/10019015/diff/7001/chrome_frame/test/c...
File chrome_frame/test/chrome_frame_test_utils.h (right):

https://chromiumcodereview.appspot.com/10019015/diff/7001/chrome_frame/test/c...
chrome_frame/test/chrome_frame_test_utils.h:207: const base::Closure& task,
int64 delay_ms) {
IMO for consistency, should modify this method to take a TimeDelta reference
instead of an int64 as well. The fallout from doing this will not be huge.

[Ted Vessenes, 2012-04-22 22:52:16]

Enough little stuff changed that it's probably worth a second look at latest
changes.  I'm double checking with the try bots that this patch is safe but I
think it will be good.

[robertshield, 2012-04-23 00:28:50]

LGTM

[commit-bot: I haz the power, 2012-04-23 00:30:34]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/tedvessenes@gmail.com/10019015/38002

[commit-bot: I haz the power, 2012-04-23 01:40:04]

Change committed as 133395