cr.ui.Grid: Consider padding to determine the number of columns in a row.

cr.ui.Grid: Consider padding to determine the number of columns in a row.

padding-left and padding-right should be subtracted from clientWidth to measure
the available width for grid items.

BUG=453764
TEST=adjust window width of Files.app and check that the problem doesn't occur.

Committed: https://crrev.com/63c50e8e43e8167dc1b570e2905827f6890c6408
Cr-Commit-Position: refs/heads/master@{#317169}

[fukino, 2015-02-12 06:43:38]

fukino@chromium.org changed reviewers:
+ dbeam@chromium.org

[fukino, 2015-02-12 06:43:38]

dbeam@, could you take a look? Thanks!

[Dan Beam, 2015-02-12 19:41:43]

https://codereview.chromium.org/916353002/diff/1/ui/webui/resources/js/cr/ui/...
File ui/webui/resources/js/cr/ui/grid.js (right):

https://codereview.chromium.org/916353002/diff/1/ui/webui/resources/js/cr/ui/...
ui/webui/resources/js/cr/ui/grid.js:109: var horizontailPadding =
horizontalPadding

https://codereview.chromium.org/916353002/diff/1/ui/webui/resources/js/cr/ui/...
ui/webui/resources/js/cr/ui/grid.js:110:
parseFloat(window.getComputedStyle(this).paddingLeft) +
only call getComputedStyle() once

https://codereview.chromium.org/916353002/diff/1/ui/webui/resources/js/cr/ui/...
ui/webui/resources/js/cr/ui/grid.js:123: (this.clientWidthWithScrollbar_ -
horizontailPadding) / width);
none of this code makes sense... before or after your change.

  var columns = X;
  if (!X)  // aka X == 0
    return 0;  // which could be the same a return X;

  if (...)
    return X;

  return X;

could all just be simplified to:

  return X;

[fukino, 2015-02-13 02:44:52]

https://codereview.chromium.org/916353002/diff/1/ui/webui/resources/js/cr/ui/...
File ui/webui/resources/js/cr/ui/grid.js (right):

https://codereview.chromium.org/916353002/diff/1/ui/webui/resources/js/cr/ui/...
ui/webui/resources/js/cr/ui/grid.js:109: var horizontailPadding =
On 2015/02/12 19:41:43, Dan Beam wrote:
> horizontalPadding

Oops, thanks!
Done.

https://codereview.chromium.org/916353002/diff/1/ui/webui/resources/js/cr/ui/...
ui/webui/resources/js/cr/ui/grid.js:110:
parseFloat(window.getComputedStyle(this).paddingLeft) +
On 2015/02/12 19:41:43, Dan Beam wrote:
> only call getComputedStyle() once

Done.

https://codereview.chromium.org/916353002/diff/1/ui/webui/resources/js/cr/ui/...
ui/webui/resources/js/cr/ui/grid.js:123: (this.clientWidthWithScrollbar_ -
horizontailPadding) / width);
On 2015/02/12 19:41:43, Dan Beam wrote:
> none of this code makes sense... before or after your change.
> 
>   var columns = X;
>   if (!X)  // aka X == 0
>     return 0;  // which could be the same a return X;
> 
>   if (...)
>     return X;
> 
>   return X;
> 
> could all just be simplified to:
> 
>   return X;

I thought the code was valid, because
- the "return 0" in line#116 is to avoid zero-division.
- the last "return Math.floor(....)" is not same as return columns, because in
the last line, clientWidth*With*Scrollbar is used instead of
clientWidth*Without*Scrollbar.

The code looks complicated, but I couldn't find a good simplification...

[Dan Beam, 2015-02-13 03:31:41]

lgtm

https://codereview.chromium.org/916353002/diff/1/ui/webui/resources/js/cr/ui/...
File ui/webui/resources/js/cr/ui/grid.js (right):

https://codereview.chromium.org/916353002/diff/1/ui/webui/resources/js/cr/ui/...
ui/webui/resources/js/cr/ui/grid.js:123: (this.clientWidthWithScrollbar_ -
horizontailPadding) / width);
On 2015/02/13 02:44:52, fukino wrote:
> On 2015/02/12 19:41:43, Dan Beam wrote:
> > none of this code makes sense... before or after your change.
> > 
> >   var columns = X;
> >   if (!X)  // aka X == 0
> >     return 0;  // which could be the same a return X;
> > 
> >   if (...)
> >     return X;
> > 
> >   return X;
> > 
> > could all just be simplified to:
> > 
> >   return X;
> 
> I thought the code was valid, because
> - the "return 0" in line#116 is to avoid zero-division.
> - the last "return Math.floor(....)" is not same as return columns, because in
> the last line, clientWidth*With*Scrollbar is used instead of
> clientWidth*Without*Scrollbar.
> 
> The code looks complicated, but I couldn't find a good simplification...

ah, With vs Without, OK

https://codereview.chromium.org/916353002/diff/20001/ui/webui/resources/js/cr...
File ui/webui/resources/js/cr/ui/grid.js (right):

https://codereview.chromium.org/916353002/diff/20001/ui/webui/resources/js/cr...
ui/webui/resources/js/cr/ui/grid.js:120: if (rows * height <=
this.clientHeight_)
nit: maybe put a comment in here along the lines of:

  // Content fits within the client area (no scrollbar required).

or anything else to show the subtle difference between these codepaths I got
confused over

[fukino, 2015-02-13 04:08:46]

New patchsets have been uploaded after l-g-t-m from dbeam@chromium.org

[fukino, 2015-02-13 04:09:17]

Thank you!

https://codereview.chromium.org/916353002/diff/20001/ui/webui/resources/js/cr...
File ui/webui/resources/js/cr/ui/grid.js (right):

https://codereview.chromium.org/916353002/diff/20001/ui/webui/resources/js/cr...
ui/webui/resources/js/cr/ui/grid.js:120: if (rows * height <=
this.clientHeight_)
On 2015/02/13 03:31:41, Dan Beam wrote:
> nit: maybe put a comment in here along the lines of:
> 
>   // Content fits within the client area (no scrollbar required).
> 
> or anything else to show the subtle difference between these codepaths I got
> confused over

Done.

[fukino, 2015-02-13 05:15:09]

The CQ bit was checked by fukino@chromium.org

[commit-bot: I haz the power, 2015-02-13 05:16:01]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/916353002/40001

[commit-bot: I haz the power, 2015-02-13 06:30:37]

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

[commit-bot: I haz the power, 2015-02-13 06:30:38]

Try jobs failed on following builders:
  linux_chromium_rel_ng on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[fukino, 2015-02-16 14:41:26]

Hi dbeam@: Could you take another look?
I moved the padding-measuring code from getColumnCount_ to updateMetrics_ so
that getColumnCount_ can be tested without DOM element and reduce the number of
invocation for getComputedStyle.

[fukino, 2015-02-18 04:15:39]

ping just in case.

[Dan Beam, 2015-02-19 21:51:01]

lgtm, thanks for the optimization

[fukino, 2015-02-19 23:25:40]

On 2015/02/19 21:51:01, Dan Beam wrote:
> lgtm, thanks for the optimization

Thanks!

[fukino, 2015-02-19 23:25:45]

The CQ bit was checked by fukino@chromium.org

[commit-bot: I haz the power, 2015-02-19 23:27:24]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/916353002/60001

[commit-bot: I haz the power, 2015-02-19 23:38:47]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2015-02-19 23:39:28]

Patchset 4 (id:??) landed as
https://crrev.com/63c50e8e43e8167dc1b570e2905827f6890c6408
Cr-Commit-Position: refs/heads/master@{#317169}