Remove old Sleep and PostDelayedTask interfaces that use int ms instead of TimeDelta.

Remove old PostDelayedTask interfaces that use int ms instead of TimeDelta.

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

Reverted due to crash in cros_x86 and cros_tegra2 builds.

[Ted Vessenes, 2012-03-15 04:48:27]

sky: Please review content/browser/*
sergeyu: Please reviewremoting/*
jam: The rest is yours

This should be the final patch in CR 108171, and as such, it removes a bunch of
old interfaces that code has relied on for a while.  So this definitely needs
some serious try-bot time.  Last attempt at this patch turned up a bunch of
stuff, but hopefully this one will be clean.

[michaeln, 2012-03-15 05:25:09]

there are a couple new overrides in
src/webkit/dom_storage/dom_storage_task_runner.h that should be removed too

[Ted Vessenes, 2012-03-15 12:39:34]

On 2012/03/15 05:25:09, michaeln wrote:
> there are a couple new overrides in
> src/webkit/dom_storage/dom_storage_task_runner.h that should be removed too

Thanks for the catch.  Just wrote quick code fixes and added you as reviewer. 
Didn't have time to wait for compilation to finish before dashing off to work,
but I uploaded them at least.

[jam, 2012-03-15 15:05:31]

content lgtm. note i'm not an owner in webkit

[Ted Vessenes, 2012-03-15 15:09:38]

On 2012/03/15 15:05:31, John Abd-El-Malek wrote:
> content lgtm. note i'm not an owner in webkit

By the way, do you mind shoving these to the try bots even before collecting all
of the LGTMs?  It would be nice to see if any errors crop up ahead of time, but
I don't have try bot access (yet).

[michaeln, 2012-03-15 16:31:46]

lgtm2

[sky, 2012-03-15 17:18:44]

LGTM

[Jamie, 2012-03-15 17:33:10]

remoting/ lgtm

[Ted Vessenes, 2012-03-15 17:36:06]

I think that's a full set of LGTMs.  Has anyone sent this to the try bots before
attempting a commit?  I'm naturally wary.

[sky, 2012-03-15 19:03:30]

If you click the commit button it'll run it through the try bots for you.

[Ted Vessenes, 2012-03-15 19:17:41]

On 2012/03/15 19:03:30, sky wrote:
> If you click the commit button it'll run it through the try bots for you.

I was specifically hoping to try the release bots (not all of which are run in
the standard commit I believe), and get a thumbs up/down before the CL is
committed to the tree.  I've had a few issues in previous patches where a CL
would be accepted, but then fail on releases.  This is generally caused by a
missing include of base/time.h, which for some reason is automatically included
everywhere by the standard commit bots but not by the release bots.

That said, I guess I'll give it a go.

[commit-bot: I haz the power, 2012-03-15 19:26:53]

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

[commit-bot: I haz the power, 2012-03-15 19:27:05]

Presubmit check for 9703053-2002 failed and returned exit status 1.

Running presubmit commit checks ...

** Presubmit Messages **
If this change requires manual test instructions to QA team, add
TEST=[instructions].

** Presubmit ERRORS **
Missing LGTM from an OWNER for files in these directories:
    base

Presubmit checks took 3.2s to calculate.

[Ted Vessenes, 2012-03-15 19:28:32]

Looks like I need a review from jar@chromium.org as well.

[jar (doing other things), 2012-03-15 19:42:12]

Please either avoid using the commit queue for this, or at least be around when
you land.  The potential for bustage due to the changing API, and having someone
land an old API call, is too great.

Thanks.

LGTM

[sky, 2012-03-15 19:44:42]

You can always explicitly run certain bots if you so desire.

  -Scott

On Thu, Mar 15, 2012 at 12:17 PM,  <tedvessenes@gmail.com> wrote:
> On 2012/03/15 19:03:30, sky wrote:
>>
>> If you click the commit button it'll run it through the try bots for you.
>
>
> I was specifically hoping to try the release bots (not all of which are run
> in
> the standard commit I believe), and get a thumbs up/down before the CL is
> committed to the tree.  I've had a few issues in previous patches where a CL
> would be accepted, but then fail on releases.  This is generally caused by a
> missing include of base/time.h, which for some reason is automatically
> included
> everywhere by the standard commit bots but not by the release bots.
>
> That said, I guess I'll give it a go.
>
> http://codereview.chromium.org/9703053/

[Ted Vessenes, 2012-03-15 19:48:11]

On 2012/03/15 19:44:42, sky wrote:
> You can always explicitly run certain bots if you so desire.

Are you certain?  I was under the strong impression I didn't have authorization
for that.  Remember, I don't (yet) have a chromium.org email address yet.  In
the past, other people have had to send my CLs to the try bots unless the CL was
sent directly to the commit queue.

If I do have permission to use those bots after all, I'm happy to do that if
someone can give me a link to them.

[Sergey Ulanov, 2012-03-15 19:58:50]

remoting/ - LGTM

https://chromiumcodereview.appspot.com/9703053/diff/2002/webkit/dom_storage/d...
File webkit/dom_storage/dom_storage_task_runner.h (right):

https://chromiumcodereview.appspot.com/9703053/diff/2002/webkit/dom_storage/d...
webkit/dom_storage/dom_storage_task_runner.h:42: // terms of PostDelayedTask
nit: add period at then end of the sentence

[sky, 2012-03-15 21:12:04]

Sorry, you're probably right.

  -Scott

On Thu, Mar 15, 2012 at 12:48 PM,  <tedvessenes@gmail.com> wrote:
> On 2012/03/15 19:44:42, sky wrote:
>>
>> You can always explicitly run certain bots if you so desire.
>
>
> Are you certain?  I was under the strong impression I didn't have
> authorization
> for that.  Remember, I don't (yet) have a chromium.org email address yet.
>  In
> the past, other people have had to send my CLs to the try bots unless the CL
> was
> sent directly to the commit queue.
>
> If I do have permission to use those bots after all, I'm happy to do that if
> someone can give me a link to them.
>
>
> http://codereview.chromium.org/9703053/

[michaeln, 2012-03-17 02:46:27]

eta on committing this change? seems like nice cleaning up of cruft

[Ted Vessenes, 2012-03-17 02:51:47]

On 2012/03/17 02:46:27, michaeln wrote:
> eta on committing this change? seems like nice cleaning up of cruft

I'm going to submit it tomorrow morning, when I'll be around most of the day to
monitor it if things blow up.  Didn't want to be mean to the Friday night shift
or I would have submitted now.

[michaeln, 2012-03-17 03:44:05]

great :)

[commit-bot: I haz the power, 2012-03-17 15:14:02]

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

[commit-bot: I haz the power, 2012-03-17 15:30:05]

Try job failure for 9703053-9001 (retry) on win for step "compile" (clobber
build).
It's a second try, previously, step "compile" failed.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win&number...

[Ted Vessenes, 2012-03-17 18:56:38]

Alright, the summary is that there's a bunch of non-linux gcc platform bugs to
handle.  Windows only has one failure but mac has almost a dozen, and of course
there's the clang and chromeos stuff.  I'll get to these issues in the coming
week and submit them in separate CLs, then try this again later.

[Ted Vessenes, 2012-05-24 03:44:03]

Finally got all the other bugs work out, and try bots seem to be accepting this
patch.  There were two additional minor changes since two months ago, one to
remove an additional PostDelayedTask() interface implementation and another to
fix a unit test for the old interface.

I'm going to submit it tomorrow evening when I have time to watch it in case
anything goes wrong, but it looks safe enough.

[jar (doing other things), 2012-05-24 17:54:57]

Although I like the abstract cleanliness, I'm wondering if this transition to
TimeDelta has a measurable impact via "death by 1000 cuts."  It changes what was
historically a passing of an int via a small register, into: 

1)a call to a 64 bit library to do a 64 bit multiply by 100, 
2) passing of a 64 bit const ref object (probably in a register... if we're
lucky)
3) a call to a 64 bit library, and a 64 bit divide by 1000.

Perhaps on some machines we have native 64bit arithmetic... but since we don't
ship a 64 version of Chrome on Windows, for the bulk of our users, this is a
giant de-optimization... but I haven't quantified it.

Is clarity improving that much?  Is cost not an issues?

Comments?

[Ted Vessenes, 2012-05-24 18:56:22]

I was actually thinking about this today when I was faced with implementing a
TimeDelta equivalent for another project.  My intuition is that there's some
processor cost but not as much as you'd think.  In the literally hundreds of
places I've updated, there's been no real consistency in what types people use
for their times, except that floating point values are exceedingly rare.  But
I've seen all permutations of 32 and 64 bit integers, and both signed and
unsigned.  So even without these changes, we were already paying some of the
cost of 64 bit math on 32 bit machines.  Now we're just paying all of it.

This cost is pretty meaningless before calling a sleep(), for what it's worth. 
If you're going to sleep for even 50 milliseconds, then spending another 4 to 20
instructions just doesn't matter.  It does seem relevant in the case of
PostDelayedTask() and friends though, where the code will continue to do other
things.

Ultimately this is a question of how much ugliness the interface needs to
sustain in order to get acceptable performance.  I can only see two justifiable
answers.  Either support only 64 bit time deltas, or support both 64 bit and 32
bits.  If it really matters-- and I have neither a way of testing nor a belief
that it substantively does-- the proper way to solve it would be to have
TimeDelta32 and TimeDelta64 objects.  Then sections of code that were time
critical and compiled for 32 bits could use a different PostDelayedTask()
interface with the 32 bit version of the class.

My personal opinion is that clarity is improving a small amount in a very large
number of places.  If you want to measure it, we can talk about how to do that,
though it seems difficult to do so.  These changes have been slowly added to the
code base over the past 5 months.  While there might be a solid before and after
set of versions to test, so much other code has changed as well that would also
affect performance.  So any measurements would have a ton of fuzz.  I guess a
better question might be how relatively happy we are with Chrome's performance
today.

[commit-bot: I haz the power, 2012-05-25 02:19:37]

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

[commit-bot: I haz the power, 2012-05-25 02:36:08]

Try job failure for 9703053-38002 (retry) on win_rel for step "compile" (clobber
build).
It's a second try, previously, step "compile" failed.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...

[commit-bot: I haz the power, 2012-05-25 14:08:23]

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

[commit-bot: I haz the power, 2012-05-25 14:30:39]

Try job failure for 9703053-38002 (retry) on win for step "compile" (clobber
build).
It's a second try, previously, step "compile" failed.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win&number...

[Ted Vessenes, 2012-05-28 15:28:54]

Found two remaining Windows issues to correct.  Try bots pass after these fixes.

abodenha: Please review cloudprint/*
joaodasilva: Please review chrome/browser/policy/*

[Albert Bodenhamer, 2012-05-29 04:39:41]

LGTM
Reviewed Cloud Print only.

[Joao da Silva, 2012-05-29 09:18:16]

chrome/browser/policy lgtm

[commit-bot: I haz the power, 2012-05-29 13:13:13]

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

[commit-bot: I haz the power, 2012-05-29 13:13:25]

Failed to apply patch for cloud_print/service/win/service_state.cc:
While running patch -p1 --forward --force;
patching file cloud_print/service/win/service_state.cc
Hunk #1 FAILED at 164.
1 out of 2 hunks FAILED -- saving rejects to file
cloud_print/service/win/service_state.cc.rej

[commit-bot: I haz the power, 2012-05-30 03:25:17]

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

[commit-bot: I haz the power, 2012-05-30 03:25:33]

Failed to apply patch for cloud_print/service/win/service_state.cc:
While running patch -p1 --forward --force;
patching file cloud_print/service/win/service_state.cc
Hunk #1 FAILED at 164.
1 out of 2 hunks FAILED -- saving rejects to file
cloud_print/service/win/service_state.cc.rej

[michaeln, 2012-05-30 03:39:06]

http://codereview.chromium.org/9703053/diff/33005/webkit/dom_storage/dom_stor...
File webkit/dom_storage/dom_storage_task_runner.cc (right):

http://codereview.chromium.org/9703053/diff/33005/webkit/dom_storage/dom_stor...
webkit/dom_storage/dom_storage_task_runner.cc:24: return
PostDelayedTask(from_here, task, delay);
whoa... this looks like a recursive call back onto this same method... something
seems amiss here?

[Ted Vessenes, 2012-05-30 03:56:08]

Yeah, that function should have been deleted, I believe.  I'm a wee bit
suspicious that the try bots didn't catch it though.

[commit-bot: I haz the power, 2012-05-30 12:45:45]

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

[commit-bot: I haz the power, 2012-05-30 12:45:57]

Failed to apply patch for cloud_print/service/win/service_state.cc:
While running patch -p1 --forward --force;
patching file cloud_print/service/win/service_state.cc
Hunk #1 FAILED at 164.
1 out of 2 hunks FAILED -- saving rejects to file
cloud_print/service/win/service_state.cc.rej

[Ted Vessenes, 2012-05-30 12:49:45]

Alright, looking into why this patch isn't applying.  First I thought it just
wasn't rebasing because of conflicting changes.  Then I thought the patch was
confused by my attempts to remove the windows ^Ms at the end of some lines. 
(Like that whitespace wasn't properly communicated or something.)  But I guess
it's something else.  Apologies for all of the reviewer spam, as I know there's
a number of only tangentially interested ones.

[michaeln, 2012-05-30 17:44:54]

http://codereview.chromium.org/9703053/diff/33005/webkit/dom_storage/dom_stor...
File webkit/dom_storage/dom_storage_task_runner.cc (right):

http://codereview.chromium.org/9703053/diff/33005/webkit/dom_storage/dom_stor...
webkit/dom_storage/dom_storage_task_runner.cc:20: bool
DomStorageTaskRunner::PostDelayedTask(
Also, please remember to remove this method with infinite recursion.

[commit-bot: I haz the power, 2012-05-31 01:06:52]

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

[commit-bot: I haz the power, 2012-05-31 01:07:21]

Failed to apply patch for cloud_print/service/win/service_state.cc:
While running svn propset svn:eol-style LF
cloud_print/service/win/service_state.cc --config-dir
/mnt/data/b/commit-queue/subversion_config --non-interactive;
patching file cloud_print/service/win/service_state.cc
svn: File 'cloud_print/service/win/service_state.cc' has inconsistent newlines
svn: Inconsistent line ending style

[Ted Vessenes, 2012-05-31 01:34:02]

So this is odd.  The file as it exists in the repository contains windows style
^M newlines.  Attempting to remove these from the lines causes the patch to fail
because they aren't preserved in the - section of the diff upload (so the patch
sections don't match), which was the preceding error.  Not removing them causes
this error, where the file doesn't want to be committed due to containing
windows newlines.

I'm not sure if I have the permissions to fix this.  Can someone else check in a
fixed version of this file with all the ^Ms trimmed out?

On 2012/05/31 01:07:21, I haz the power (commit-bot) wrote:
> Failed to apply patch for cloud_print/service/win/service_state.cc:
> While running svn propset svn:eol-style LF
> cloud_print/service/win/service_state.cc --config-dir
> /mnt/data/b/commit-queue/subversion_config --non-interactive;
> patching file cloud_print/service/win/service_state.cc
> svn: File 'cloud_print/service/win/service_state.cc' has inconsistent newlines
> svn: Inconsistent line ending style

[Joao da Silva, 2012-05-31 08:52:16]

On Thu, May 31, 2012 at 3:34 AM, <tedvessenes@gmail.com> wrote:

> So this is odd.  The file as it exists in the repository contains windows
> style
> ^M newlines.  Attempting to remove these from the lines causes the patch
> to fail
> because they aren't preserved in the - section of the diff upload (so the
> patch
> sections don't match), which was the preceding error.  Not removing them
> causes
> this error, where the file doesn't want to be committed due to containing
> windows newlines.
>
> I'm not sure if I have the permissions to fix this.  Can someone else
> check in a
> fixed version of this file with all the ^Ms trimmed out?


http://crrev.com/139756

I'll tick the box in an hour or so.


>
>
> On 2012/05/31 01:07:21, I haz the power (commit-bot) wrote:
>
>> Failed to apply patch for cloud_print/service/win/**service_state.cc:
>> While running svn propset svn:eol-style LF
>> cloud_print/service/win/**service_state.cc --config-dir
>> /mnt/data/b/commit-queue/**subversion_config --non-interactive;
>> patching file cloud_print/service/win/**service_state.cc
>> svn: File 'cloud_print/service/win/**service_state.cc' has inconsistent
>> newlines
>> svn: Inconsistent line ending style
>>
>
>
>
>
https://chromiumcodereview.**appspot.com/9703053/<https://chromiumcodereview....
>

[commit-bot: I haz the power, 2012-05-31 13:24:15]

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

[commit-bot: I haz the power, 2012-05-31 13:55:39]

Try job failure for 9703053-31007 (retry) on android for steps "compile, build"
(clobber build).
It's a second try, previously, steps "compile, build" failed.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android&nu...

[commit-bot: I haz the power, 2012-06-01 18:43:05]

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

[commit-bot: I haz the power, 2012-06-01 21:33:04]

Change committed as 140102