Add an icon to "Install new extensions" in the providers popup menu.

Add an icon to "Install new extensions" in the providers popup menu.

This CL in order to add the icon implements a cr.ui.ProvidersMenuItem class
since cr.ui.MenuItem doesn't handle icons with hidpi properly, nor it supports
well custom icons (setting a label overwrites any inserted DOM elements to the
menu item).

TEST=Tested manually.
BUG=474146
Committed: https://crrev.com/5c5d52a7c5e1d81b78a6040582adcb3c8c9ed638
Cr-Commit-Position: refs/heads/master@{#329369}

[mtomasz, 2015-05-12 05:41:26]

mtomasz@chromium.org changed reviewers:
+ hirono@chromium.org

[mtomasz, 2015-05-12 05:41:35]

@hirono: PTAL. Thanks!

[hirono, 2015-05-12 05:56:46]

https://codereview.chromium.org/1132843004/diff/20001/ui/file_manager/file_ma...
File ui/file_manager/file_manager/foreground/js/ui/providers_menu.js (right):

https://codereview.chromium.org/1132843004/diff/20001/ui/file_manager/file_ma...
ui/file_manager/file_manager/foreground/js/ui/providers_menu.js:43: get
leftIconImage() {
Cannot we override iconUrl?

[mtomasz, 2015-05-12 06:01:04]

https://codereview.chromium.org/1132843004/diff/20001/ui/file_manager/file_ma...
File ui/file_manager/file_manager/foreground/js/ui/providers_menu.js (right):

https://codereview.chromium.org/1132843004/diff/20001/ui/file_manager/file_ma...
ui/file_manager/file_manager/foreground/js/ui/providers_menu.js:43: get
leftIconImage() {
On 2015/05/12 05:56:45, hirono wrote:
> Cannot we override iconUrl?

IMHO the naming would be confusing. The value of it is not a url anymore.

[mtomasz, 2015-05-12 06:02:57]

The CQ bit was checked by mtomasz@chromium.org

[mtomasz, 2015-05-12 06:02:58]

The CQ bit was unchecked by mtomasz@chromium.org

[hirono, 2015-05-12 06:56:56]

lgtm

https://codereview.chromium.org/1132843004/diff/20001/ui/file_manager/file_ma...
File ui/file_manager/file_manager/foreground/js/ui/providers_menu.js (right):

https://codereview.chromium.org/1132843004/diff/20001/ui/file_manager/file_ma...
ui/file_manager/file_manager/foreground/js/ui/providers_menu.js:43: get
leftIconImage() {
On 2015/05/12 06:01:04, mtomasz wrote:
> On 2015/05/12 05:56:45, hirono wrote:
> > Cannot we override iconUrl?
> 
> IMHO the naming would be confusing. The value of it is not a url anymore.

Having both iconUrl and leftIconImage is also a bit confusing to me. And
leftIconImage sounds the property takes Image object. My preference is
overriding iconUrl and renaming the property when you upstream the change to
cr.ui.MenuItem. But it's up to you.

If you choose adding new property for icon, just 'iconImage' may be better.
'left' is sometimes ambiguous in the context of RTL. Actually this is an icon
placed at the begin edge. But 'beginIconImage' sounds wrong.

[mtomasz, 2015-05-12 07:11:22]

https://codereview.chromium.org/1132843004/diff/20001/ui/file_manager/file_ma...
File ui/file_manager/file_manager/foreground/js/ui/providers_menu.js (right):

https://codereview.chromium.org/1132843004/diff/20001/ui/file_manager/file_ma...
ui/file_manager/file_manager/foreground/js/ui/providers_menu.js:43: get
leftIconImage() {
On 2015/05/12 06:56:56, hirono wrote:
> On 2015/05/12 06:01:04, mtomasz wrote:
> > On 2015/05/12 05:56:45, hirono wrote:
> > > Cannot we override iconUrl?
> > 
> > IMHO the naming would be confusing. The value of it is not a url anymore.
> 
> Having both iconUrl and leftIconImage is also a bit confusing to me. And
> leftIconImage sounds the property takes Image object. My preference is
> overriding iconUrl and renaming the property when you upstream the change to
> cr.ui.MenuItem. But it's up to you.
> 
> If you choose adding new property for icon, just 'iconImage' may be better.
> 'left' is sometimes ambiguous in the context of RTL. Actually this is an icon
> placed at the begin edge. But 'beginIconImage' sounds wrong.

My idea here was to just extend MenuItem and add new features, without changing
existing ones. So ProvidedMenuItem supports two types of icons: legacy one (only
low-dpi) and the new one on the left.

Indeed leftIconImage may be confusing, as well as iconUrl which doesn't hold a
url. How about renaming leftIconImage to extraIcon and when upstreaming combine
"iconUrl" and "extraIcon" into simply "icon"?

[hirono, 2015-05-12 07:40:34]

On 2015/05/12 07:11:22, mtomasz wrote:
>
https://codereview.chromium.org/1132843004/diff/20001/ui/file_manager/file_ma...
> File ui/file_manager/file_manager/foreground/js/ui/providers_menu.js (right):
> 
>
https://codereview.chromium.org/1132843004/diff/20001/ui/file_manager/file_ma...
> ui/file_manager/file_manager/foreground/js/ui/providers_menu.js:43: get
> leftIconImage() {
> On 2015/05/12 06:56:56, hirono wrote:
> > On 2015/05/12 06:01:04, mtomasz wrote:
> > > On 2015/05/12 05:56:45, hirono wrote:
> > > > Cannot we override iconUrl?
> > > 
> > > IMHO the naming would be confusing. The value of it is not a url anymore.
> > 
> > Having both iconUrl and leftIconImage is also a bit confusing to me. And
> > leftIconImage sounds the property takes Image object. My preference is
> > overriding iconUrl and renaming the property when you upstream the change to
> > cr.ui.MenuItem. But it's up to you.
> > 
> > If you choose adding new property for icon, just 'iconImage' may be better.
> > 'left' is sometimes ambiguous in the context of RTL. Actually this is an
icon
> > placed at the begin edge. But 'beginIconImage' sounds wrong.
> 
> My idea here was to just extend MenuItem and add new features, without
changing
> existing ones. So ProvidedMenuItem supports two types of icons: legacy one
(only
> low-dpi) and the new one on the left.
> 
> Indeed leftIconImage may be confusing, as well as iconUrl which doesn't hold a
> url. How about renaming leftIconImage to extraIcon and when upstreaming
combine
> "iconUrl" and "extraIcon" into simply "icon"?

SGTM. Thanks!

[mtomasz, 2015-05-12 07:49:56]

On 2015/05/12 07:40:34, hirono wrote:
> On 2015/05/12 07:11:22, mtomasz wrote:
> >
>
https://codereview.chromium.org/1132843004/diff/20001/ui/file_manager/file_ma...
> > File ui/file_manager/file_manager/foreground/js/ui/providers_menu.js
(right):
> > 
> >
>
https://codereview.chromium.org/1132843004/diff/20001/ui/file_manager/file_ma...
> > ui/file_manager/file_manager/foreground/js/ui/providers_menu.js:43: get
> > leftIconImage() {
> > On 2015/05/12 06:56:56, hirono wrote:
> > > On 2015/05/12 06:01:04, mtomasz wrote:
> > > > On 2015/05/12 05:56:45, hirono wrote:
> > > > > Cannot we override iconUrl?
> > > > 
> > > > IMHO the naming would be confusing. The value of it is not a url
anymore.
> > > 
> > > Having both iconUrl and leftIconImage is also a bit confusing to me. And
> > > leftIconImage sounds the property takes Image object. My preference is
> > > overriding iconUrl and renaming the property when you upstream the change
to
> > > cr.ui.MenuItem. But it's up to you.
> > > 
> > > If you choose adding new property for icon, just 'iconImage' may be
better.
> > > 'left' is sometimes ambiguous in the context of RTL. Actually this is an
> icon
> > > placed at the begin edge. But 'beginIconImage' sounds wrong.
> > 
> > My idea here was to just extend MenuItem and add new features, without
> changing
> > existing ones. So ProvidedMenuItem supports two types of icons: legacy one
> (only
> > low-dpi) and the new one on the left.
> > 
> > Indeed leftIconImage may be confusing, as well as iconUrl which doesn't hold
a
> > url. How about renaming leftIconImage to extraIcon and when upstreaming
> combine
> > "iconUrl" and "extraIcon" into simply "icon"?
> 
> SGTM. Thanks!

Thanks!

[mtomasz, 2015-05-12 07:50:01]

The CQ bit was checked by mtomasz@chromium.org

[commit-bot: I haz the power, 2015-05-12 07:50:29]

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

[commit-bot: I haz the power, 2015-05-12 07:54:24]

Committed patchset #2 (id:20001)

[commit-bot: I haz the power, 2015-05-12 07:55:18]

Patchset 2 (id:??) landed as
https://crrev.com/5c5d52a7c5e1d81b78a6040582adcb3c8c9ed638
Cr-Commit-Position: refs/heads/master@{#329369}