s/s_currentTask/s_staticCurrentTask/ to help rewrite_to_chrome_style tool.

Rename m_currentTask and s_currentTask/ to help rewrite_to_chrome_style tool.

This CL helps rewrite_to_chrome_style tool, by preventing the tool from
suggesting the following conflicting renames:
- |s_field| -> |field_|
- |m_field| -> |field_|

BUG=641004
Committed: https://crrev.com/1f0dfac7f60c84a476ce546a5ecc1687d4e7b658
Cr-Commit-Position: refs/heads/master@{#438058}

[Łukasz Anforowicz, 2016-12-09 17:37:47]

lukasza@chromium.org changed reviewers:
+ dcheng@chromium.org

[Łukasz Anforowicz, 2016-12-09 17:37:47]

Daniel, can you take a look please?

[Łukasz Anforowicz, 2016-12-09 17:37:52]

The CQ bit was checked by lukasza@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-12-09 17:38:42]

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

[dcheng, 2016-12-09 18:21:20]

What is the difference between m_currentTask and s_currentTask? Maybe we can
pick more meaningful names?

[commit-bot: I haz the power, 2016-12-09 20:19:22]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-12-09 20:19:23]

Dry run: This issue passed the CQ dry run.

[Łukasz Anforowicz, 2016-12-10 00:55:31]

The CQ bit was checked by lukasza@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-12-10 00:56:47]

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

[Łukasz Anforowicz, 2016-12-10 01:01:41]

On 2016/12/09 18:21:20, dcheng wrote:
> What is the difference between m_currentTask and s_currentTask? Maybe we can
> pick more meaningful names?

Thanks for the feedback - I appreciate it.  You're right - we should have better
names here.  I tried to tweak the names in patchset #2.

I am not sure if I fully understand the changed class, so let me also ask one of
core/animation/OWNERS for feedback.

https://codereview.chromium.org/2569433002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/animation/AnimationClock.h (left):

https://codereview.chromium.org/2569433002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/animation/AnimationClock.h:53: void
updateTime(double time);
It might also make sense to:
- s/updateTime/updateTargetTime/
- s/time/targetTime/

I didn't do this yet, because:
- this would require touching extra 7 files
- I am not sure I understand this class and how it is used (and therefore I am
not sure if the new name is really better).

Similarily, we can do:
- s/m_time/m_targetTime/
- s/currentTime/targetTime/g
- s/resetTimeForTesting/resetTargetTimeForTesting/

[Łukasz Anforowicz, 2016-12-10 01:04:29]

lukasza@chromium.org changed reviewers:
+ suzyh@chromium.org

[Łukasz Anforowicz, 2016-12-10 01:04:30]

suzyh@, could you also take a look please?  Daniel is right that I shouldn't
just rename s_currentTime to s_staticCurrentTime (although it does avoid a
conflict after the big rename), but that I should come up with better names
here.  OTOH, I am not that familiar with AnimationClock class - could you please
double-check that the changes I proposed make sense?

[Łukasz Anforowicz, 2016-12-10 01:05:20]

Description was changed from

==========
s/s_currentTask/s_staticCurrentTask/ to help rewrite_to_chrome_style tool.

This CL helps rewrite_to_chrome_style tool, by preventing the tool from
suggesting the following conflicting renames:
- |s_field| -> |field_|
- |m_field| -> |field_|

BUG=641004
==========

to

==========
Rename m_currentTask and s_currentTask/ to help rewrite_to_chrome_style tool.

This CL helps rewrite_to_chrome_style tool, by preventing the tool from
suggesting the following conflicting renames:
- |s_field| -> |field_|
- |m_field| -> |field_|

BUG=641004
==========

[commit-bot: I haz the power, 2016-12-10 03:50:03]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-12-10 03:50:03]

Dry run: This issue passed the CQ dry run.

[suzyh_UTC10 (ex-contributor), 2016-12-12 03:52:49]

suzyh@chromium.org changed reviewers:
+ dstockwell@chromium.org

[suzyh_UTC10 (ex-contributor), 2016-12-12 03:54:01]

+dstockwell as the original author of this class. Doug, do you have any
recommendations for this patch?

I'm more or less happy with the patch, aside from the comment about making it a
no-op.

https://codereview.chromium.org/2569433002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/animation/AnimationClock.cpp (right):

https://codereview.chromium.org/2569433002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/animation/AnimationClock.cpp:78:
m_taskForWhichTimeWasCalculated = s_currentlyRunningTask;
I'd prefer that this patch was only a rename, without the control flow change
(moving this line out of the "else" clause) or the DCHECK changes. While I'm
happy for those changes to also occur, I'd like to have the variable name
changes in a no-op patch.

https://codereview.chromium.org/2569433002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/animation/AnimationClock.h (left):

https://codereview.chromium.org/2569433002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/animation/AnimationClock.h:53: void
updateTime(double time);
On 2016/12/10 at 01:01:41, Łukasz Anforowicz wrote:
> It might also make sense to:
> - s/updateTime/updateTargetTime/
> - s/time/targetTime/
> 
> I didn't do this yet, because:
> - this would require touching extra 7 files
> - I am not sure I understand this class and how it is used (and therefore I am
not sure if the new name is really better).
> 
> Similarily, we can do:
> - s/m_time/m_targetTime/
> - s/currentTime/targetTime/g
> - s/resetTimeForTesting/resetTargetTimeForTesting/

