DOMUI: Implement new-style password manager.

DOMUI: Implement new-style password manager.

BUG=59282
TEST=none

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

[James Hawkins, 2010-12-16 19:28:28]

http://codereview.chromium.org/5935003/diff/3001/chrome/browser/resources/opt...
File chrome/browser/resources/options/deletable_item_list.js (right):

http://codereview.chromium.org/5935003/diff/3001/chrome/browser/resources/opt...
chrome/browser/resources/options/deletable_item_list.js:57:
this.baseItem_.selected = this.selected;
I'm not particularly happy with this API.  The fact that we have to bubble up a
virtual function seems wrong.  We should probably use DeletableListItem directly
in any user of DeletableItemList.  I also talked with Stuart about moving this
functionality into List itself, since most uses of List need the ability to
delete items from the list.

[Evan Stade, 2010-12-16 21:06:47]

http://codereview.chromium.org/5935003/diff/6001/chrome/browser/resources/opt...
File chrome/browser/resources/options/deletable_item_list.js (right):

http://codereview.chromium.org/5935003/diff/6001/chrome/browser/resources/opt...
chrome/browser/resources/options/deletable_item_list.js:47: //this.className =
'deletable-item';
remove?

http://codereview.chromium.org/5935003/diff/6001/chrome/browser/resources/opt...
File chrome/browser/resources/options/options_page.css (right):

http://codereview.chromium.org/5935003/diff/6001/chrome/browser/resources/opt...
chrome/browser/resources/options/options_page.css:395: -webkit-box-flex: 1;
for webkit-box-flex to have an effect, doesn't the parent have to have display:
-webkit-box? but it seems the parent div no longer has the display set

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

