Files.app: Change timing to update progress item properties.

Files.app: Change timing to update progress item properties.

This CL adds AnimationFrameCallback class to update the properties of progress
center items at the correct timing, and ensure it invokes the CSS animation.

Previously, we use setTimeout function to ensure changes of progress items
properties are applied to one by one, but if the number of milliseconds is less
than the frame inteval, it does not work.

BUG=348493
TEST=Make a repro function and call it multiple times.

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=255349

[hirono, 2014-03-04 06:07:05]

PTAL the CL? Thanks!

[mtomasz, 2014-03-04 06:17:33]

https://codereview.chromium.org/185193003/diff/1/chrome/browser/resources/fil...
File
chrome/browser/resources/file_manager/foreground/js/ui/progress_center_panel.js
(right):

https://codereview.chromium.org/185193003/diff/1/chrome/browser/resources/fil...
chrome/browser/resources/file_manager/foreground/js/ui/progress_center_panel.js:13:
* Wehther the current frame change is applied (rendered) or not.
nit: Whether

https://codereview.chromium.org/185193003/diff/1/chrome/browser/resources/fil...
chrome/browser/resources/file_manager/foreground/js/ui/progress_center_panel.js:198:
this.animationFrameCallback_.requestNow(setWidth);
Could you describe the problem in details? Can't we fix it easier? Is the
AnimationFrameCallback class really necessary?

requestAnimationFrame is almost the same as setTimeout(..., 1000 / 60). Why is
it used instead of the timer?

[hirono, 2014-03-05 04:19:51]

Thanks!

https://codereview.chromium.org/185193003/diff/1/chrome/browser/resources/fil...
File
chrome/browser/resources/file_manager/foreground/js/ui/progress_center_panel.js
(right):

https://codereview.chromium.org/185193003/diff/1/chrome/browser/resources/fil...
chrome/browser/resources/file_manager/foreground/js/ui/progress_center_panel.js:13:
* Wehther the current frame change is applied (rendered) or not.
On 2014/03/04 06:17:33, mtomasz wrote:
> nit: Whether

Done.

https://codereview.chromium.org/185193003/diff/1/chrome/browser/resources/fil...
chrome/browser/resources/file_manager/foreground/js/ui/progress_center_panel.js:198:
this.animationFrameCallback_.requestNow(setWidth);
On 2014/03/04 06:17:33, mtomasz wrote:
> Could you describe the problem in details? Can't we fix it easier? Is the
> AnimationFrameCallback class really necessary?
> 
> requestAnimationFrame is almost the same as setTimeout(..., 1000 / 60). Why is
> it used instead of the timer?

The problem is we must ensure the start properties of the transition animation
are rendered before the end properties are set. Otherwise, the end properties
are just applied without transition.

The fix of setTimeout(..., 1000/60) probably works, but it still does not ensure
that the start properties are rendered before the callback function is called.
It can become a problem for another FPS, or in 
an overloaded environment. On the other hand, the callback of
webkitRequestAnimationFrame is invoked just before rendering. So the properties
set in the callback are surely rendered.

[mtomasz, 2014-03-06 01:56:14]

https://codereview.chromium.org/185193003/diff/1/chrome/browser/resources/fil...
File
chrome/browser/resources/file_manager/foreground/js/ui/progress_center_panel.js
(right):

https://codereview.chromium.org/185193003/diff/1/chrome/browser/resources/fil...
chrome/browser/resources/file_manager/foreground/js/ui/progress_center_panel.js:198:
this.animationFrameCallback_.requestNow(setWidth);
On 2014/03/05 04:19:51, hirono wrote:
> On 2014/03/04 06:17:33, mtomasz wrote:
> > Could you describe the problem in details? Can't we fix it easier? Is the
> > AnimationFrameCallback class really necessary?
> > 
> > requestAnimationFrame is almost the same as setTimeout(..., 1000 / 60). Why
is
> > it used instead of the timer?
> 
> The problem is we must ensure the start properties of the transition animation
> are rendered before the end properties are set. Otherwise, the end properties
> are just applied without transition.
> 
> The fix of setTimeout(..., 1000/60) probably works, but it still does not
ensure
> that the start properties are rendered before the callback function is called.
> It can become a problem for another FPS, or in 
> an overloaded environment. On the other hand, the callback of
> webkitRequestAnimationFrame is invoked just before rendering. So the
properties
> set in the callback are surely rendered.

Got it. So basically, this trick is used to ensure that the webkitTransitionEnd
event is called, right?

