Moving file selection checkbox to the field name.

Moving file selection checkbox to the field name.

BUG=chromium-os:20156, chromium-os:22832
TEST=None


Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=112533

[SeRya, 2011-11-28 17:03:23]

Vlad, please review the CL.

Josh agreed to move the checkbox instead fine tuning the column.

[Vladislav Kaznacheev, 2011-11-29 19:11:59]

http://codereview.chromium.org/8720006/diff/1/chrome/browser/resources/file_m...
File chrome/browser/resources/file_manager/css/file_manager.css (right):

http://codereview.chromium.org/8720006/diff/1/chrome/browser/resources/file_m...
chrome/browser/resources/file_manager/css/file_manager.css:382: top: 1px;
Is it any better than having margin-top: 1px?

http://codereview.chromium.org/8720006/diff/1/chrome/browser/resources/file_m...
File chrome/browser/resources/file_manager/main.html (right):

http://codereview.chromium.org/8720006/diff/1/chrome/browser/resources/file_m...
chrome/browser/resources/file_manager/main.html:33:
document.getElementsByTagName('base')[0].setAttribute('href',
document.querySelector('base') is cleaner

http://codereview.chromium.org/8720006/diff/1/chrome/browser/resources/shared...
File chrome/browser/resources/shared/css/table.css (right):

http://codereview.chromium.org/8720006/diff/1/chrome/browser/resources/shared...
chrome/browser/resources/shared/css/table.css:53: -webkit-box-align: center;
This file is used by several other Chrome components. Please make sure they are
not broken by this patch.

[SeRya, 2011-11-30 13:04:04]

http://codereview.chromium.org/8720006/diff/1/chrome/browser/resources/file_m...
File chrome/browser/resources/file_manager/css/file_manager.css (right):

http://codereview.chromium.org/8720006/diff/1/chrome/browser/resources/file_m...
chrome/browser/resources/file_manager/css/file_manager.css:382: top: 1px;
On 2011/11/29 19:11:59, Vladislav Kaznacheev wrote:
> Is it any better than having margin-top: 1px?

It would also affect the text label. top in relative positioning doesn't.

http://codereview.chromium.org/8720006/diff/1/chrome/browser/resources/file_m...
File chrome/browser/resources/file_manager/main.html (right):

http://codereview.chromium.org/8720006/diff/1/chrome/browser/resources/file_m...
chrome/browser/resources/file_manager/main.html:33:
document.getElementsByTagName('base')[0].setAttribute('href',
On 2011/11/29 19:11:59, Vladislav Kaznacheev wrote:
> document.querySelector('base') is cleaner

Fixed. Thanks.

http://codereview.chromium.org/8720006/diff/1/chrome/browser/resources/shared...
File chrome/browser/resources/shared/css/table.css (right):

http://codereview.chromium.org/8720006/diff/1/chrome/browser/resources/shared...
chrome/browser/resources/shared/css/table.css:53: -webkit-box-align: center;
On 2011/11/29 19:11:59, Vladislav Kaznacheev wrote:
> This file is used by several other Chrome components. Please make sure they
are
> not broken by this patch.

It's also used the Task Manager. Checked and fixed.

[Vladislav Kaznacheev, 2011-11-30 13:25:42]

LGTM with 1 comment

http://codereview.chromium.org/8720006/diff/1/chrome/browser/resources/file_m...
File chrome/browser/resources/file_manager/css/file_manager.css (right):

http://codereview.chromium.org/8720006/diff/1/chrome/browser/resources/file_m...
chrome/browser/resources/file_manager/css/file_manager.css:382: top: 1px;
It would be nice to add a comment that explains it here.
On 2011/11/30 13:04:04, SeRya wrote:
> On 2011/11/29 19:11:59, Vladislav Kaznacheev wrote:
> > Is it any better than having margin-top: 1px?
> 
> It would also affect the text label. top in relative positioning doesn't.

[SeRya, 2011-11-30 13:47:13]

I need owners' review of my changes in the table.css, please.

http://codereview.chromium.org/8720006/diff/1/chrome/browser/resources/file_m...
File chrome/browser/resources/file_manager/css/file_manager.css (right):

http://codereview.chromium.org/8720006/diff/1/chrome/browser/resources/file_m...
chrome/browser/resources/file_manager/css/file_manager.css:382: top: 1px;
On 2011/11/30 13:25:42, Vladislav Kaznacheev wrote:
> It would be nice to add a comment that explains it here.
> On 2011/11/30 13:04:04, SeRya wrote:
> > On 2011/11/29 19:11:59, Vladislav Kaznacheev wrote:
> > > Is it any better than having margin-top: 1px?
> > 
> > It would also affect the text label. top in relative positioning doesn't.
> 

Done.

[James Hawkins, 2011-11-30 16:10:52]

Have you investigated all the pages that use table.css to verify they're not
hosed visually by this change?  It seems invasive.

http://codereview.chromium.org/8720006/diff/10003/chrome/browser/resources/fi...
File chrome/browser/resources/file_manager/css/file_manager.css (right):

http://codereview.chromium.org/8720006/diff/10003/chrome/browser/resources/fi...
chrome/browser/resources/file_manager/css/file_manager.css:408: margin: 0px 6px
0 0;
s/0px/0/

http://codereview.chromium.org/8720006/diff/10003/chrome/browser/resources/fi...
chrome/browser/resources/file_manager/css/file_manager.css:417: margin-left:
7px;
RTL

http://codereview.chromium.org/8720006/diff/10003/chrome/browser/resources/fi...
chrome/browser/resources/file_manager/css/file_manager.css:587: display:
-webkit-box;
Is this necessary?

http://codereview.chromium.org/8720006/diff/10003/chrome/browser/resources/sh...
File chrome/browser/resources/shared/css/table.css (right):

http://codereview.chromium.org/8720006/diff/10003/chrome/browser/resources/sh...
chrome/browser/resources/shared/css/table.css:53: -webkit-box-align: center;
-webkit properties go at the top.

[SeRya, 2011-12-01 12:57:42]

> Have you investigated all the pages that use table.css to
> verify they're not
> hosed visually by this change?  It seems invasive.

Yes, I have. It's only used in file manager and task manager.

http://www.google.com/codesearch#search/&exact_package=chromium&q=table.css&t...

http://codereview.chromium.org/8720006/diff/10003/chrome/browser/resources/fi...
File chrome/browser/resources/file_manager/css/file_manager.css (right):

http://codereview.chromium.org/8720006/diff/10003/chrome/browser/resources/fi...
chrome/browser/resources/file_manager/css/file_manager.css:408: margin: 0px 6px
0 0;
On 2011/11/30 16:10:52, James Hawkins wrote:
> s/0px/0/

Done.

http://codereview.chromium.org/8720006/diff/10003/chrome/browser/resources/fi...
chrome/browser/resources/file_manager/css/file_manager.css:417: margin-left:
7px;
On 2011/11/30 16:10:52, James Hawkins wrote:
> RTL

Done.

http://codereview.chromium.org/8720006/diff/10003/chrome/browser/resources/fi...
chrome/browser/resources/file_manager/css/file_manager.css:587: display:
-webkit-box;
On 2011/11/30 16:10:52, James Hawkins wrote:
> Is this necessary?

It is not. removed.

http://codereview.chromium.org/8720006/diff/10003/chrome/browser/resources/sh...
File chrome/browser/resources/shared/css/table.css (right):

http://codereview.chromium.org/8720006/diff/10003/chrome/browser/resources/sh...
chrome/browser/resources/shared/css/table.css:53: -webkit-box-align: center;
On 2011/11/30 16:10:52, James Hawkins wrote:
> -webkit properties go at the top.

Done.

[yoshiki, 2011-12-01 13:52:12]

I reviewed the code related with task_manager.

http://codereview.chromium.org/8720006/diff/21001/chrome/browser/resources/ta...
File chrome/browser/resources/task_manager/task_manager.css (left):

http://codereview.chromium.org/8720006/diff/21001/chrome/browser/resources/ta...
chrome/browser/resources/task_manager/task_manager.css:114: -webkit-box-orient:
vertical;
Is this necessary? Please don't remove this line. This line makes multiple tasks
in a process aligned vertically on WebUI TaskManager.

[SeRya, 2011-12-01 16:44:32]

Yoshiki, thank you for the point. I realized that I did wrongly understand what
the line "-webkit-box-align: center;" does and removed it. Changes in the
task_manager.css are not needed anymore and reverted.

[James Hawkins, 2011-12-01 17:55:53]

LGTM with nit.

http://codereview.chromium.org/8720006/diff/23002/chrome/browser/resources/sh...
File chrome/browser/resources/shared/css/table.css (right):

http://codereview.chromium.org/8720006/diff/23002/chrome/browser/resources/sh...
chrome/browser/resources/shared/css/table.css:45: display: -webkit-box;
nit: Alphabetize properties.

[SeRya, 2011-12-01 19:35:47]

http://codereview.chromium.org/8720006/diff/23002/chrome/browser/resources/sh...
File chrome/browser/resources/shared/css/table.css (right):

http://codereview.chromium.org/8720006/diff/23002/chrome/browser/resources/sh...
chrome/browser/resources/shared/css/table.css:45: display: -webkit-box;
On 2011/12/01 17:55:53, James Hawkins wrote:
> nit: Alphabetize properties.

Done.

[commit-bot: I haz the power, 2011-12-01 19:36:06]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/serya@chromium.org/8720006/23003

[commit-bot: I haz the power, 2011-12-01 21:09:29]

Change committed as 112533