<html>
<head>
<base href="https://bugs.webkit.org/">
</head>
<body>
<p>
<div>
<b><a class="bz_bug_link
bz_status_NEW "
title="NEW - MotionMark tests using Date.now() to drive animations don't work under web-page-replay"
href="https://bugs.webkit.org/show_bug.cgi?id=170198#c5">Comment # 5</a>
on <a class="bz_bug_link
bz_status_NEW "
title="NEW - MotionMark tests using Date.now() to drive animations don't work under web-page-replay"
href="https://bugs.webkit.org/show_bug.cgi?id=170198">bug 170198</a>
from <span class="vcard"><a class="email" href="mailto:rniwa@webkit.org" title="Ryosuke Niwa <rniwa@webkit.org>"> <span class="fn">Ryosuke Niwa</span></a>
</span></b>
<pre>Comment on <span class=""><a href="attachment.cgi?id=305682&action=diff" name="attach_305682" title="Patch">attachment 305682</a> <a href="attachment.cgi?id=305682&action=edit" title="Patch">[details]</a></span>
Patch
View in context: <a href="https://bugs.webkit.org/attachment.cgi?id=305682&action=review">https://bugs.webkit.org/attachment.cgi?id=305682&action=review</a>
Useful inline comments would explain why you're making each code change, and how it impacts the rest of the codebase.
<span class="quote">> PerformanceTests/MotionMark/tests/3d/resources/webgl.js:143
> - this._startTime = Stage.dateCounterValue(1000);
> - var elapsedTime = Stage.dateCounterValue(1000) - this._startTime;
> + this._startTime = this.timestampCounterValue(1000);
> + var elapsedTime = this.timestampCounterValue(1000) - this._startTime;</span >
Why are we replacing Stage.dateCounterValue with this.timestampCounterValue?
What's the significance of it? e.g. does Stage.dateCounterValue use Date.now() and this.timestampCounterValue doesn't?
None of that is clear from your main change log description.
<span class="quote">> PerformanceTests/MotionMark/tests/master/resources/canvas-tests.js:19
> + this._stage = stage;</span >
A useful inline comment here would be explaining why we need to store "stage" as an instance variable.
<span class="quote">> PerformanceTests/MotionMark/tests/resources/main.js:654
> + set timestamp(stamp)
> + {
> + this._currentTimestamp = stamp;
> + },
> +
> + get timestamp()
> + {
> + return this._currentTimestamp;
> + },
> +</span >
What are these getters and setters used for? Who is getting the value?
<span class="quote">> PerformanceTests/MotionMark/tests/resources/main.js:676
> + rotatingColor: function(cycleLengthMs, saturation, lightness)
> + {</span >
Why are you moving these methods here?
That should be explained in the change log.</pre>
</div>
</p>
<hr>
<span>You are receiving this mail because:</span>
<ul>
<li>You are the assignee for the bug.</li>
</ul>
</body>
</html>