Make resizing in Files.app more responsive.

Make resizing in Files.app more responsive.

Before, the resizing of the file list and the grid list was delayed to avoid making the UI janky. That delay caused flickering of the screen when showing the Save-as dialog as well as when maximizing/restoring the window.
This patch solves this problem by calling immediately resizing the file list and grid view, but delaying any consecutive resizes.

TEST=Maximize, minimize, check the Save-as dialog.
BUG=245042
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=203910

[mtomasz, 2013-05-31 10:56:42]

@hirono: PTAL. Thanks.

[hirono, 2013-05-31 11:11:21]

Thank you!

https://codereview.chromium.org/15809008/diff/1/chrome/browser/resources/file...
File chrome/browser/resources/file_manager/js/file_grid.js (right):

https://codereview.chromium.org/15809008/diff/1/chrome/browser/resources/file...
chrome/browser/resources/file_manager/js/file_grid.js:77: resize();
After this resizing, the timer is set and the resize function is called one more
time.
Is it redundant?

https://codereview.chromium.org/15809008/diff/1/chrome/browser/resources/file...
File chrome/browser/resources/file_manager/js/file_table.js (right):

https://codereview.chromium.org/15809008/diff/1/chrome/browser/resources/file...
chrome/browser/resources/file_manager/js/file_table.js:682: resize();
ditto.

[mtomasz, 2013-06-03 01:01:38]

https://codereview.chromium.org/15809008/diff/1/chrome/browser/resources/file...
File chrome/browser/resources/file_manager/js/file_grid.js (right):

https://codereview.chromium.org/15809008/diff/1/chrome/browser/resources/file...
chrome/browser/resources/file_manager/js/file_grid.js:77: resize();
On 2013/05/31 11:11:21, hirono wrote:
> After this resizing, the timer is set and the resize function is called one
more
> time.
> Is it redundant?

You're right. Done.

https://codereview.chromium.org/15809008/diff/1/chrome/browser/resources/file...
File chrome/browser/resources/file_manager/js/file_table.js (right):

https://codereview.chromium.org/15809008/diff/1/chrome/browser/resources/file...
chrome/browser/resources/file_manager/js/file_table.js:682: resize();
On 2013/05/31 11:11:21, hirono wrote:
> ditto.

Done.

[hirono, 2013-06-03 01:59:08]

https://codereview.chromium.org/15809008/diff/2002/chrome/browser/resources/f...
File chrome/browser/resources/file_manager/js/file_grid.js (right):

https://codereview.chromium.org/15809008/diff/2002/chrome/browser/resources/f...
chrome/browser/resources/file_manager/js/file_grid.js:76: this.redraw();
This second redraw is needed?