In Camera app, I have a simple waitForTransitionCompletion() utility function,
which accepts an additional 'timeout' argument, to solve similar problem.
(https://chromium.googlesource.com/apps/camera/+/master/src/js/util.js #212).

The disadvantage is that we have to provide some timeout value, which needs to
be kept in sync with CSS.

The approach with requestAnimationFrame is nice, but I think we could simplify
it, and make it easier to understand (and shorter). How about having just:

/**
 * Calls the animation setting closure, with a guarantee, that the
 * webkitTransitionEnd event is called.
 * @param {function()} callback Closure setting the animation.
 * @return {function()} Cancellation closure.
 */
util.safelySetAnimation = function(callback) {
  var requestId = requestAnimationFrame(function() {
    requestId = requestAnimationFrame(callback);
  });
  return function() {
    cancelAnimationFrame(requestId);
  };
};

If you prefer your current solution, then how about better naming?

The current class AnimationFrameCallback, doesn't say that it is used to
guarantee that webkitTransitionEnd is called. It is hard to understand why
requestNow and requestNextFrame are called. Since the class will not be used in
other scenarios than to ensure webkitTransitionEnd is called, better naming
would be helpful. Eg. SafeAnimationRunner::setAnimationStart +
SafeAnimationRunner::setAnimationEnd.

WDYT?

[hirono, 2014-03-06 05:37:55]

> In Camera app, I have a simple waitForTransitionCompletion() utility function,
> which accepts an additional 'timeout' argument, to solve similar problem.
> (https://chromium.googlesource.com/apps/camera/+/master/src/js/util.js #212).

This is a possible solution. But I'd also like to ensure the progress bar
animates, though it is a minor UI issue.

> The approach with requestAnimationFrame is nice, but I think we could simplify
> it, and make it easier to understand (and shorter). How about having just:
> 
> /**
>  * Calls the animation setting closure, with a guarantee, that the
>  * webkitTransitionEnd event is called.
>  * @param {function()} callback Closure setting the animation.
>  * @return {function()} Cancellation closure.
>  */
> util.safelySetAnimation = function(callback) {
>   var requestId = requestAnimationFrame(function() {
>     requestId = requestAnimationFrame(callback);
>   });
>   return function() {
>     cancelAnimationFrame(requestId);
>   };
> };

SGTM. I'll do this.

[hirono, 2014-03-06 06:25:59]

On 2014/03/06 05:37:55, hirono wrote:
> > In Camera app, I have a simple waitForTransitionCompletion() utility
function,
> > which accepts an additional 'timeout' argument, to solve similar problem.
> > (https://chromium.googlesource.com/apps/camera/+/master/src/js/util.js
#212).
> 
> This is a possible solution. But I'd also like to ensure the progress bar
> animates, though it is a minor UI issue.
> 
> > The approach with requestAnimationFrame is nice, but I think we could
simplify
> > it, and make it easier to understand (and shorter). How about having just:
> > 
> > /**
> >  * Calls the animation setting closure, with a guarantee, that the
> >  * webkitTransitionEnd event is called.
> >  * @param {function()} callback Closure setting the animation.
> >  * @return {function()} Cancellation closure.
> >  */
> > util.safelySetAnimation = function(callback) {
> >   var requestId = requestAnimationFrame(function() {
> >     requestId = requestAnimationFrame(callback);
> >   });
> >   return function() {
> >     cancelAnimationFrame(requestId);
> >   };
> > };
> 
> SGTM. I'll do this.

Done, thanks!

[mtomasz, 2014-03-06 06:30:46]

https://codereview.chromium.org/185193003/diff/40002/chrome/browser/resources...
File
chrome/browser/resources/file_manager/foreground/js/ui/progress_center_panel.js
(right):

https://codereview.chromium.org/185193003/diff/40002/chrome/browser/resources...
chrome/browser/resources/file_manager/foreground/js/ui/progress_center_panel.js:53:
// And the transition end peroperties set by the callback is rendered at the
typo: properties

https://codereview.chromium.org/185193003/diff/40002/chrome/browser/resources...
chrome/browser/resources/file_manager/foreground/js/ui/progress_center_panel.js:128:
this.cancelTransition_ =
Shall it be:
this.cancelTransition_ = this.cancelTransition || safelySetAnimation()?

OR calling cancelTransition_(), before assigning a new value?

If #128 is called twice, then we loose the cancellation ability for the first
invocation. So if #135 is called after (within 30 ms), then we will cancel only
the second request, but the first one will finish after calling setWidth(). Is
it OK?

[hirono, 2014-03-06 06:37:28]

Thanks!

https://codereview.chromium.org/185193003/diff/40002/chrome/browser/resources...
File
chrome/browser/resources/file_manager/foreground/js/ui/progress_center_panel.js
(right):

https://codereview.chromium.org/185193003/diff/40002/chrome/browser/resources...
chrome/browser/resources/file_manager/foreground/js/ui/progress_center_panel.js:53:
// And the transition end peroperties set by the callback is rendered at the
On 2014/03/06 06:30:46, mtomasz wrote:
> typo: properties

Done.

https://codereview.chromium.org/185193003/diff/40002/chrome/browser/resources...
chrome/browser/resources/file_manager/foreground/js/ui/progress_center_panel.js:128:
this.cancelTransition_ =
On 2014/03/06 06:30:46, mtomasz wrote:
> Shall it be:
> this.cancelTransition_ = this.cancelTransition || safelySetAnimation()?
> 
> OR calling cancelTransition_(), before assigning a new value?
> 
> If #128 is called twice, then we loose the cancellation ability for the first
> invocation. So if #135 is called after (within 30 ms), then we will cancel
only
> the second request, but the first one will finish after calling setWidth(). Is
> it OK?

No, should be fixed!
I added the cancel call before setting new width.

[mtomasz, 2014-03-06 06:42:11]

Thanks, LGTM!

[hirono, 2014-03-06 06:44:13]

The CQ bit was checked by hirono@chromium.org

[commit-bot: I haz the power, 2014-03-06 06:44:43]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/hirono@chromium.org/185193003/80001

[commit-bot: I haz the power, 2014-03-06 07:46:29]

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

[commit-bot: I haz the power, 2014-03-06 07:46:30]

Try jobs failed on following builders: mac_chromium_compile_dbg

[hirono, 2014-03-06 08:00:11]

The CQ bit was checked by hirono@chromium.org

[commit-bot: I haz the power, 2014-03-06 08:01:10]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/hirono@chromium.org/185193003/80001

[commit-bot: I haz the power, 2014-03-06 08:11:56]

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

[commit-bot: I haz the power, 2014-03-06 08:11:57]

Try jobs failed on following builders: mac_chromium_compile_dbg

[hirono, 2014-03-06 12:15:34]

The CQ bit was checked by hirono@chromium.org

[commit-bot: I haz the power, 2014-03-06 12:15:42]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/hirono@chromium.org/185193003/80001

[commit-bot: I haz the power, 2014-03-06 14:32:05]

Change committed as 255349