DOMUI: Implement the new-style Autofill options page.

DOMUI: Implement the new-style Autofill options page.

BUG=59281
TEST=none

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

[James Hawkins, 2010-12-22 01:39:40]

http://codereview.chromium.org/6034005/diff/3001/chrome/browser/dom_ui/option...
File chrome/browser/dom_ui/options/autofill_options_handler.cc (right):

http://codereview.chromium.org/6034005/diff/3001/chrome/browser/dom_ui/option...
chrome/browser/dom_ui/options/autofill_options_handler.cc:45:
localized_strings->SetString("editButton",
Not used, remove.

http://codereview.chromium.org/6034005/diff/3001/chrome/browser/dom_ui/option...
chrome/browser/dom_ui/options/autofill_options_handler.cc:47:
localized_strings->SetString("deleteButton",
Not used, remove.

http://codereview.chromium.org/6034005/diff/3001/chrome/browser/dom_ui/option...
chrome/browser/dom_ui/options/autofill_options_handler.cc:187:
printf("LoadProfileEditor\n");
Remove cruft.

http://codereview.chromium.org/6034005/diff/3001/chrome/browser/dom_ui/option...
File chrome/browser/dom_ui/options/autofill_options_handler.h (right):

http://codereview.chromium.org/6034005/diff/3001/chrome/browser/dom_ui/option...
chrome/browser/dom_ui/options/autofill_options_handler.h:47: // |args| - A
string, the unique ID of the profile to remove.
s/remove/load/

http://codereview.chromium.org/6034005/diff/3001/chrome/browser/dom_ui/option...
chrome/browser/dom_ui/options/autofill_options_handler.h:57: // Adds or updates
a credit card, depending on the GUID of the credit card.
s/credit card/profile/

[James Hawkins, 2010-12-22 02:23:53]

http://codereview.chromium.org/6034005/diff/3001/chrome/browser/dom_ui/option...
File chrome/browser/dom_ui/options/autofill_options_handler.cc (right):

http://codereview.chromium.org/6034005/diff/3001/chrome/browser/dom_ui/option...
chrome/browser/dom_ui/options/autofill_options_handler.cc:45:
localized_strings->SetString("editButton",
On 2010/12/22 01:39:40, James Hawkins wrote:
> Not used, remove.

Done.

http://codereview.chromium.org/6034005/diff/3001/chrome/browser/dom_ui/option...
chrome/browser/dom_ui/options/autofill_options_handler.cc:47:
localized_strings->SetString("deleteButton",
On 2010/12/22 01:39:40, James Hawkins wrote:
> Not used, remove.

Done.

http://codereview.chromium.org/6034005/diff/3001/chrome/browser/dom_ui/option...
chrome/browser/dom_ui/options/autofill_options_handler.cc:187:
printf("LoadProfileEditor\n");
On 2010/12/22 01:39:40, James Hawkins wrote:
> Remove cruft.

Done.

http://codereview.chromium.org/6034005/diff/3001/chrome/browser/dom_ui/option...
File chrome/browser/dom_ui/options/autofill_options_handler.h (right):

http://codereview.chromium.org/6034005/diff/3001/chrome/browser/dom_ui/option...
chrome/browser/dom_ui/options/autofill_options_handler.h:47: // |args| - A
string, the unique ID of the profile to remove.
On 2010/12/22 01:39:40, James Hawkins wrote:
> s/remove/load/

Done.

http://codereview.chromium.org/6034005/diff/3001/chrome/browser/dom_ui/option...
chrome/browser/dom_ui/options/autofill_options_handler.h:57: // Adds or updates
a credit card, depending on the GUID of the credit card.
On 2010/12/22 01:39:40, James Hawkins wrote:
> s/credit card/profile/

Done.

[arv (Not doing code reviews), 2010-12-22 18:14:07]

http://codereview.chromium.org/6034005/diff/8001/chrome/browser/dom_ui/option...
File chrome/browser/dom_ui/options/autofill_options_handler.cc (right):

http://codereview.chromium.org/6034005/diff/8001/chrome/browser/dom_ui/option...
chrome/browser/dom_ui/options/autofill_options_handler.cc:191: if
(personal_data_->GetProfileByGUID(guid) != NULL)
I find this confusing (and same in RemoveAutoFillProfile). Could it be made
clearer why a null profile means edit/remove credit card?

http://codereview.chromium.org/6034005/diff/8001/chrome/browser/resources/opt...
File chrome/browser/resources/options/autofill_options.css (right):

http://codereview.chromium.org/6034005/diff/8001/chrome/browser/resources/opt...
chrome/browser/resources/options/autofill_options.css:2: padding-left: 8px;
-webkit-padding-start?

http://codereview.chromium.org/6034005/diff/8001/chrome/browser/resources/opt...
File chrome/browser/resources/options/autofill_options.html (right):

http://codereview.chromium.org/6034005/diff/8001/chrome/browser/resources/opt...
chrome/browser/resources/options/autofill_options.html:7: <div>
Can this div be skipped?

http://codereview.chromium.org/6034005/diff/8001/chrome/browser/resources/opt...
chrome/browser/resources/options/autofill_options.html:17: <div>
same here?

http://codereview.chromium.org/6034005/diff/8001/chrome/browser/resources/opt...
File chrome/browser/resources/options/autofill_options_list.js (right):

http://codereview.chromium.org/6034005/diff/8001/chrome/browser/resources/opt...
chrome/browser/resources/options/autofill_options_list.js:19: el.dataItem =
entry;
How about storing guid and label simple value properties instead of having the
dataItem indirection?

http://codereview.chromium.org/6034005/diff/8001/chrome/browser/resources/opt...
chrome/browser/resources/options/autofill_options_list.js:80: var item =
this.getListItemByIndex(index);
Shouldn't this be going through the dataModel?

http://codereview.chromium.org/6034005/diff/8001/chrome/browser/resources/opt...
chrome/browser/resources/options/autofill_options_list.js:86: var item =
this.getListItemByIndex(index);
same here

http://codereview.chromium.org/6034005/diff/8001/chrome/browser/resources/opt...
chrome/browser/resources/options/autofill_options_list.js:93: get length() {
What is this used for?

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

http://codereview.chromium.org/6034005/diff/8001/chrome/browser/resources/opt...
chrome/browser/resources/options/options_page.css:374: .list-title {
Maybe you should have used a hx in the markup for this?

http://codereview.chromium.org/6034005/diff/8001/chrome/browser/resources/sha...
File chrome/browser/resources/shared/js/cr/ui/list.js (right):

http://codereview.chromium.org/6034005/diff/8001/chrome/browser/resources/sha...
chrome/browser/resources/shared/js/cr/ui/list.js:306: if (target == this &&
!inViewport(target, e))
In this case I think you can skip this check since we are use the DOM to find
the list item and you already handle the case of no list item being clicked
below.

http://codereview.chromium.org/6034005/diff/8001/chrome/browser/resources/sha...
chrome/browser/resources/shared/js/cr/ui/list.js:312:
this.activateItemAtIndex(index);
Now the question arise if we should allow Enter (and Command+Down on Mac) to
also call activateItem(s)At?

Maybe by calling this something with dblclick in it we can get around the issue.

[csilv, 2010-12-22 20:03:06]

http://codereview.chromium.org/6034005/diff/8001/chrome/browser/dom_ui/option...
File chrome/browser/dom_ui/options/autofill_options_handler.h (right):

http://codereview.chromium.org/6034005/diff/8001/chrome/browser/dom_ui/option...
chrome/browser/dom_ui/options/autofill_options_handler.h:39: // Removes either
an address or a credit card, depending on the type of the
Totally picky, but i'll throw it out.  In some of the comments, you refer to the
argument as a "unique ID", and in other comments you refer to it as a "GUID". 
While noone should be tripped up by this, it might be nice to be consistent.

[James Hawkins, 2010-12-22 21:22:30]

http://codereview.chromium.org/6034005/diff/8001/chrome/browser/dom_ui/option...
File chrome/browser/dom_ui/options/autofill_options_handler.cc (right):

http://codereview.chromium.org/6034005/diff/8001/chrome/browser/dom_ui/option...
chrome/browser/dom_ui/options/autofill_options_handler.cc:191: if
(personal_data_->GetProfileByGUID(guid) != NULL)
On 2010/12/22 18:14:07, arv wrote:
> I find this confusing (and same in RemoveAutoFillProfile). Could it be made
> clearer why a null profile means edit/remove credit card?

I'll add a comment to explain.

http://codereview.chromium.org/6034005/diff/8001/chrome/browser/dom_ui/option...
File chrome/browser/dom_ui/options/autofill_options_handler.h (right):

http://codereview.chromium.org/6034005/diff/8001/chrome/browser/dom_ui/option...
chrome/browser/dom_ui/options/autofill_options_handler.h:39: // Removes either
an address or a credit card, depending on the type of the
On 2010/12/22 20:03:07, csilv wrote:
> Totally picky, but i'll throw it out.  In some of the comments, you refer to
the
> argument as a "unique ID", and in other comments you refer to it as a "GUID". 
> While noone should be tripped up by this, it might be nice to be consistent.

Done. These used to be unique IDs at the model level, but they're GUIDs now and
I forgot to rename them.

http://codereview.chromium.org/6034005/diff/8001/chrome/browser/resources/opt...
File chrome/browser/resources/options/autofill_options.css (right):

http://codereview.chromium.org/6034005/diff/8001/chrome/browser/resources/opt...
chrome/browser/resources/options/autofill_options.css:2: padding-left: 8px;
On 2010/12/22 18:14:07, arv wrote:
> -webkit-padding-start?

Done.

http://codereview.chromium.org/6034005/diff/8001/chrome/browser/resources/opt...
File chrome/browser/resources/options/autofill_options.html (right):

http://codereview.chromium.org/6034005/diff/8001/chrome/browser/resources/opt...
chrome/browser/resources/options/autofill_options.html:7: <div>
On 2010/12/22 18:14:07, arv wrote:
> Can this div be skipped?

No, it's the lower-button div which has a border line at the top.

http://codereview.chromium.org/6034005/diff/8001/chrome/browser/resources/opt...
chrome/browser/resources/options/autofill_options.html:17: <div>
On 2010/12/22 18:14:07, arv wrote:
> same here?

Same as above.

http://codereview.chromium.org/6034005/diff/8001/chrome/browser/resources/opt...
File chrome/browser/resources/options/autofill_options_list.js (right):

http://codereview.chromium.org/6034005/diff/8001/chrome/browser/resources/opt...
chrome/browser/resources/options/autofill_options_list.js:19: el.dataItem =
entry;
On 2010/12/22 18:14:07, arv wrote:
> How about storing guid and label simple value properties instead of having the
> dataItem indirection?

Done.

http://codereview.chromium.org/6034005/diff/8001/chrome/browser/resources/opt...
chrome/browser/resources/options/autofill_options_list.js:80: var item =
this.getListItemByIndex(index);
On 2010/12/22 18:14:07, arv wrote:
> Shouldn't this be going through the dataModel?

Done.

http://codereview.chromium.org/6034005/diff/8001/chrome/browser/resources/opt...
chrome/browser/resources/options/autofill_options_list.js:86: var item =
this.getListItemByIndex(index);
On 2010/12/22 18:14:07, arv wrote:
> same here

Done.

http://codereview.chromium.org/6034005/diff/8001/chrome/browser/resources/opt...
chrome/browser/resources/options/autofill_options_list.js:93: get length() {
On 2010/12/22 18:14:07, arv wrote:
> What is this used for?

Copy/pasted from another implementation. Removed.

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

http://codereview.chromium.org/6034005/diff/8001/chrome/browser/resources/opt...
chrome/browser/resources/options/options_page.css:374: .list-title {
On 2010/12/22 18:14:07, arv wrote:
> Maybe you should have used a hx in the markup for this?

Done.

http://codereview.chromium.org/6034005/diff/8001/chrome/browser/resources/sha...
File chrome/browser/resources/shared/js/cr/ui/list.js (right):

http://codereview.chromium.org/6034005/diff/8001/chrome/browser/resources/sha...
chrome/browser/resources/shared/js/cr/ui/list.js:306: if (target == this &&
!inViewport(target, e))
On 2010/12/22 18:14:07, arv wrote:
> In this case I think you can skip this check since we are use the DOM to find
> the list item and you already handle the case of no list item being clicked
> below.

Done.

http://codereview.chromium.org/6034005/diff/8001/chrome/browser/resources/sha...
chrome/browser/resources/shared/js/cr/ui/list.js:312:
this.activateItemAtIndex(index);
On 2010/12/22 18:14:07, arv wrote:
> Now the question arise if we should allow Enter (and Command+Down on Mac) to
> also call activateItem(s)At?
> 

We should allow Enter to activate the item as well, I just haven't added
handling for it yet.

[csilv, 2010-12-22 21:29:13]

LGTM

[James Hawkins, 2010-12-22 22:11:55]

Erik, I'm assuming you're ok with the general design of this CL, and I can
address any future comments you have in a followup CL.  Going ahead with the
commit.

[arv (Not doing code reviews), 2010-12-22 23:00:35]

http://codereview.chromium.org/6034005/diff/20001/chrome/browser/resources/op...
File chrome/browser/resources/options/autofill_options_list.js (right):

http://codereview.chromium.org/6034005/diff/20001/chrome/browser/resources/op...
chrome/browser/resources/options/autofill_options_list.js:44: get guid() {
No need for getter and setters for these since there are no side effects.

http://codereview.chromium.org/6034005/diff/20001/chrome/browser/resources/op...
chrome/browser/resources/options/autofill_options_list.js:45: return this.guid;
This will not work. It will cause infinite recursion.

[James Hawkins, 2010-12-22 23:13:08]

The changes are in http://codereview.chromium.org/6083006

http://codereview.chromium.org/6034005/diff/20001/chrome/browser/resources/op...
File chrome/browser/resources/options/autofill_options_list.js (right):

http://codereview.chromium.org/6034005/diff/20001/chrome/browser/resources/op...
chrome/browser/resources/options/autofill_options_list.js:44: get guid() {
On 2010/12/22 23:00:35, arv wrote:
> No need for getter and setters for these since there are no side effects.

Done.

http://codereview.chromium.org/6034005/diff/20001/chrome/browser/resources/op...
chrome/browser/resources/options/autofill_options_list.js:45: return this.guid;
On 2010/12/22 23:00:35, arv wrote:
> This will not work. It will cause infinite recursion.

Done.

[Evan Stade, 2011-01-12 00:46:12]

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

http://codereview.chromium.org/6034005/diff/20001/chrome/browser/resources/sh...
chrome/browser/resources/shared/js/cr/ui/list.js:304:
this.activateItemAtIndex(index);
this is causing js errors in lists that don't implement the function. We might
add a stub in List, but currently only Autofill lists use this, and AFAIK other
list implementations don't need it. Can we just move it to the autofill list
class?

[James Hawkins, 2011-01-12 00:54:03]

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

http://codereview.chromium.org/6034005/diff/20001/chrome/browser/resources/sh...
chrome/browser/resources/shared/js/cr/ui/list.js:304:
this.activateItemAtIndex(index);
On 2011/01/12 00:46:12, Evan Stade wrote:
> this is causing js errors in lists that don't implement the function. We might
> add a stub in List, but currently only Autofill lists use this, and AFAIK
other
> list implementations don't need it. Can we just move it to the autofill list
> class?

I guess I forgot to stub it out; I'll add the stub.