https://codereview.chromium.org/15809008/diff/2002/chrome/browser/resources/f...
chrome/browser/resources/file_manager/js/file_grid.js:87:
this.suppressResizeTimer_ = setTimeout(function() {
How about introducing this.lastResizedTime?
This can remove not only redundant relayout but also redundant setTimeout.

if (now - this.lastResizedTime < SUPPRESS_TIME) {
  if (this.suppressResizeTimer_) // clear timer...
  setTimeout(this.relayout.bind(this), SUPPRESS_TIME - 1);
  return;
}

// do relayout ...
this.lastResizedTime = now;

[mtomasz, 2013-06-03 02:11:02]

https://codereview.chromium.org/15809008/diff/2002/chrome/browser/resources/f...
File chrome/browser/resources/file_manager/js/file_grid.js (right):

https://codereview.chromium.org/15809008/diff/2002/chrome/browser/resources/f...
chrome/browser/resources/file_manager/js/file_grid.js:76: this.redraw();
On 2013/06/03 01:59:08, hirono wrote:
> This second redraw is needed?

Yep, copy paste fault.

https://codereview.chromium.org/15809008/diff/2002/chrome/browser/resources/f...
chrome/browser/resources/file_manager/js/file_grid.js:87:
this.suppressResizeTimer_ = setTimeout(function() {
On 2013/06/03 01:59:08, hirono wrote:
> How about introducing this.lastResizedTime?
> This can remove not only redundant relayout but also redundant setTimeout.
> 
> if (now - this.lastResizedTime < SUPPRESS_TIME) {
>   if (this.suppressResizeTimer_) // clear timer...
>   setTimeout(this.relayout.bind(this), SUPPRESS_TIME - 1);
>   return;
> }
> 
> // do relayout ...
> this.lastResizedTime = now;

Can you elaborate? Why it would be better? In the current solution we don't have
any redundant relayout I think.

What advantages has your code over the current implementation?

[hirono, 2013-06-03 03:12:20]

https://codereview.chromium.org/15809008/diff/2002/chrome/browser/resources/f...
File chrome/browser/resources/file_manager/js/file_grid.js (right):

https://codereview.chromium.org/15809008/diff/2002/chrome/browser/resources/f...
chrome/browser/resources/file_manager/js/file_grid.js:87:
this.suppressResizeTimer_ = setTimeout(function() {
On 2013/06/03 02:11:02, mtomasz wrote:
> On 2013/06/03 01:59:08, hirono wrote:
> > How about introducing this.lastResizedTime?
> > This can remove not only redundant relayout but also redundant setTimeout.
> > 
> > if (now - this.lastResizedTime < SUPPRESS_TIME) {
> >   if (this.suppressResizeTimer_) // clear timer...
> >   setTimeout(this.relayout.bind(this), SUPPRESS_TIME - 1);
> >   return;
> > }
> > 
> > // do relayout ...
> > this.lastResizedTime = now;
> 
> Can you elaborate? Why it would be better? In the current solution we don't
have
> any redundant relayout I think.
> 
> What advantages has your code over the current implementation?

There are 2 reasons.

If window is done relayout by maximize button, relayout function is called only
once.
In the case, relayout is done as following.

1. relayout is done
2. setTimeout
3. the next relayout is ignored (because this.resizedScheduled_ is false)

But setTimeout of step 2 is not necessary and I'd like to reduce it.

The second reason a bit weak and may be a part of my preference.
I feel the alternative code is more readable because readers of code does not
have to follow the condition of 'resizeScheduled_' flag.

[mtomasz, 2013-06-03 07:11:04]

https://codereview.chromium.org/15809008/diff/2002/chrome/browser/resources/f...
File chrome/browser/resources/file_manager/js/file_grid.js (right):

https://codereview.chromium.org/15809008/diff/2002/chrome/browser/resources/f...
chrome/browser/resources/file_manager/js/file_grid.js:87:
this.suppressResizeTimer_ = setTimeout(function() {
On 2013/06/03 03:12:20, hirono wrote:
> On 2013/06/03 02:11:02, mtomasz wrote:
> > On 2013/06/03 01:59:08, hirono wrote:
> > > How about introducing this.lastResizedTime?
> > > This can remove not only redundant relayout but also redundant setTimeout.
> > > 
> > > if (now - this.lastResizedTime < SUPPRESS_TIME) {
> > >   if (this.suppressResizeTimer_) // clear timer...
> > >   setTimeout(this.relayout.bind(this), SUPPRESS_TIME - 1);
> > >   return;
> > > }
> > > 
> > > // do relayout ...
> > > this.lastResizedTime = now;
> > 
> > Can you elaborate? Why it would be better? In the current solution we don't
> have
> > any redundant relayout I think.
> > 
> > What advantages has your code over the current implementation?
> 
> There are 2 reasons.
> 
> If window is done relayout by maximize button, relayout function is called
only
> once.
> In the case, relayout is done as following.
> 
> 1. relayout is done
> 2. setTimeout
> 3. the next relayout is ignored (because this.resizedScheduled_ is false)
> 
> But setTimeout of step 2 is not necessary and I'd like to reduce it.
> 
> The second reason a bit weak and may be a part of my preference.
> I feel the alternative code is more readable because readers of code does not
> have to follow the condition of 'resizeScheduled_' flag.

I see, this makes sense. However, the code doesn't look much cleaner in my
opinion. Please compare grid_table.js with table_list.js and let me know which
do you prefer.

[mtomasz, 2013-06-03 07:12:11]

https://codereview.chromium.org/15809008/diff/17001/chrome/browser/resources/...
File chrome/browser/resources/file_manager/js/file_table.js (right):

https://codereview.chromium.org/15809008/diff/17001/chrome/browser/resources/...
chrome/browser/resources/file_manager/js/file_table.js:702:
this.lastResizedTimer_ = Date.now();
This should be lastResizedTime.

[hirono, 2013-06-03 07:46:00]

Thank you very much for addressing this.
After comparing grid_table.js with table_list.js,
I'm no longer sure about cleanness of codes.
But I think it is better to save the calling setTimeout.

https://codereview.chromium.org/15809008/diff/17001/chrome/browser/resources/...
File chrome/browser/resources/file_manager/js/file_table.js (right):

https://codereview.chromium.org/15809008/diff/17001/chrome/browser/resources/...
chrome/browser/resources/file_manager/js/file_table.js:690:
FileTable.RELAYOUT_DELAY - 1);
Sorry, I missed.
Not '- 1' but '+ 1' have been suitable to ensure to do relayout at the next
call.

[mtomasz, 2013-06-03 08:32:03]

https://codereview.chromium.org/15809008/diff/17001/chrome/browser/resources/...
File chrome/browser/resources/file_manager/js/file_table.js (right):

https://codereview.chromium.org/15809008/diff/17001/chrome/browser/resources/...
chrome/browser/resources/file_manager/js/file_table.js:690:
FileTable.RELAYOUT_DELAY - 1);
On 2013/06/03 07:46:00, hirono wrote:
> Sorry, I missed.
> Not '- 1' but '+ 1' have been suitable to ensure to do relayout at the next
> call.

Done.

https://codereview.chromium.org/15809008/diff/17001/chrome/browser/resources/...
chrome/browser/resources/file_manager/js/file_table.js:702:
this.lastResizedTimer_ = Date.now();
On 2013/06/03 07:12:11, mtomasz wrote:
> This should be lastResizedTime.

Done.

[hirono, 2013-06-03 09:46:33]

On 2013/06/03 08:32:03, mtomasz wrote:
>
https://codereview.chromium.org/15809008/diff/17001/chrome/browser/resources/...
> File chrome/browser/resources/file_manager/js/file_table.js (right):
> 
>
https://codereview.chromium.org/15809008/diff/17001/chrome/browser/resources/...
> chrome/browser/resources/file_manager/js/file_table.js:690:
> FileTable.RELAYOUT_DELAY - 1);
> On 2013/06/03 07:46:00, hirono wrote:
> > Sorry, I missed.
> > Not '- 1' but '+ 1' have been suitable to ensure to do relayout at the next
> > call.
> 
> Done.
> 
>
https://codereview.chromium.org/15809008/diff/17001/chrome/browser/resources/...
> chrome/browser/resources/file_manager/js/file_table.js:702:
> this.lastResizedTimer_ = Date.now();
> On 2013/06/03 07:12:11, mtomasz wrote:
> > This should be lastResizedTime.
> 
> Done.

AsyncUtil.Aggregation is so cool!
Thank you very much and lgtm!

[hirono, 2013-06-03 09:47:55]

lgtm with nit.

https://codereview.chromium.org/15809008/diff/25001/chrome/browser/resources/...
File chrome/browser/resources/file_manager/js/async_util.js (right):

https://codereview.chromium.org/15809008/diff/25001/chrome/browser/resources/...
chrome/browser/resources/file_manager/js/async_util.js:184: if
(this.lastRunTime_ && Date.now() - this.lastRunTime_ < this.delay_) {
If the constructor initializes this.lastRunTime_ as 0, "this.lastRunTime_ && "
can be removed.

[mtomasz, 2013-06-04 00:47:59]

https://codereview.chromium.org/15809008/diff/25001/chrome/browser/resources/...
File chrome/browser/resources/file_manager/js/async_util.js (right):

https://codereview.chromium.org/15809008/diff/25001/chrome/browser/resources/...
chrome/browser/resources/file_manager/js/async_util.js:184: if
(this.lastRunTime_ && Date.now() - this.lastRunTime_ < this.delay_) {
On 2013/06/03 09:47:55, hirono wrote:
> If the constructor initializes this.lastRunTime_ as 0, "this.lastRunTime_ && "
> can be removed.

Done.

[commit-bot: I haz the power, 2013-06-04 00:48:55]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/mtomasz@chromium.org/15809008/32001

[commit-bot: I haz the power, 2013-06-04 08:39:15]

Change committed as 203910

[Isaac (away), 2013-06-10 01:12:28]

I believe this CL has caused browser_tests
OpenSpecialTypes/FileManagerBrowserSimpleTest test suite to become flay on
chromiumos.

This is the first run with this CL and the first failures (which were silenced
because they were retried): http://bit.ly/12dxazv

[Isaac (away), 2013-06-10 01:29:54]

Filed: crbug.com/247997