Show location, search button, and change-view button on the toolbar.

Show location, search button, and change-view button on the toolbar.

This CL includes following changes.
- Use system default header, rather than customized Maximize/Minimize/Close buttons.
- Current location is displayed on the left side of toolbar.
- Only the selected file's information is displayed on the preview panel.
- Change-view button is moved from gear menu to the right side of toolbar.
- Search button is placed on toolbar as a button. It expands on click.

BUG=397222, 308445
TEST=run browser_tests, manually tested

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

[fukino, 2014-08-07 03:39:27]

hirono san, could you take a look?

I combined some changes on this one CL because they have dependency on UI
structure with each other...
Sorry for make it hard to review.

[hirono, 2014-08-07 03:59:23]

https://codereview.chromium.org/443333003/diff/1/ui/file_manager/file_manager...
File ui/file_manager/file_manager/foreground/css/file_manager.css (right):

https://codereview.chromium.org/443333003/diff/1/ui/file_manager/file_manager...
ui/file_manager/file_manager/foreground/css/file_manager.css:489:
background-image: -webkit-image-set(
nit: shorthand properties are preferred.
https://google-styleguide.googlecode.com/svn/trunk/htmlcssguide.xml#Shorthand...

https://codereview.chromium.org/443333003/diff/1/ui/file_manager/file_manager...
ui/file_manager/file_manager/foreground/css/file_manager.css:495:
background-image: -webkit-image-set(
ditto.

https://codereview.chromium.org/443333003/diff/1/ui/file_manager/file_manager...
ui/file_manager/file_manager/foreground/css/file_manager.css:501:
background-image: -webkit-image-set(
ditto.

https://codereview.chromium.org/443333003/diff/1/ui/file_manager/file_manager...
ui/file_manager/file_manager/foreground/css/file_manager.css:942:
background-color: white;
nit: shorthand property

https://codereview.chromium.org/443333003/diff/1/ui/file_manager/file_manager...
File ui/file_manager/file_manager/foreground/js/file_manager.js (right):

https://codereview.chromium.org/443333003/diff/1/ui/file_manager/file_manager...
ui/file_manager/file_manager/foreground/js/file_manager.js:782:
this.locationBreadcrumbs_ = new BreadcrumbsController(
Can we move locationBreadcrumbs_ to FileManagerUI class? We'd like to diet the
FileManager class.

https://codereview.chromium.org/443333003/diff/1/ui/file_manager/file_manager...
ui/file_manager/file_manager/foreground/js/file_manager.js:788:
this.locationVolumeIcon_ = dom.querySelector('#location-volume-icon');
ditto.

https://codereview.chromium.org/443333003/diff/1/ui/file_manager/file_manager...
ui/file_manager/file_manager/foreground/js/file_manager.js:1174:
this.toggleViewButton_.classList.add('grid');
I think it's better to add 'table' class explicitly.

https://codereview.chromium.org/443333003/diff/1/ui/file_manager/file_manager...
ui/file_manager/file_manager/foreground/js/file_manager.js:2231: * Update the
location information displayed on the toolbar.
nit: Updates

https://codereview.chromium.org/443333003/diff/1/ui/file_manager/file_manager...
ui/file_manager/file_manager/foreground/js/file_manager.js:2232: * @private
Please add comment of opt_entry.

https://codereview.chromium.org/443333003/diff/1/ui/file_manager/file_manager...
ui/file_manager/file_manager/foreground/js/file_manager.js:2240:
'volume-type-icon', location.rootType);
nit: Indent should be 4. Same for the followings.

https://codereview.chromium.org/443333003/diff/1/ui/file_manager/file_manager...
ui/file_manager/file_manager/foreground/js/file_manager.js:2933: * Invoked when
the search icon is clicked.
nit: Though old code does not follow the rule, we would start function comments
with verb + 's'. e.g. Handles ... events.

https://codereview.chromium.org/443333003/diff/1/ui/file_manager/file_manager...
ui/file_manager/file_manager/foreground/js/file_manager.js:2942: * Invoked when
the toggle-view button is clicked.
ditto.

[hirono, 2014-08-07 05:08:54]

https://codereview.chromium.org/443333003/diff/1/ui/file_manager/file_manager...
File ui/file_manager/file_manager/foreground/js/file_manager.js (right):

https://codereview.chromium.org/443333003/diff/1/ui/file_manager/file_manager...
ui/file_manager/file_manager/foreground/js/file_manager.js:851:
this.searchButton_ = this.dialogDom_.querySelector('#search-button');
Can we move it to FileManagerUI?

https://codereview.chromium.org/443333003/diff/1/ui/file_manager/file_manager...
ui/file_manager/file_manager/foreground/js/file_manager.js:855:
this.toggleViewButton_ = this.dialogDom_.querySelector('#view-button');
Can we move it to FileManagerUI?

https://codereview.chromium.org/443333003/diff/1/ui/file_manager/file_manager...
File ui/file_manager/file_manager/foreground/js/ui/search_box.js (right):

https://codereview.chromium.org/443333003/diff/1/ui/file_manager/file_manager...
ui/file_manager/file_manager/foreground/js/ui/search_box.js:43:
this.textMeasure_ = new TextMeasure(this.inputElement);
Can we remove the TextMeasure class?

[fukino, 2014-08-07 06:45:01]

Thank you for comments!
I have a question about "background" shorthand inline.

https://codereview.chromium.org/443333003/diff/1/ui/file_manager/file_manager...
File ui/file_manager/file_manager/foreground/css/file_manager.css (right):

https://codereview.chromium.org/443333003/diff/1/ui/file_manager/file_manager...
ui/file_manager/file_manager/foreground/css/file_manager.css:489:
background-image: -webkit-image-set(
On 2014/08/07 03:59:22, hirono wrote:
> nit: shorthand properties are preferred.
>
https://google-styleguide.googlecode.com/svn/trunk/htmlcssguide.xml#Shorthand...

I want to avoid overwriting the backgroun-color defined for topbotton-bar
button:hover in
https://code.google.com/p/chromium/codesearch#chromium/src/ui/webui/resources....
(Using shorthand for ".topbutton-bar .view-button.grid" overwrites it.)

If you know a way to avoid this, please let me know!

https://codereview.chromium.org/443333003/diff/1/ui/file_manager/file_manager...
ui/file_manager/file_manager/foreground/css/file_manager.css:942:
background-color: white;
On 2014/08/07 03:59:22, hirono wrote:
> nit: shorthand property

Done.

https://codereview.chromium.org/443333003/diff/1/ui/file_manager/file_manager...
File ui/file_manager/file_manager/foreground/js/file_manager.js (right):

https://codereview.chromium.org/443333003/diff/1/ui/file_manager/file_manager...
ui/file_manager/file_manager/foreground/js/file_manager.js:782:
this.locationBreadcrumbs_ = new BreadcrumbsController(
On 2014/08/07 03:59:22, hirono wrote:
> Can we move locationBreadcrumbs_ to FileManagerUI class? We'd like to diet the
> FileManager class.
> 

Done.

https://codereview.chromium.org/443333003/diff/1/ui/file_manager/file_manager...
ui/file_manager/file_manager/foreground/js/file_manager.js:788:
this.locationVolumeIcon_ = dom.querySelector('#location-volume-icon');
On 2014/08/07 03:59:23, hirono wrote:
> ditto.

Done.

https://codereview.chromium.org/443333003/diff/1/ui/file_manager/file_manager...
ui/file_manager/file_manager/foreground/js/file_manager.js:851:
this.searchButton_ = this.dialogDom_.querySelector('#search-button');
On 2014/08/07 05:08:54, hirono wrote:
> Can we move it to FileManagerUI?

Done.

https://codereview.chromium.org/443333003/diff/1/ui/file_manager/file_manager...
ui/file_manager/file_manager/foreground/js/file_manager.js:855:
this.toggleViewButton_ = this.dialogDom_.querySelector('#view-button');
On 2014/08/07 05:08:54, hirono wrote:
> Can we move it to FileManagerUI?

Done.

https://codereview.chromium.org/443333003/diff/1/ui/file_manager/file_manager...
ui/file_manager/file_manager/foreground/js/file_manager.js:1174:
this.toggleViewButton_.classList.add('grid');
On 2014/08/07 03:59:22, hirono wrote:
> I think it's better to add 'table' class explicitly.

Done.

https://codereview.chromium.org/443333003/diff/1/ui/file_manager/file_manager...
ui/file_manager/file_manager/foreground/js/file_manager.js:2231: * Update the
location information displayed on the toolbar.
On 2014/08/07 03:59:22, hirono wrote:
> nit: Updates

Done.

https://codereview.chromium.org/443333003/diff/1/ui/file_manager/file_manager...
ui/file_manager/file_manager/foreground/js/file_manager.js:2232: * @private
On 2014/08/07 03:59:22, hirono wrote:
> Please add comment of opt_entry.

Done.

https://codereview.chromium.org/443333003/diff/1/ui/file_manager/file_manager...
ui/file_manager/file_manager/foreground/js/file_manager.js:2240:
'volume-type-icon', location.rootType);
On 2014/08/07 03:59:22, hirono wrote:
> nit: Indent should be 4. Same for the followings.

Done.

https://codereview.chromium.org/443333003/diff/1/ui/file_manager/file_manager...
ui/file_manager/file_manager/foreground/js/file_manager.js:2933: * Invoked when
the search icon is clicked.
On 2014/08/07 03:59:22, hirono wrote:
> nit: Though old code does not follow the rule, we would start function
comments
> with verb + 's'. e.g. Handles ... events.

Done.

https://codereview.chromium.org/443333003/diff/1/ui/file_manager/file_manager...
ui/file_manager/file_manager/foreground/js/file_manager.js:2942: * Invoked when
the toggle-view button is clicked.
On 2014/08/07 03:59:22, hirono wrote:
> ditto.

Done.

https://codereview.chromium.org/443333003/diff/1/ui/file_manager/file_manager...
File ui/file_manager/file_manager/foreground/js/ui/search_box.js (right):

https://codereview.chromium.org/443333003/diff/1/ui/file_manager/file_manager...
ui/file_manager/file_manager/foreground/js/ui/search_box.js:43:
this.textMeasure_ = new TextMeasure(this.inputElement);
On 2014/08/07 05:08:54, hirono wrote:
> Can we remove the TextMeasure class?

I think so as the new input box have fixed width.
Removed.

[hirono, 2014-08-07 06:57:51]

Thanks! lgtm with a nit.

https://codereview.chromium.org/443333003/diff/1/ui/file_manager/file_manager...
File ui/file_manager/file_manager/foreground/css/file_manager.css (right):

https://codereview.chromium.org/443333003/diff/1/ui/file_manager/file_manager...
ui/file_manager/file_manager/foreground/css/file_manager.css:489:
background-image: -webkit-image-set(
On 2014/08/07 06:45:00, fukino wrote:
> On 2014/08/07 03:59:22, hirono wrote:
> > nit: shorthand properties are preferred.
> >
>
https://google-styleguide.googlecode.com/svn/trunk/htmlcssguide.xml#Shorthand...
> 
> I want to avoid overwriting the backgroun-color defined for topbotton-bar
> button:hover in
>
https://code.google.com/p/chromium/codesearch#chromium/src/ui/webui/resources....
> (Using shorthand for ".topbutton-bar .view-button.grid" overwrites it.)
> 
> If you know a way to avoid this, please let me know!

Ah, I got it.
There is no way to avoid this AFAIK.
Thank you!

https://codereview.chromium.org/443333003/diff/1/ui/file_manager/file_manager...
File ui/file_manager/file_manager/foreground/js/ui/search_box.js (right):

https://codereview.chromium.org/443333003/diff/1/ui/file_manager/file_manager...
ui/file_manager/file_manager/foreground/js/ui/search_box.js:43:
this.textMeasure_ = new TextMeasure(this.inputElement);
On 2014/08/07 06:45:01, fukino wrote:
> On 2014/08/07 05:08:54, hirono wrote:
> > Can we remove the TextMeasure class?
> 
> I think so as the new input box have fixed width.
> Removed.

Could you also remove the TextMeasure class itself? The class is no longer used.

[fukino, 2014-08-07 08:18:10]

I add additional differences regarding browser_tests and your comment.
- Removed TextMeasure completely.
- Fixed some broken browser tests.
- Adjust tab order as there're some new elements. 

Could you take a look at these? Thanks!

[hirono, 2014-08-07 08:25:04]

https://codereview.chromium.org/443333003/diff/40001/chrome/test/data/extensi...
File
chrome/test/data/extensions/api_test/file_manager_browsertest/file_manager/restore_geometry.js
(right):

https://codereview.chromium.org/443333003/diff/40001/chrome/test/data/extensi...
chrome/test/data/extensions/api_test/file_manager_browsertest/file_manager/restore_geometry.js:27:
waitForWindowGeometry(appId, 640, 480 - 33).then(this.next);
Maybe we can change them outerWidth/outerHeight.
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/test/data/e...

[fukino, 2014-08-07 08:49:25]

Thanks!

Uses outer width/height to confirm the sizes.
waitForWindowGeometry() is the only function which reference inner width/height,
so I removed inner size from the return value of test.util.getWindows().

https://codereview.chromium.org/443333003/diff/40001/chrome/test/data/extensi...
File
chrome/test/data/extensions/api_test/file_manager_browsertest/file_manager/restore_geometry.js
(right):

https://codereview.chromium.org/443333003/diff/40001/chrome/test/data/extensi...
chrome/test/data/extensions/api_test/file_manager_browsertest/file_manager/restore_geometry.js:27:
waitForWindowGeometry(appId, 640, 480 - 33).then(this.next);
On 2014/08/07 08:25:03, hirono wrote:
> Maybe we can change them outerWidth/outerHeight.
>
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/test/data/e...

Done.

[hirono, 2014-08-07 08:54:21]

lgtm again! Thank you!

[fukino, 2014-08-07 09:03:53]

On 2014/08/07 08:54:21, hirono wrote:
> lgtm again! Thank you!

Thank you for very quick reviews!

[fukino, 2014-08-07 15:50:45]

The CQ bit was checked by fukino@chromium.org

[commit-bot: I haz the power, 2014-08-07 15:55:22]

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

[commit-bot: I haz the power, 2014-08-07 16:49:49]

Change committed as 288082