http://codereview.chromium.org/5935003/diff/6001/chrome/browser/resources/sha...
chrome/browser/resources/shared/js/cr/ui/list_item.js:38: selectionChanged:
function() {
why not just use

item.addEventListener('selectedChange', function(event) {});

[James Hawkins, 2010-12-16 23:03:01]

http://codereview.chromium.org/5935003/diff/6001/chrome/browser/resources/opt...
File chrome/browser/resources/options/deletable_item_list.js (right):

http://codereview.chromium.org/5935003/diff/6001/chrome/browser/resources/opt...
chrome/browser/resources/options/deletable_item_list.js:47: //this.className =
'deletable-item';
On 2010/12/16 21:06:47, Evan Stade wrote:
> remove?

Done.

http://codereview.chromium.org/5935003/diff/6001/chrome/browser/resources/opt...
File chrome/browser/resources/options/options_page.css (right):

http://codereview.chromium.org/5935003/diff/6001/chrome/browser/resources/opt...
chrome/browser/resources/options/options_page.css:395: -webkit-box-flex: 1;
On 2010/12/16 21:06:47, Evan Stade wrote:
> for webkit-box-flex to have an effect, doesn't the parent have to have
display:
> -webkit-box? but it seems the parent div no longer has the display set

The parent div gets -webkit-box from the rule "list > *" in options_page.css.

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

http://codereview.chromium.org/5935003/diff/6001/chrome/browser/resources/sha...
chrome/browser/resources/shared/js/cr/ui/list_item.js:38: selectionChanged:
function() {
On 2010/12/16 21:06:47, Evan Stade wrote:
> why not just use
> 
> item.addEventListener('selectedChange', function(event) {});

That's an option, but arv and I agree it's better to have a virtual callback
than to listen to yourself for the event.

[csilv, 2010-12-16 23:38:02]

LGTM

I like the idea of building delete functionality into the list class, assuming
that it doesn't overly complicate it and can be disabled.

http://codereview.chromium.org/5935003/diff/14001/chrome/app/generated_resour...
File chrome/app/generated_resources.grd (right):

http://codereview.chromium.org/5935003/diff/14001/chrome/app/generated_resour...
chrome/app/generated_resources.grd:4632: <message
name="IDS_PASSWORDS_EXCEPTIONS_WINDOW_TITLE" desc="Title for 'Passwords and
exceptions dialog'">
Changing these strings affects the native dialogs also.  Is that acceptable
given the native dialogs are going away?

[James Hawkins, 2010-12-16 23:40:07]

http://codereview.chromium.org/5935003/diff/14001/chrome/app/generated_resour...
File chrome/app/generated_resources.grd (right):

http://codereview.chromium.org/5935003/diff/14001/chrome/app/generated_resour...
chrome/app/generated_resources.grd:4632: <message
name="IDS_PASSWORDS_EXCEPTIONS_WINDOW_TITLE" desc="Title for 'Passwords and
exceptions dialog'">
On 2010/12/16 23:38:03, csilv wrote:
> Changing these strings affects the native dialogs also.  Is that acceptable
> given the native dialogs are going away?

Yap.

[stuartmorgan, 2010-12-17 00:40:21]

http://codereview.chromium.org/5935003/diff/14001/chrome/browser/resources/op...
File chrome/browser/resources/options/options_page.css (right):

http://codereview.chromium.org/5935003/diff/14001/chrome/browser/resources/op...
chrome/browser/resources/options/options_page.css:409: visibility: hidden;
Are we sure we want this behavior? I like getting more text when not hovering.

http://codereview.chromium.org/5935003/diff/14001/chrome/browser/resources/op...
File chrome/browser/resources/options/password_manager.css (right):

http://codereview.chromium.org/5935003/diff/14001/chrome/browser/resources/op...
chrome/browser/resources/options/password_manager.css:10: padding-bottom: 1px;
Remove this; it's no longer necessary after my recent change to list style.

http://codereview.chromium.org/5935003/diff/14001/chrome/browser/resources/op...
chrome/browser/resources/options/password_manager.css:20: width: 20%;
Why both a width a flex? Which one wins?

http://codereview.chromium.org/5935003/diff/14001/chrome/browser/resources/op...
chrome/browser/resources/options/password_manager.css:31: right: 50px;
This needs to be different for ltr vs rtl; see the main options CSS for
examples.

http://codereview.chromium.org/5935003/diff/14001/chrome/browser/resources/op...
File chrome/browser/resources/options/password_manager.js (right):

http://codereview.chromium.org/5935003/diff/14001/chrome/browser/resources/op...
chrome/browser/resources/options/password_manager.js:30: savedPasswordsList_:
null,
Should these be documented? I've been documenting all member variables in JS.

http://codereview.chromium.org/5935003/diff/14001/chrome/browser/resources/op...
chrome/browser/resources/options/password_manager.js:42:
createSavedPasswordsList_: function() {
Needs documentation

http://codereview.chromium.org/5935003/diff/14001/chrome/browser/resources/op...
chrome/browser/resources/options/password_manager.js:58: setSavedPasswordsList_:
function(entries) {
Ditto

http://codereview.chromium.org/5935003/diff/14001/chrome/browser/resources/op...
chrome/browser/resources/options/password_manager.js:62:
setPasswordExceptionsList_: function(entries) {
Ditto

[James Hawkins, 2010-12-17 02:33:57]

http://codereview.chromium.org/5935003/diff/14001/chrome/browser/resources/op...
File chrome/browser/resources/options/options_page.css (right):

http://codereview.chromium.org/5935003/diff/14001/chrome/browser/resources/op...
chrome/browser/resources/options/options_page.css:409: visibility: hidden;
On 2010/12/17 00:40:21, stuartmorgan wrote:
> Are we sure we want this behavior? I like getting more text when not hovering.

As discussed offline, this is good to go.

http://codereview.chromium.org/5935003/diff/14001/chrome/browser/resources/op...
File chrome/browser/resources/options/password_manager.css (right):

http://codereview.chromium.org/5935003/diff/14001/chrome/browser/resources/op...
chrome/browser/resources/options/password_manager.css:10: padding-bottom: 1px;
On 2010/12/17 00:40:21, stuartmorgan wrote:
> Remove this; it's no longer necessary after my recent change to list style.

Done.

http://codereview.chromium.org/5935003/diff/14001/chrome/browser/resources/op...
chrome/browser/resources/options/password_manager.css:20: width: 20%;
On 2010/12/17 00:40:21, stuartmorgan wrote:
> Why both a width a flex? Which one wins?

No, they're both important and affect the layout, though I'm having difficulty
explaining it.

http://codereview.chromium.org/5935003/diff/14001/chrome/browser/resources/op...
chrome/browser/resources/options/password_manager.css:31: right: 50px;
On 2010/12/17 00:40:21, stuartmorgan wrote:
> This needs to be different for ltr vs rtl; see the main options CSS for
> examples.

Done.

http://codereview.chromium.org/5935003/diff/14001/chrome/browser/resources/op...
File chrome/browser/resources/options/password_manager.js (right):

http://codereview.chromium.org/5935003/diff/14001/chrome/browser/resources/op...
chrome/browser/resources/options/password_manager.js:30: savedPasswordsList_:
null,
On 2010/12/17 00:40:21, stuartmorgan wrote:
> Should these be documented? I've been documenting all member variables in JS.

Done.

http://codereview.chromium.org/5935003/diff/14001/chrome/browser/resources/op...
chrome/browser/resources/options/password_manager.js:58: setSavedPasswordsList_:
function(entries) {
On 2010/12/17 00:40:21, stuartmorgan wrote:
> Ditto

Done.

http://codereview.chromium.org/5935003/diff/14001/chrome/browser/resources/op...
chrome/browser/resources/options/password_manager.js:62:
setPasswordExceptionsList_: function(entries) {
On 2010/12/17 00:40:21, stuartmorgan wrote:
> Ditto

Done.

[stuartmorgan, 2010-12-17 17:27:42]

LGTM except for some CSS nits I missed before.

http://codereview.chromium.org/5935003/diff/22001/chrome/browser/resources/op...
File chrome/browser/resources/options/password_manager.css (right):

http://codereview.chromium.org/5935003/diff/22001/chrome/browser/resources/op...
chrome/browser/resources/options/password_manager.css:1: .list-title {
This should be in the main CSS if it's general purpose, or have a more specific
title if not; the possibility for collision here is high.

http://codereview.chromium.org/5935003/diff/22001/chrome/browser/resources/op...
chrome/browser/resources/options/password_manager.css:33: cursor: pointer;
This whole rule set needs alphabetizing.

[arv (Not doing code reviews), 2010-12-17 20:07:34]

http://codereview.chromium.org/5935003/diff/22001/chrome/browser/resources/op...
File chrome/browser/resources/options/options_page.css (right):

http://codereview.chromium.org/5935003/diff/22001/chrome/browser/resources/op...
chrome/browser/resources/options/options_page.css:409: visibility: hidden;
Now that we control this using visibility maybe we should use "opacity: 0" and
"pointer-events: none" so that we can have a transition on the opacity?

http://codereview.chromium.org/5935003/diff/22001/chrome/browser/resources/op...
File chrome/browser/resources/options/password_manager.css (right):

http://codereview.chromium.org/5935003/diff/22001/chrome/browser/resources/op...
chrome/browser/resources/options/password_manager.css:27: #saved-passwords-list
.password span {
Can some of these be direct descendant selectors instead?

http://codereview.chromium.org/5935003/diff/22001/chrome/browser/resources/op...
File chrome/browser/resources/options/password_manager.js (right):

http://codereview.chromium.org/5935003/diff/22001/chrome/browser/resources/op...
chrome/browser/resources/options/password_manager.js:32: * @type
{!DeletableItemList}
! means non nullable

http://codereview.chromium.org/5935003/diff/22001/chrome/browser/resources/op...
File chrome/browser/resources/options/password_manager_list.js (right):

http://codereview.chromium.org/5935003/diff/22001/chrome/browser/resources/op...
chrome/browser/resources/options/password_manager_list.js:90: passwordInput.type
= (type == 'password') ? 'text' : 'password';
useless parens

http://codereview.chromium.org/5935003/diff/22001/chrome/browser/resources/sh...
File chrome/browser/resources/shared/js/cr/ui/list.js (right):

http://codereview.chromium.org/5935003/diff/22001/chrome/browser/resources/sh...
chrome/browser/resources/shared/js/cr/ui/list.js:351:
listItem.selectionChanged();
You could just have made the selected setter virtual.

http://codereview.chromium.org/5935003/diff/22001/chrome/browser/resources/sh...
File chrome/browser/resources/shared/js/cr/ui/list_item.js (right):

http://codereview.chromium.org/5935003/diff/22001/chrome/browser/resources/sh...
chrome/browser/resources/shared/js/cr/ui/list_item.js:38: selectionChanged:
function() {
I think a better way is to change the current selected setter to call this. That
unfortunately means that the macro can no longer be used but at least it
guarantees that we always call selectionChanged when selected changes instead of
relying on the caller to take care of it.

get selected() {
  return this.hasAttribute('selected');
},

set selected(selected) {
  selected = Boolean(selected);
  var oldSelected = this.selected;
  if (oldSelected == selected)
    return;
  if (selected)
    this.setAttribute('selected', '');
  else
    this.removeAttribute('selected');
  cr.dispatchPropertyChange(this, 'selected', selected, oldSelected);
  this.selectionChanged();
}

[James Hawkins, 2010-12-17 21:32:39]

http://codereview.chromium.org/5935003/diff/22001/chrome/browser/resources/op...
File chrome/browser/resources/options/options_page.css (right):

http://codereview.chromium.org/5935003/diff/22001/chrome/browser/resources/op...
chrome/browser/resources/options/options_page.css:409: visibility: hidden;
On 2010/12/17 20:07:34, arv wrote:
> Now that we control this using visibility maybe we should use "opacity: 0" and
> "pointer-events: none" so that we can have a transition on the opacity?

Can you describe the entire flow that you're imagining? Once the user hovers
over another item, do we transition the highlighting of the old and the new list
item as well?

http://codereview.chromium.org/5935003/diff/22001/chrome/browser/resources/op...
File chrome/browser/resources/options/password_manager.css (right):

http://codereview.chromium.org/5935003/diff/22001/chrome/browser/resources/op...
chrome/browser/resources/options/password_manager.css:1: .list-title {
On 2010/12/17 17:27:42, stuartmorgan wrote:
> This should be in the main CSS if it's general purpose, or have a more
specific
> title if not; the possibility for collision here is high.

Done.

http://codereview.chromium.org/5935003/diff/22001/chrome/browser/resources/op...
chrome/browser/resources/options/password_manager.css:27: #saved-passwords-list
.password span {
On 2010/12/17 20:07:34, arv wrote:
> Can some of these be direct descendant selectors instead?

Done.

http://codereview.chromium.org/5935003/diff/22001/chrome/browser/resources/op...
chrome/browser/resources/options/password_manager.css:33: cursor: pointer;
On 2010/12/17 17:27:42, stuartmorgan wrote:
> This whole rule set needs alphabetizing.

Done.

http://codereview.chromium.org/5935003/diff/22001/chrome/browser/resources/op...
File chrome/browser/resources/options/password_manager.js (right):

http://codereview.chromium.org/5935003/diff/22001/chrome/browser/resources/op...
chrome/browser/resources/options/password_manager.js:32: * @type
{!DeletableItemList}
On 2010/12/17 20:07:34, arv wrote:
> ! means non nullable

Done.

http://codereview.chromium.org/5935003/diff/22001/chrome/browser/resources/op...
File chrome/browser/resources/options/password_manager_list.js (right):

http://codereview.chromium.org/5935003/diff/22001/chrome/browser/resources/op...
chrome/browser/resources/options/password_manager_list.js:90: passwordInput.type
= (type == 'password') ? 'text' : 'password';
On 2010/12/17 20:07:34, arv wrote:
> useless parens

Done.

http://codereview.chromium.org/5935003/diff/22001/chrome/browser/resources/sh...
File chrome/browser/resources/shared/js/cr/ui/list.js (right):

http://codereview.chromium.org/5935003/diff/22001/chrome/browser/resources/sh...
chrome/browser/resources/shared/js/cr/ui/list.js:351:
listItem.selectionChanged();
On 2010/12/17 20:07:34, arv wrote:
> You could just have made the selected setter virtual.

Done.

http://codereview.chromium.org/5935003/diff/22001/chrome/browser/resources/sh...
File chrome/browser/resources/shared/js/cr/ui/list_item.js (right):

http://codereview.chromium.org/5935003/diff/22001/chrome/browser/resources/sh...
chrome/browser/resources/shared/js/cr/ui/list_item.js:38: selectionChanged:
function() {
On 2010/12/17 20:07:34, arv wrote:
> I think a better way is to change the current selected setter to call this.
That
> unfortunately means that the macro can no longer be used but at least it
> guarantees that we always call selectionChanged when selected changes instead
of
> relying on the caller to take care of it.
> 
> get selected() {
>   return this.hasAttribute('selected');
> },
> 
> set selected(selected) {
>   selected = Boolean(selected);
>   var oldSelected = this.selected;
>   if (oldSelected == selected)
>     return;
>   if (selected)
>     this.setAttribute('selected', '');
>   else
>     this.removeAttribute('selected');
>   cr.dispatchPropertyChange(this, 'selected', selected, oldSelected);
>   this.selectionChanged();
> }

Ok, I think I've made the change like you've suggested.  Question: why do you
cast selected to Boolean?  Is this a best practice to ensure that we're dealing
w/ a Boolean?

[arv (Not doing code reviews), 2010-12-17 21:46:20]

On Fri, Dec 17, 2010 at 13:32,  <jhawkins@chromium.org> wrote:
>> Now that we control this using visibility maybe we should use
>
> "opacity: 0" and
>>
>> "pointer-events: none" so that we can have a transition on the
>
> opacity?
>
> Can you describe the entire flow that you're imagining? Once the user
> hovers over another item, do we transition the highlighting of the old
> and the new list item as well?

OK.

To display the close button use "opacity: 1". For a hidden close
button use "opacity: 0; pointer-events: none". Then set
"-webkit-transition: .15s opacity" on the button. This way instead of
just instantly appearing and disappearing as it does with "visibility:
visible/hidden" we get a fast fade. All this is done in CSS and one of
the early goals for Chrome was (and still is) that all UI changes
should be animated.

Does that make sense?

-- 
erik

[arv (Not doing code reviews), 2010-12-17 21:48:19]

LGTM

http://codereview.chromium.org/5935003/diff/33001/chrome/browser/resources/sh...
File chrome/browser/resources/shared/js/cr/ui/list_item.js (right):

http://codereview.chromium.org/5935003/diff/33001/chrome/browser/resources/sh...
chrome/browser/resources/shared/js/cr/ui/list_item.js:37: selected =
Boolean(selected);
The reason for the Boolean case is so that the test below works even in case
where a non boolean is passed.

[James Hawkins, 2010-12-17 22:14:46]

On 2010/12/17 21:46:20, arv wrote:
> On Fri, Dec 17, 2010 at 13:32,  <mailto:jhawkins@chromium.org> wrote:
> >> Now that we control this using visibility maybe we should use
> >
> > "opacity: 0" and
> >>
> >> "pointer-events: none" so that we can have a transition on the
> >
> > opacity?
> >
> > Can you describe the entire flow that you're imagining? Once the user
> > hovers over another item, do we transition the highlighting of the old
> > and the new list item as well?
> 
> OK.
> 
> To display the close button use "opacity: 1". For a hidden close
> button use "opacity: 0; pointer-events: none". Then set
> "-webkit-transition: .15s opacity" on the button. This way instead of
> just instantly appearing and disappearing as it does with "visibility:
> visible/hidden" we get a fast fade. All this is done in CSS and one of
> the early goals for Chrome was (and still is) that all UI changes
> should be animated.
> 

I've added transitions for both the close button and the list item background
color.

[arv (Not doing code reviews), 2010-12-17 22:19:16]

LGTM again