Likewise deferring to dstockwell for suggestions here.

https://codereview.chromium.org/2569433002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/animation/AnimationClock.h (right):

https://codereview.chromium.org/2569433002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/animation/AnimationClock.h:43: // Tracks the time
at which the currently running task should finish,
I'm not entirely sure if this is the best way to describe the function of this
class. Deferring to dstockwell for suggestions.

[Łukasz Anforowicz, 2016-12-12 17:41:12]

dstockwell@, can you PTAL?  There are two things that need your attention:

1. Please review the latest patchset (i.e. review the accuracy of the new
variable names and comments).  This is higher priority - landing a CL that
renames the variables is needed to help with the "big rename" in Blink.

2. Please provide feedback on whether further renames and refactorings are
desirable / worth it (see previous CR comments).  This might be lower priority
(this doesn't impact the "big rename").

https://codereview.chromium.org/2569433002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/animation/AnimationClock.cpp (right):

https://codereview.chromium.org/2569433002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/animation/AnimationClock.cpp:78:
m_taskForWhichTimeWasCalculated = s_currentlyRunningTask;
On 2016/12/12 03:54:01, suzyh wrote:
> I'd prefer that this patch was only a rename, without the control flow change
> (moving this line out of the "else" clause) or the DCHECK changes. While I'm
> happy for those changes to also occur, I'd like to have the variable name
> changes in a no-op patch.

Done.

[alancutter (OOO until 2018), 2016-12-12 23:19:22]

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

[alancutter (OOO until 2018), 2016-12-12 23:19:23]

https://codereview.chromium.org/2569433002/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/animation/AnimationClock.h (right):

https://codereview.chromium.org/2569433002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/animation/AnimationClock.h:44: // if finishing
before the next animation frame is desired.
// Maintains a stationary clock time during script execution.
// Ideally tracks the glass time (the moment photons leave the screen) of the
current frame.

[Łukasz Anforowicz, 2016-12-12 23:40:33]

https://codereview.chromium.org/2569433002/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/animation/AnimationClock.h (right):

https://codereview.chromium.org/2569433002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/animation/AnimationClock.h:44: // if finishing
before the next animation frame is desired.
On 2016/12/12 23:19:23, alancutter wrote:
> // Maintains a stationary clock time during script execution.
> // Ideally tracks the glass time (the moment photons leave the screen) of the
> current frame.

Done, except that:

- I used "tries to track" instead of "ideally tracks", because the latter is
slightly abiguous ("tries to track" VS "perfectly tracks")

- I used "animation frame" instead of "frame" (to avoid ambiguity with
blink::Frame).


Also - given your feedback I doubt more and more my understanding of what this
class does, so I also got rid of the comments I tried to helpfully add to
updateTime and currentTime methods.  I think that I do understand what
s_currentlyRunningTask and m_taskForWhichTimeWasCalculated are doing, so I am
still adding the 2 comments related to these fields.

[alancutter (OOO until 2018), 2016-12-12 23:47:57]

On 2016/12/12 at 23:40:33, lukasza wrote:
>
https://codereview.chromium.org/2569433002/diff/40001/third_party/WebKit/Sour...
> File third_party/WebKit/Source/core/animation/AnimationClock.h (right):
> 
>
https://codereview.chromium.org/2569433002/diff/40001/third_party/WebKit/Sour...
> third_party/WebKit/Source/core/animation/AnimationClock.h:44: // if finishing
before the next animation frame is desired.
> On 2016/12/12 23:19:23, alancutter wrote:
> > // Maintains a stationary clock time during script execution.
> > // Ideally tracks the glass time (the moment photons leave the screen) of
the
> > current frame.
> 
> Done, except that:
> 
> - I used "tries to track" instead of "ideally tracks", because the latter is
slightly abiguous ("tries to track" VS "perfectly tracks")
> 
> - I used "animation frame" instead of "frame" (to avoid ambiguity with
blink::Frame).
> 
> 
> Also - given your feedback I doubt more and more my understanding of what this
class does, so I also got rid of the comments I tried to helpfully add to
updateTime and currentTime methods.  I think that I do understand what
s_currentlyRunningTask and m_taskForWhichTimeWasCalculated are doing, so I am
still adding the 2 comments related to these fields.

lgtm

[alancutter (OOO until 2018), 2016-12-12 23:52:19]

https://codereview.chromium.org/2569433002/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/animation/AnimationClock.h (right):

https://codereview.chromium.org/2569433002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/animation/AnimationClock.h:72: // this case it
recalculates |m_time|.
Reading this one again it's a bit too much like a prose version of the code in
the CPP file IMO. The purpose of these is already captured by "Maintains a
stationary clock time during script execution", I don't think it's necessary to
document how they get used internally.

[Łukasz Anforowicz, 2016-12-13 00:06:21]

https://codereview.chromium.org/2569433002/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/animation/AnimationClock.h (right):

https://codereview.chromium.org/2569433002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/animation/AnimationClock.h:72: // this case it
recalculates |m_time|.
On 2016/12/12 23:52:18, alancutter wrote:
> Reading this one again it's a bit too much like a prose version of the code in
> the CPP file IMO. The purpose of these is already captured by "Maintains a
> stationary clock time during script execution", I don't think it's necessary
to
> document how they get used internally.

Hmmm... okay.  It is not immediately obvious to me that "stationary clock" =
"clock that is incremented/recalculated once per task" (and the latter is
probably be entirely accurate because of updateTime).  OTOH, maybe the new field
names are sufficiently self-explanatory and the comment is indeed duplicating
some of what can be found in the code.

Done.

[alancutter (OOO until 2018), 2016-12-13 00:19:01]

https://codereview.chromium.org/2569433002/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/animation/AnimationClock.h (right):

https://codereview.chromium.org/2569433002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/animation/AnimationClock.h:45: // frame.
Optional:
The "Maintains a stationary clock time during script execution" probably
deserves a why. Here's the spec text that supports this behaviour, might be
worth linking to: http://w3c.github.io/web-animations/#timing-model-concepts

[Łukasz Anforowicz, 2016-12-13 00:34:04]

https://codereview.chromium.org/2569433002/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/animation/AnimationClock.h (right):

https://codereview.chromium.org/2569433002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/animation/AnimationClock.h:45: // frame.
On 2016/12/13 00:19:01, alancutter wrote:
> Optional:
> The "Maintains a stationary clock time during script execution" probably
> deserves a why. Here's the spec text that supports this behaviour, might be
> worth linking to: http://w3c.github.io/web-animations/#timing-model-concepts

Thanks for the link.  I think I'll leave the comment as-is - mostly because the
link didn't help me with understanding the class or the comment (OTOH, I work on
parts of Chromium and Blink not related to animations, so this might be my
fault;  at any rate - I didn't find definition of "stationary" or an equivalent
of AnimationClock in the linked spec).

