Add icon-visibility attribute to cr.ui.Tree.

Add icon-visibility attribute to cr.ui.Tree.

This adds an icon-visibility attribute to cr.ui.Tree. If 'hidden', no space is reserved beside .tree-item labels and icons are not displayed. 'all' => show folder icons beside all nodes. 'parent' => show folder icons beside expandable/parent nodes. Removes icons from certificate viewer.

BUG=101920
TEST=None


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

[flackr, 2011-10-27 21:22:19]

We discussed a few ways of allowing the Tree icons to be removed, this was one
of the ways we thought might be more portable. PTAL, thanks!

[arv (Not doing code reviews), 2011-10-28 00:03:40]

Do you mind adding a property to cr.ui.Tree that reflects this?

http://codereview.chromium.org/8341081/diff/1/chrome/browser/resources/option...
File chrome/browser/resources/options/certificate_manager.html (right):

http://codereview.chromium.org/8341081/diff/1/chrome/browser/resources/option...
chrome/browser/resources/options/certificate_manager.html:39:
icon-visibility="parent"></tree>
indent 4 spaces when breaking the start tag

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

http://codereview.chromium.org/8341081/diff/1/chrome/browser/resources/shared...
chrome/browser/resources/shared/css/tree.css:111: tree[icon-visibility='all']
.tree-label,
remove quotes for consistency... oh it is already inconsistent in this file. Do
you mind fixing?

[flackr, 2011-10-28 15:19:36]

Is undefined a reasonable value? Or should I use something like
icon-visibility=hidden to hide icons?

http://codereview.chromium.org/8341081/diff/1/chrome/browser/resources/option...
File chrome/browser/resources/options/certificate_manager.html (right):

http://codereview.chromium.org/8341081/diff/1/chrome/browser/resources/option...
chrome/browser/resources/options/certificate_manager.html:39:
icon-visibility="parent"></tree>
On 2011/10/28 00:03:40, arv wrote:
> indent 4 spaces when breaking the start tag

Done, and fixed the other inconsistencies.

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

http://codereview.chromium.org/8341081/diff/1/chrome/browser/resources/shared...
chrome/browser/resources/shared/css/tree.css:111: tree[icon-visibility='all']
.tree-label,
On 2011/10/28 00:03:40, arv wrote:
> remove quotes for consistency... oh it is already inconsistent in this file.
Do
> you mind fixing?

Done.

[arv (Not doing code reviews), 2011-10-28 16:15:12]

http://codereview.chromium.org/8341081/diff/4002/chrome/browser/resources/sha...
File chrome/browser/resources/shared/css/tree.css (right):

http://codereview.chromium.org/8341081/diff/4002/chrome/browser/resources/sha...
chrome/browser/resources/shared/css/tree.css:105: tree[icon-visibility]
.tree-label {
Sorry, I should have paid more attention the first round.

<tree> should work like before. The default behavior should not require an
attribute.

So what are the options here?

a) <tree>
b) <tree icon-visibility="">
c) <tree icon-visibility="all">
d) <tree icon-visibility="parent">

A and c seems to be the same to me then. What does b do?

http://codereview.chromium.org/8341081/diff/4002/chrome/browser/resources/sha...
File chrome/browser/resources/shared/js/cr/ui/tree.js (right):

http://codereview.chromium.org/8341081/diff/4002/chrome/browser/resources/sha...
chrome/browser/resources/shared/js/cr/ui/tree.js:227: cr.defineProperty(Tree,
'icon-visibility', cr.PropertyKind.ATTR);
Sorry, this will not work. The property name should be iconVisibility.

The default is probably 'all'?

[flackr, 2011-10-28 22:32:50]

http://codereview.chromium.org/8341081/diff/4002/chrome/browser/resources/sha...
File chrome/browser/resources/shared/css/tree.css (right):

http://codereview.chromium.org/8341081/diff/4002/chrome/browser/resources/sha...
chrome/browser/resources/shared/css/tree.css:105: tree[icon-visibility]
.tree-label {
If A and C are the same then this needs to change. Before, A did not reserve
space for icons, where B does. The way it is now (not having set a default
property value), A and B will be the same as D, not C.

On 2011/10/28 16:15:12, arv wrote:
> Sorry, I should have paid more attention the first round.
> 
> <tree> should work like before. The default behavior should not require an
> attribute.
> 
> So what are the options here?
> 
> a) <tree>
> b) <tree icon-visibility="">
> c) <tree icon-visibility="all">
> d) <tree icon-visibility="parent">
> 
> A and c seems to be the same to me then. What does b do?

http://codereview.chromium.org/8341081/diff/4002/chrome/browser/resources/sha...
File chrome/browser/resources/shared/js/cr/ui/tree.js (right):

http://codereview.chromium.org/8341081/diff/4002/chrome/browser/resources/sha...
chrome/browser/resources/shared/js/cr/ui/tree.js:227: cr.defineProperty(Tree,
'icon-visibility', cr.PropertyKind.ATTR);
On 2011/10/28 16:15:12, arv wrote:
> Sorry, this will not work. The property name should be iconVisibility.
> 
> The default is probably 'all'?

