[FileBrowser] Migrated ComboButton to menu as dropdown.

Migrated ComboButton to menu as dropdown.

Added menuItem icon support, and little API for menu modification

BUG=126927
TEST=

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

[Dmitry Zvorygin, 2012-05-17 12:25:55]

Please take a look

[dgozman, 2012-05-18 13:15:14]

http://codereview.chromium.org/10391184/diff/2001/chrome/browser/resources/fi...
File chrome/browser/resources/file_manager/css/common.css (right):

http://codereview.chromium.org/10391184/diff/2001/chrome/browser/resources/fi...
chrome/browser/resources/file_manager/css/common.css:214: 
extra empty line

http://codereview.chromium.org/10391184/diff/2001/chrome/browser/resources/fi...
File chrome/browser/resources/file_manager/js/combobutton.js (right):

http://codereview.chromium.org/10391184/diff/2001/chrome/browser/resources/fi...
chrome/browser/resources/file_manager/js/combobutton.js:32: addItem:
function(item) {
This method does not add the initially shown item (as one can think), so
consider renaming to |addDropDownItem|.

http://codereview.chromium.org/10391184/diff/2001/chrome/browser/resources/fi...
chrome/browser/resources/file_manager/js/combobutton.js:87: // Remove mousedown
event listener created by MenuButton::decorate,
I don't like this dependency on internals of MenuButton.
Why don't make trigger the menubutton itself instead?

http://codereview.chromium.org/10391184/diff/2001/chrome/browser/resources/fi...
chrome/browser/resources/file_manager/js/combobutton.js:133:
cr.defineProperty(ComboButton, 'label', cr.PropertyKind.ATTR);
Why do you need this?

http://codereview.chromium.org/10391184/diff/2001/chrome/browser/resources/fi...
File chrome/browser/resources/file_manager/js/file_manager.js (right):

http://codereview.chromium.org/10391184/diff/2001/chrome/browser/resources/fi...
chrome/browser/resources/file_manager/js/file_manager.js:2138: if (index == 0) {
Merge these changes.

http://codereview.chromium.org/10391184/diff/2001/chrome/browser/resources/fi...
chrome/browser/resources/file_manager/js/file_manager.js:2436: if
(tasksList.length > 1)
This is wrong. tasksList.length is not equal to tasksCount, since we can remove
some tasks from the list.

http://codereview.chromium.org/10391184/diff/2001/chrome/browser/resources/fi...
File chrome/browser/resources/file_manager/main.html (right):

http://codereview.chromium.org/10391184/diff/2001/chrome/browser/resources/fi...
chrome/browser/resources/file_manager/main.html:126: <menu id="tasks-menu"
class="chrome-menu" style="z-index:1000">
Move this to css. And 1000 scares me :)

[Dmitry Zvorygin, 2012-05-21 10:51:01]

Please take a look again.

http://codereview.chromium.org/10391184/diff/2001/chrome/browser/resources/fi...
File chrome/browser/resources/file_manager/css/common.css (right):

http://codereview.chromium.org/10391184/diff/2001/chrome/browser/resources/fi...
chrome/browser/resources/file_manager/css/common.css:214: 
On 2012/05/18 13:15:14, dgozman wrote:
> extra empty line

Done.

http://codereview.chromium.org/10391184/diff/2001/chrome/browser/resources/fi...
File chrome/browser/resources/file_manager/js/combobutton.js (right):

http://codereview.chromium.org/10391184/diff/2001/chrome/browser/resources/fi...
chrome/browser/resources/file_manager/js/combobutton.js:32: addItem:
function(item) {
On 2012/05/18 13:15:14, dgozman wrote:
> This method does not add the initially shown item (as one can think), so
> consider renaming to |addDropDownItem|.

Done.

http://codereview.chromium.org/10391184/diff/2001/chrome/browser/resources/fi...
chrome/browser/resources/file_manager/js/combobutton.js:87: // Remove mousedown
event listener created by MenuButton::decorate,
On 2012/05/18 13:15:14, dgozman wrote:
> I don't like this dependency on internals of MenuButton.
> Why don't make trigger the menubutton itself instead?

I don't like this complex logic too, but if we make trigger the menubutton
instead, menu layouting mechanics would fail, or we'd have to dig deeper into
menubutton to fix menu layouting.

http://codereview.chromium.org/10391184/diff/2001/chrome/browser/resources/fi...
chrome/browser/resources/file_manager/js/combobutton.js:133:
cr.defineProperty(ComboButton, 'label', cr.PropertyKind.ATTR);
On 2012/05/18 13:15:14, dgozman wrote:
> Why do you need this?

Removed.
Just some stuff from research has been left.

http://codereview.chromium.org/10391184/diff/2001/chrome/browser/resources/fi...
File chrome/browser/resources/file_manager/js/file_manager.js (right):

http://codereview.chromium.org/10391184/diff/2001/chrome/browser/resources/fi...
chrome/browser/resources/file_manager/js/file_manager.js:2138: if (index == 0) {
On 2012/05/18 13:15:14, dgozman wrote:
> Merge these changes.

Done.

http://codereview.chromium.org/10391184/diff/2001/chrome/browser/resources/fi...
chrome/browser/resources/file_manager/js/file_manager.js:2436: if
(tasksList.length > 1)
On 2012/05/18 13:15:14, dgozman wrote:
> This is wrong. tasksList.length is not equal to tasksCount, since we can
remove
> some tasks from the list.

Good point! Fixed.

http://codereview.chromium.org/10391184/diff/2001/chrome/browser/resources/fi...
File chrome/browser/resources/file_manager/main.html (right):

http://codereview.chromium.org/10391184/diff/2001/chrome/browser/resources/fi...
chrome/browser/resources/file_manager/main.html:126: <menu id="tasks-menu"
class="chrome-menu" style="z-index:1000">
On 2012/05/18 13:15:14, dgozman wrote:
> Move this to css. And 1000 scares me :)

Done.

[dgozman, 2012-05-21 12:40:38]

Add sorting of tasks please.

LGTM

http://codereview.chromium.org/10391184/diff/6002/chrome/browser/resources/fi...
File chrome/browser/resources/file_manager/js/file_manager.js (right):

http://codereview.chromium.org/10391184/diff/6002/chrome/browser/resources/fi...
chrome/browser/resources/file_manager/js/file_manager.js:2421: return { label:
task.title, iconUrl: task.iconUrl, task: task};
space before }

[Dmitry Zvorygin, 2012-05-21 13:20:14]

PTAL

http://codereview.chromium.org/10391184/diff/6002/chrome/browser/resources/fi...
File chrome/browser/resources/file_manager/js/file_manager.js (right):

http://codereview.chromium.org/10391184/diff/6002/chrome/browser/resources/fi...
chrome/browser/resources/file_manager/js/file_manager.js:2421: return { label:
task.title, iconUrl: task.iconUrl, task: task};
On 2012/05/21 12:40:38, dgozman wrote:
> space before }

Done.

[dgozman, 2012-05-21 13:48:36]

LGTM