[dstockwell, 2016-12-13 01:58:35]

lgtm

[Łukasz Anforowicz, 2016-12-13 02:45:18]

Thanks for the reviews everyone.

[Łukasz Anforowicz, 2016-12-13 02:46:32]

The CQ bit was checked by lukasza@chromium.org

[Łukasz Anforowicz, 2016-12-13 02:46:33]

The patchset sent to the CQ was uploaded after l-g-t-m from
alancutter@chromium.org
Link to the patchset: https://codereview.chromium.org/2569433002/#ps80001
(title: "Got rid of one more comment.")

[commit-bot: I haz the power, 2016-12-13 02:47:37]

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, 2016-12-13 04:41:01]

CQ is committing da patch.

Bot data: {"patchset_id": 80001, "attempt_start_ts": 1481597192973700,
"parent_rev": "5f86e313436ed20d90fd273fc387a66e37dd5509", "commit_rev":
"3057be3da22b3ff7f70f1437bf47cd770db13abb"}

[commit-bot: I haz the power, 2016-12-13 04:41:33]

Description was changed from

==========
Rename m_currentTask and s_currentTask/ to help rewrite_to_chrome_style tool.

This CL helps rewrite_to_chrome_style tool, by preventing the tool from
suggesting the following conflicting renames:
- |s_field| -> |field_|
- |m_field| -> |field_|

BUG=641004
==========

to

==========
Rename m_currentTask and s_currentTask/ to help rewrite_to_chrome_style tool.

This CL helps rewrite_to_chrome_style tool, by preventing the tool from
suggesting the following conflicting renames:
- |s_field| -> |field_|
- |m_field| -> |field_|

BUG=641004

Review-Url: https://codereview.chromium.org/2569433002
==========

[commit-bot: I haz the power, 2016-12-13 04:41:36]

Committed patchset #5 (id:80001)

[commit-bot: I haz the power, 2016-12-13 04:43:23]

Description was changed from

==========
Rename m_currentTask and s_currentTask/ to help rewrite_to_chrome_style tool.

This CL helps rewrite_to_chrome_style tool, by preventing the tool from
suggesting the following conflicting renames:
- |s_field| -> |field_|
- |m_field| -> |field_|

BUG=641004

Review-Url: https://codereview.chromium.org/2569433002
==========

to

==========
Rename m_currentTask and s_currentTask/ to help rewrite_to_chrome_style tool.

This CL helps rewrite_to_chrome_style tool, by preventing the tool from
suggesting the following conflicting renames:
- |s_field| -> |field_|
- |m_field| -> |field_|

BUG=641004

Committed: https://crrev.com/1f0dfac7f60c84a476ce546a5ecc1687d4e7b658
Cr-Commit-Position: refs/heads/master@{#438058}
==========

[commit-bot: I haz the power, 2016-12-13 04:43:24]

Patchset 5 (id:??) landed as
https://crrev.com/1f0dfac7f60c84a476ce546a5ecc1687d4e7b658
Cr-Commit-Position: refs/heads/master@{#438058}