Sorry, is there an appropriate place to define the default? I've looked for
similar examples and checked the cr API. As it is with this patch the default
(undefined) will behave the same as 'parent'.

[arv (Not doing code reviews), 2011-10-28 22:41:03]

http://codereview.chromium.org/8341081/diff/4002/chrome/browser/resources/sha...
File chrome/browser/resources/shared/css/tree.css (right):

http://codereview.chromium.org/8341081/diff/4002/chrome/browser/resources/sha...
chrome/browser/resources/shared/css/tree.css:105: tree[icon-visibility]
.tree-label {
On 2011/10/28 22:32:50, flackr wrote:
> If A and C are the same then this needs to change. Before, A did not reserve
> space for icons, where B does. The way it is now (not having set a default
> property value), A and B will be the same as D, not C.
> 
> On 2011/10/28 16:15:12, arv wrote:
> > Sorry, I should have paid more attention the first round.
> > 
> > <tree> should work like before. The default behavior should not require an
> > attribute.
> > 
> > So what are the options here?
> > 
> > a) <tree>
> > b) <tree icon-visibility="">
> > c) <tree icon-visibility="all">
> > d) <tree icon-visibility="parent">
> > 
> > A and c seems to be the same to me then. What does b do?
> 

To me, the important part is that <tree> does not change. I also find <tree
icon-visibility=""> strange.

http://codereview.chromium.org/8341081/diff/4002/chrome/browser/resources/sha...
File chrome/browser/resources/shared/js/cr/ui/tree.js (right):

http://codereview.chromium.org/8341081/diff/4002/chrome/browser/resources/sha...
chrome/browser/resources/shared/js/cr/ui/tree.js:227: cr.defineProperty(Tree,
'icon-visibility', cr.PropertyKind.ATTR);
On 2011/10/28 22:32:50, flackr wrote:
> On 2011/10/28 16:15:12, arv wrote:
> > Sorry, this will not work. The property name should be iconVisibility.
> > 
> > The default is probably 'all'?
> 
> Sorry, is there an appropriate place to define the default? I've looked for
> similar examples and checked the cr API. As it is with this patch the default
> (undefined) will behave the same as 'parent'.

I think I actually removed support for default value for attribute mapped
properties since it adds complexity.

[flackr, 2011-10-31 14:37:56]

http://codereview.chromium.org/8341081/diff/4002/chrome/browser/resources/sha...
File chrome/browser/resources/shared/css/tree.css (right):

http://codereview.chromium.org/8341081/diff/4002/chrome/browser/resources/sha...
chrome/browser/resources/shared/css/tree.css:105: tree[icon-visibility]
.tree-label {
On 2011/10/28 22:41:03, arv wrote:
> On 2011/10/28 22:32:50, flackr wrote:
> > If A and C are the same then this needs to change. Before, A did not reserve
> > space for icons, where B does. The way it is now (not having set a default
> > property value), A and B will be the same as D, not C.
> > 
> > On 2011/10/28 16:15:12, arv wrote:
> > > Sorry, I should have paid more attention the first round.
> > > 
> > > <tree> should work like before. The default behavior should not require an
> > > attribute.
> > > 
> > > So what are the options here?
> > > 
> > > a) <tree>
> > > b) <tree icon-visibility="">
> > > c) <tree icon-visibility="all">
> > > d) <tree icon-visibility="parent">
> > > 
> > > A and c seems to be the same to me then. What does b do?
> > 
> 
> To me, the important part is that <tree> does not change. I also find <tree
> icon-visibility=""> strange.

Okay, the default behaviour will be as it was before, the equivalent of 'all'.
There's no reason to use icon-visibility="" as it's no different than not
specifying it now.

http://codereview.chromium.org/8341081/diff/4002/chrome/browser/resources/sha...
File chrome/browser/resources/shared/js/cr/ui/tree.js (right):

http://codereview.chromium.org/8341081/diff/4002/chrome/browser/resources/sha...
chrome/browser/resources/shared/js/cr/ui/tree.js:227: cr.defineProperty(Tree,
'icon-visibility', cr.PropertyKind.ATTR);
On 2011/10/28 22:41:03, arv wrote:
> On 2011/10/28 22:32:50, flackr wrote:
> > On 2011/10/28 16:15:12, arv wrote:
> > > Sorry, this will not work. The property name should be iconVisibility.
> > > 
> > > The default is probably 'all'?
> > 
> > Sorry, is there an appropriate place to define the default? I've looked for
> > similar examples and checked the cr API. As it is with this patch the
default
> > (undefined) will behave the same as 'parent'.
> 
> I think I actually removed support for default value for attribute mapped
> properties since it adds complexity.

No problem, I changed the styles to make anything other than 'hidden' or
'parent' (including nothing) behave as 'all'.

[arv (Not doing code reviews), 2011-10-31 18:44:54]

http://codereview.chromium.org/8341081/diff/6001/chrome/browser/resources/boo...
File chrome/browser/resources/bookmark_manager/main.html (right):

http://codereview.chromium.org/8341081/diff/6001/chrome/browser/resources/boo...
chrome/browser/resources/bookmark_manager/main.html:77: <tree id=tree tabindex=2
role="tree" icon-visibility="all"></tree>
this can be removed now

http://codereview.chromium.org/8341081/diff/6001/chrome/browser/resources/quo...
File chrome/browser/resources/quota_internals/main.html (right):

http://codereview.chromium.org/8341081/diff/6001/chrome/browser/resources/quo...
chrome/browser/resources/quota_internals/main.html:58: <tree id="tree-view"
icon-visibility="all"></tree>
this can be removed now

http://codereview.chromium.org/8341081/diff/6001/chrome/browser/resources/sha...
File chrome/browser/resources/shared/js/cr/ui/tree.js (right):

http://codereview.chromium.org/8341081/diff/6001/chrome/browser/resources/sha...
chrome/browser/resources/shared/js/cr/ui/tree.js:228: cr.defineProperty(Tree,
'iconVisibility', cr.PropertyKind.ATTR);
Sorry this will not work. We need to translate the name.

We would need to update defineProperty to allow passing dash-separated-words and
then convert those to camelCase for the property. Another option could be to
pass an array of the two names instead. This would be more flexible since it
could allow cases like ['tabIndex', 'tabindex'] to work.

http://www.google.com/codesearch#OAMlx_jo-ck/src/chrome/browser/resources/sha...

[flackr, 2011-11-01 17:06:14]

http://codereview.chromium.org/8341081/diff/6001/chrome/browser/resources/boo...
File chrome/browser/resources/bookmark_manager/main.html (right):

http://codereview.chromium.org/8341081/diff/6001/chrome/browser/resources/boo...
chrome/browser/resources/bookmark_manager/main.html:77: <tree id=tree tabindex=2
role="tree" icon-visibility="all"></tree>
On 2011/10/31 18:44:54, arv wrote:
> this can be removed now

Done.

http://codereview.chromium.org/8341081/diff/6001/chrome/browser/resources/quo...
File chrome/browser/resources/quota_internals/main.html (right):

http://codereview.chromium.org/8341081/diff/6001/chrome/browser/resources/quo...
chrome/browser/resources/quota_internals/main.html:58: <tree id="tree-view"
icon-visibility="all"></tree>
On 2011/10/31 18:44:54, arv wrote:
> this can be removed now

Done.

http://codereview.chromium.org/8341081/diff/6001/chrome/browser/resources/sha...
File chrome/browser/resources/shared/js/cr/ui/tree.js (right):

http://codereview.chromium.org/8341081/diff/6001/chrome/browser/resources/sha...
chrome/browser/resources/shared/js/cr/ui/tree.js:228: cr.defineProperty(Tree,
'iconVisibility', cr.PropertyKind.ATTR);
On 2011/10/31 18:44:54, arv wrote:
> Sorry this will not work. We need to translate the name.
> 
> We would need to update defineProperty to allow passing dash-separated-words
and
> then convert those to camelCase for the property. Another option could be to
> pass an array of the two names instead. This would be more flexible since it
> could allow cases like ['tabIndex', 'tabindex'] to work.
> 
>
http://www.google.com/codesearch#OAMlx_jo-ck/src/chrome/browser/resources/sha...

Have a look, I think this should work.

[arv1, 2011-11-01 17:46:23]

http://codereview.chromium.org/8341081/diff/14001/chrome/browser/resources/sh...
File chrome/browser/resources/shared/js/cr.js (right):

http://codereview.chromium.org/8341081/diff/14001/chrome/browser/resources/sh...
chrome/browser/resources/shared/js/cr.js:145: return
jsName.replace(/([a-z](?=[A-Z])|[A-Z](?=[A-Z][a-z]))/g, '$&-')
This looks overly complex. The following works just as well.

return jsName.replace(/([A-Z])/g, '-$1').toLowerCase());

(this is the same code we have in Closure)

[flackr, 2011-11-01 17:55:46]

http://codereview.chromium.org/8341081/diff/14001/chrome/browser/resources/sh...
File chrome/browser/resources/shared/js/cr.js (right):

http://codereview.chromium.org/8341081/diff/14001/chrome/browser/resources/sh...
chrome/browser/resources/shared/js/cr.js:145: return
jsName.replace(/([a-z](?=[A-Z])|[A-Z](?=[A-Z][a-z]))/g, '$&-')
On 2011/11/01 17:46:24, arv1 wrote:
> This looks overly complex. The following works just as well.
> 
> return jsName.replace(/([A-Z])/g, '-$1').toLowerCase());
> 
> (this is the same code we have in Closure)

Done. I guess worrying about handling uppercase acronyms is overkill?

[arv1, 2011-11-01 18:13:58]

LGTM

Thanks for being patient with an old, slow reviewer.

[commit-bot: I haz the power, 2011-11-01 18:23:42]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/flackr@chromium.org/8341081/13002

[commit-bot: I haz the power, 2011-11-01 20:30:24]

Change committed as 108160