Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
554081:
First revision of the AutoFill settings dialog.... (Closed)
|
Created:
10 years, 11 months ago by GeorgeY Modified:
9 years, 6 months ago CC:
chromium-reviews, ben+cc_chromium.org Base URL:
svn://chrome-svn/chrome/trunk/src/ Visibility:
Public. |
DescriptionFirst revision of the AutoFill settings dialog.
What works:
1. Layout (for the most part)
2. Editing and saving the items
What does not
1. Scroll
2. Delete/Add buttons
3. some layout quirks
4. collapsible editable sets.
BUG=33026
TEST=none for now.
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=37357
Patch Set 1 #
Total comments: 51
Patch Set 2 : '' #
Total comments: 44
Patch Set 3 : '' #
Total comments: 22
Patch Set 4 : '' #Patch Set 5 : '' #
Messages
Total messages: 11 (0 generated)
I am continuing to work on it, but I decided to send intermediate as it already large.
On 2010/01/26 19:10:12, georgey wrote: > I am continuing to work on it, but I decided to send intermediate as it already > large. Can you send us a few screenshots please? Thanks.
http://codereview.chromium.org/554081/diff/1/3 File chrome/app/resources/locale_settings.grd (right): http://codereview.chromium.org/554081/diff/1/3#newcode299 chrome/app/resources/locale_settings.grd:299: <message name="IDS_AUTOFILL_PROFILES_DIALOG_WIDTH_CHARS" use_name_for_id="true"> There's already an IDS_AUTOFILL_DIALOG_WIDTH_CHARS and IDS_AUTOFILL_DIALOG_HEIGHT_LINES. If you want to use different values than gtk, you should use the grd logic to change those values. http://codereview.chromium.org/554081/diff/1/8 File chrome/browser/autofill/autofill_dialog.cc (right): http://codereview.chromium.org/554081/diff/1/8#newcode15 chrome/browser/autofill/autofill_dialog.cc:15: AutoFillProfilesView::Show(observer, profiles, credit_cards); This will make the call for Mac too, which will fail to compile/link. This is a platform-agnostic file. You need to add your own implementation of ShowAutoFillDialog in autofill_dialog_win.cc and remove the defined(OS_WIN) once you do. http://codereview.chromium.org/554081/diff/1/4 File chrome/browser/autofill/autofill_dialog.h (right): http://codereview.chromium.org/554081/diff/1/4#newcode10 chrome/browser/autofill/autofill_dialog.h:10: #include "app/gfx/native_widget_types.h" You don't need this include in this file. http://codereview.chromium.org/554081/diff/1/10 File chrome/browser/autofill/autofill_profiles_view.cc (right): http://codereview.chromium.org/554081/diff/1/10#newcode26 chrome/browser/autofill/autofill_profiles_view.cc:26: const int kDialogPadding = 7; Please add a comment for this constant. http://codereview.chromium.org/554081/diff/1/10#newcode100 chrome/browser/autofill/autofill_profiles_view.cc:100: void AutoFillProfilesView::ViewHierarchyChanged(bool is_add, views::View* parent, nit: line length http://codereview.chromium.org/554081/diff/1/10#newcode143 chrome/browser/autofill/autofill_profiles_view.cc:143: std::vector<EditableSetInfo>::iterator it; Move the iterator inside the for loop. http://codereview.chromium.org/554081/diff/1/10#newcode163 chrome/browser/autofill/autofill_profiles_view.cc:163: nit: Remove the blank line. http://codereview.chromium.org/554081/diff/1/10#newcode322 chrome/browser/autofill/autofill_profiles_view.cc:322: } else { What is this empty else for? http://codereview.chromium.org/554081/diff/1/10#newcode331 chrome/browser/autofill/autofill_profiles_view.cc:331: layout->SetInsets(5, 5, 5, 5); Move the 5 to a documented constant. http://codereview.chromium.org/554081/diff/1/9 File chrome/browser/autofill/autofill_profiles_view.h (right): http://codereview.chromium.org/554081/diff/1/9#newcode5 chrome/browser/autofill/autofill_profiles_view.h:5: #ifndef CHROME_BROWSER_AUTOFILL_AUTOFILL_PROFILES_VIEW_H_ This file should be named autofill_dialog_win.h, and the implementation (including ShowAutoFillDialog) should be in autofill_dialog_win.cc. See autofill_dialog_gtk.cc. You're confusing the name of the dialog. It's not an AutoFill Profiles dialog; rather, it's just an AutoFillDialog, which can edit addresses (profiles) and credit cards. http://codereview.chromium.org/554081/diff/1/9#newcode17 chrome/browser/autofill/autofill_profiles_view.h:17: class ScrollView; nit: No indentation for forward declarations. http://codereview.chromium.org/554081/diff/1/9#newcode62 chrome/browser/autofill/autofill_profiles_view.h:62: CreditCard *credit_card; Addresses and credit cards are not linked. You should be able to add and delete addresses and credit cards separately. http://codereview.chromium.org/554081/diff/1/9#newcode65 chrome/browser/autofill/autofill_profiles_view.h:65: explicit EditableSetInfo(const AutoFillProfile* input_address, bool opened) The explicit keyword is only for constructors with one parameter, or one parameter and one or more parameters with default values. http://codereview.chromium.org/554081/diff/1/9#newcode77 chrome/browser/autofill/autofill_profiles_view.h:77: void Clear() { This looks like you're leaking address and credit_card if you don't call Clear(). You should make them scoped_ptrs. http://codereview.chromium.org/554081/diff/1/9#newcode95 chrome/browser/autofill/autofill_profiles_view.h:95: explicit AutoFillProfilesView(AutoFillDialogObserver* observer, Remove explicit. http://codereview.chromium.org/554081/diff/1/9#newcode101 chrome/browser/autofill/autofill_profiles_view.h:101: class PhoneSubView : public views::View { Comment this class. http://codereview.chromium.org/554081/diff/1/9#newcode103 chrome/browser/autofill/autofill_profiles_view.h:103: explicit PhoneSubView(views::Label* label, Remove explicit. http://codereview.chromium.org/554081/diff/1/9#newcode194 chrome/browser/autofill/autofill_profiles_view.h:194: static const int four_column_ccnumber_expiration_cvc_ = 5; Add DISALLOW_COPY_AND_ASSIGN. http://codereview.chromium.org/554081/diff/1/9#newcode201 chrome/browser/autofill/autofill_profiles_view.h:201: explicit ScrollViewContents(std::vector<EditableSetInfo>* profiles, Remove explicit. http://codereview.chromium.org/554081/diff/1/9#newcode221 chrome/browser/autofill/autofill_profiles_view.h:221: static int line_height_; Add DISALLOW_COPY_AND_ASSIGN. http://codereview.chromium.org/554081/diff/1/9#newcode226 chrome/browser/autofill/autofill_profiles_view.h:226: explicit AutoFillScrollView(std::vector<EditableSetInfo>* profiles, Remove explicit. http://codereview.chromium.org/554081/diff/1/9#newcode238 chrome/browser/autofill/autofill_profiles_view.h:238: DISALLOW_EVIL_CONSTRUCTORS(AutoFillScrollView); DISALLOW_COPY_AND_ASSIGN http://codereview.chromium.org/554081/diff/1/12 File chrome/browser/autofill/credit_card.h (right): http://codereview.chromium.org/554081/diff/1/12#newcode19 chrome/browser/autofill/credit_card.h:19: // For use in STL containers. (needed for vector<>::resize()) You can remove this comment. The comment above will suffice. http://codereview.chromium.org/554081/diff/1/5 File chrome/browser/views/options/content_page_view.cc (right): http://codereview.chromium.org/554081/diff/1/5#newcode112 chrome/browser/views/options/content_page_view.cc:112: /* TODO(georgey) : finish it :) You should move these changes to a separate CL and only send it when everything is ready.
I have a first pass of nits/readability stuff that I'll get you to correct first then I'll take another pass and verify the logic. http://codereview.chromium.org/554081/diff/2006/2013 File chrome/browser/autofill/autofill_dialog.cc (right): http://codereview.chromium.org/554081/diff/2006/2013#newcode7 chrome/browser/autofill/autofill_dialog.cc:7: #include "views/window/window.h" why? http://codereview.chromium.org/554081/diff/2006/2009 File chrome/browser/autofill/autofill_dialog.h (right): http://codereview.chromium.org/554081/diff/2006/2009#newcode10 chrome/browser/autofill/autofill_dialog.h:10: #include "app/gfx/native_widget_types.h" why? http://codereview.chromium.org/554081/diff/2006/2015 File chrome/browser/autofill/autofill_profiles_view.cc (right): http://codereview.chromium.org/554081/diff/2006/2015#newcode56 chrome/browser/autofill/autofill_profiles_view.cc:56: // AutoFillProfilesView, public colons : on the end of all of these lines... http://codereview.chromium.org/554081/diff/2006/2015#newcode57 chrome/browser/autofill/autofill_profiles_view.cc:57: AutoFillProfilesView::AutoFillProfilesView(AutoFillDialogObserver* observer, this first param should wrap to the next line http://codereview.chromium.org/554081/diff/2006/2015#newcode164 chrome/browser/autofill/autofill_profiles_view.cc:164: } NOTIMPLEMENTED(); http://codereview.chromium.org/554081/diff/2006/2015#newcode175 chrome/browser/autofill/autofill_profiles_view.cc:175: if (!instance_->window()->IsVisible()) { no braces needed for this one. single line contents. http://codereview.chromium.org/554081/diff/2006/2015#newcode636 chrome/browser/autofill/autofill_profiles_view.cc:636: int AutoFillProfilesView::ScrollViewContents::line_height_ = 0; these sort of constant/static definitions should go at the start of the file. http://codereview.chromium.org/554081/diff/2006/2014 File chrome/browser/autofill/autofill_profiles_view.h (right): http://codereview.chromium.org/554081/diff/2006/2014#newcode33 chrome/browser/autofill/autofill_profiles_view.h:33: // views::View methods. : instead of . http://codereview.chromium.org/554081/diff/2006/2014#newcode52 chrome/browser/autofill/autofill_profiles_view.h:52: // views::ButtonListener methods : on the end http://codereview.chromium.org/554081/diff/2006/2014#newcode58 chrome/browser/autofill/autofill_profiles_view.h:58: const std::vector<CreditCard*>& credit_cards); Move your public API above the interface implementations and View overrides. Make sure the order in the .cc matches the .h once you've done that. http://codereview.chromium.org/554081/diff/2006/2014#newcode97 chrome/browser/autofill/autofill_profiles_view.h:97: const std::vector<CreditCard*>& credit_cards); no need for explicit here http://codereview.chromium.org/554081/diff/2006/2014#newcode106 chrome/browser/autofill/autofill_profiles_view.h:106: views::Textfield* text_phone); no need for explicit here http://codereview.chromium.org/554081/diff/2006/2014#newcode110 chrome/browser/autofill/autofill_profiles_view.h:110: virtual void ViewHierarchyChanged(bool is_add, views::View* parent, protected: http://codereview.chromium.org/554081/diff/2006/2014#newcode116 chrome/browser/autofill/autofill_profiles_view.h:116: views::Textfield* text_phone_; DISALLOW_COPY_AND_ASSIGN... http://codereview.chromium.org/554081/diff/2006/2014#newcode132 chrome/browser/autofill/autofill_profiles_view.h:132: views::View* child); Move the views::View overrides to the end and make ViewHierarchyChanged be protected:. Actually, all your interface implementations can be protected too. You may as well just make all of this protected: http://codereview.chromium.org/554081/diff/2006/2014#newcode195 chrome/browser/autofill/autofill_profiles_view.h:195: }; DISALLOW_COPY_AND_ASSIGN... http://codereview.chromium.org/554081/diff/2006/2014#newcode202 chrome/browser/autofill/autofill_profiles_view.h:202: std::vector<EditableSetInfo>* credit_cards); no need for explicit here http://codereview.chromium.org/554081/diff/2006/2014#newcode207 chrome/browser/autofill/autofill_profiles_view.h:207: bool is_horizontal, bool is_positive); Again, just make all of this protected: http://codereview.chromium.org/554081/diff/2006/2014#newcode221 chrome/browser/autofill/autofill_profiles_view.h:221: static int line_height_; DISALLOW_COPY_AND_ASSIGN... http://codereview.chromium.org/554081/diff/2006/2014#newcode227 chrome/browser/autofill/autofill_profiles_view.h:227: std::vector<EditableSetInfo>* credit_cards); no need for explicit here. http://codereview.chromium.org/554081/diff/2006/2014#newcode238 chrome/browser/autofill/autofill_profiles_view.h:238: DISALLOW_EVIL_CONSTRUCTORS(AutoFillScrollView); should be DISALLOW_COPY_AND_ASSIGN http://codereview.chromium.org/554081/diff/2006/2010 File chrome/browser/views/options/content_page_view.cc (right): http://codereview.chromium.org/554081/diff/2006/2010#newcode114 chrome/browser/views/options/content_page_view.cc:114: */ Can you disable the button too so we don't get bug reports about a button that does nothing? :-)
Fixed or responded to all comments. Will re-upload when all compiles... http://codereview.chromium.org/554081/diff/1/3 File chrome/app/resources/locale_settings.grd (right): http://codereview.chromium.org/554081/diff/1/3#newcode299 chrome/app/resources/locale_settings.grd:299: <message name="IDS_AUTOFILL_PROFILES_DIALOG_WIDTH_CHARS" use_name_for_id="true"> On 2010/01/26 19:35:28, James Hawkins wrote: > There's already an IDS_AUTOFILL_DIALOG_WIDTH_CHARS and > IDS_AUTOFILL_DIALOG_HEIGHT_LINES. If you want to use different values than gtk, > you should use the grd logic to change those values. I want use the ones that you use, I think I added it before you added yours, reverted. http://codereview.chromium.org/554081/diff/1/8 File chrome/browser/autofill/autofill_dialog.cc (right): http://codereview.chromium.org/554081/diff/1/8#newcode15 chrome/browser/autofill/autofill_dialog.cc:15: AutoFillProfilesView::Show(observer, profiles, credit_cards); On 2010/01/26 19:35:28, James Hawkins wrote: > This will make the call for Mac too, which will fail to compile/link. This is a > platform-agnostic file. You need to add your own implementation of > ShowAutoFillDialog in autofill_dialog_win.cc and remove the defined(OS_WIN) once > you do. added _win, renamed my view file to _win http://codereview.chromium.org/554081/diff/1/4 File chrome/browser/autofill/autofill_dialog.h (right): http://codereview.chromium.org/554081/diff/1/4#newcode10 chrome/browser/autofill/autofill_dialog.h:10: #include "app/gfx/native_widget_types.h" On 2010/01/26 19:35:28, James Hawkins wrote: > You don't need this include in this file. Done. http://codereview.chromium.org/554081/diff/1/10 File chrome/browser/autofill/autofill_profiles_view.cc (right): http://codereview.chromium.org/554081/diff/1/10#newcode26 chrome/browser/autofill/autofill_profiles_view.cc:26: const int kDialogPadding = 7; On 2010/01/26 19:35:28, James Hawkins wrote: > Please add a comment for this constant. Done. http://codereview.chromium.org/554081/diff/1/10#newcode100 chrome/browser/autofill/autofill_profiles_view.cc:100: void AutoFillProfilesView::ViewHierarchyChanged(bool is_add, views::View* parent, On 2010/01/26 19:35:28, James Hawkins wrote: > nit: line length Done. http://codereview.chromium.org/554081/diff/1/10#newcode143 chrome/browser/autofill/autofill_profiles_view.cc:143: std::vector<EditableSetInfo>::iterator it; On 2010/01/26 19:35:28, James Hawkins wrote: > Move the iterator inside the for loop. It is used twice, the loop after uses it as well http://codereview.chromium.org/554081/diff/1/10#newcode163 chrome/browser/autofill/autofill_profiles_view.cc:163: On 2010/01/26 19:35:28, James Hawkins wrote: > nit: Remove the blank line. Done. http://codereview.chromium.org/554081/diff/1/10#newcode322 chrome/browser/autofill/autofill_profiles_view.cc:322: } else { On 2010/01/26 19:35:28, James Hawkins wrote: > What is this empty else for? Remnant of old breakpoint. removed. http://codereview.chromium.org/554081/diff/1/10#newcode331 chrome/browser/autofill/autofill_profiles_view.cc:331: layout->SetInsets(5, 5, 5, 5); On 2010/01/26 19:35:28, James Hawkins wrote: > Move the 5 to a documented constant. Done. http://codereview.chromium.org/554081/diff/1/9 File chrome/browser/autofill/autofill_profiles_view.h (right): http://codereview.chromium.org/554081/diff/1/9#newcode17 chrome/browser/autofill/autofill_profiles_view.h:17: class ScrollView; On 2010/01/26 19:35:28, James Hawkins wrote: > nit: No indentation for forward declarations. Done. http://codereview.chromium.org/554081/diff/1/9#newcode62 chrome/browser/autofill/autofill_profiles_view.h:62: CreditCard *credit_card; On 2010/01/26 19:35:28, James Hawkins wrote: > Addresses and credit cards are not linked. You should be able to add and delete > addresses and credit cards separately. They are not linked here, as you see it is more union, than anything else. Which I am probably going to change it to. http://codereview.chromium.org/554081/diff/1/9#newcode65 chrome/browser/autofill/autofill_profiles_view.h:65: explicit EditableSetInfo(const AutoFillProfile* input_address, bool opened) On 2010/01/26 19:35:28, James Hawkins wrote: > The explicit keyword is only for constructors with one parameter, or one > parameter and one or more parameters with default values. Here are two constructors, that differ only in the (possibly related) pointer parameter, so I think explicit here is apt. http://codereview.chromium.org/554081/diff/1/9#newcode77 chrome/browser/autofill/autofill_profiles_view.h:77: void Clear() { On 2010/01/26 19:35:28, James Hawkins wrote: > This looks like you're leaking address and credit_card if you don't call > Clear(). You should make them scoped_ptrs. I do not leak it, but I rearranged it some for more clear code. http://codereview.chromium.org/554081/diff/1/9#newcode95 chrome/browser/autofill/autofill_profiles_view.h:95: explicit AutoFillProfilesView(AutoFillDialogObserver* observer, On 2010/01/26 19:35:28, James Hawkins wrote: > Remove explicit. Done. http://codereview.chromium.org/554081/diff/1/9#newcode101 chrome/browser/autofill/autofill_profiles_view.h:101: class PhoneSubView : public views::View { On 2010/01/26 19:35:28, James Hawkins wrote: > Comment this class. Done. http://codereview.chromium.org/554081/diff/1/9#newcode103 chrome/browser/autofill/autofill_profiles_view.h:103: explicit PhoneSubView(views::Label* label, On 2010/01/26 19:35:28, James Hawkins wrote: > Remove explicit. Done. http://codereview.chromium.org/554081/diff/1/9#newcode194 chrome/browser/autofill/autofill_profiles_view.h:194: static const int four_column_ccnumber_expiration_cvc_ = 5; On 2010/01/26 19:35:28, James Hawkins wrote: > Add DISALLOW_COPY_AND_ASSIGN. Done. http://codereview.chromium.org/554081/diff/1/9#newcode201 chrome/browser/autofill/autofill_profiles_view.h:201: explicit ScrollViewContents(std::vector<EditableSetInfo>* profiles, On 2010/01/26 19:35:28, James Hawkins wrote: > Remove explicit. Done. http://codereview.chromium.org/554081/diff/1/9#newcode221 chrome/browser/autofill/autofill_profiles_view.h:221: static int line_height_; On 2010/01/26 19:35:28, James Hawkins wrote: > Add DISALLOW_COPY_AND_ASSIGN. Done. http://codereview.chromium.org/554081/diff/1/9#newcode226 chrome/browser/autofill/autofill_profiles_view.h:226: explicit AutoFillScrollView(std::vector<EditableSetInfo>* profiles, On 2010/01/26 19:35:28, James Hawkins wrote: > Remove explicit. Done. http://codereview.chromium.org/554081/diff/1/9#newcode238 chrome/browser/autofill/autofill_profiles_view.h:238: DISALLOW_EVIL_CONSTRUCTORS(AutoFillScrollView); On 2010/01/26 19:35:28, James Hawkins wrote: > DISALLOW_COPY_AND_ASSIGN Done. http://codereview.chromium.org/554081/diff/1/12 File chrome/browser/autofill/credit_card.h (right): http://codereview.chromium.org/554081/diff/1/12#newcode19 chrome/browser/autofill/credit_card.h:19: // For use in STL containers. (needed for vector<>::resize()) On 2010/01/26 19:35:28, James Hawkins wrote: > You can remove this comment. The comment above will suffice. Done. http://codereview.chromium.org/554081/diff/1/5 File chrome/browser/views/options/content_page_view.cc (right): http://codereview.chromium.org/554081/diff/1/5#newcode112 chrome/browser/views/options/content_page_view.cc:112: /* TODO(georgey) : finish it :) On 2010/01/26 19:35:28, James Hawkins wrote: > You should move these changes to a separate CL and only send it when everything > is ready. Done. http://codereview.chromium.org/554081/diff/2006/2013 File chrome/browser/autofill/autofill_dialog.cc (right): http://codereview.chromium.org/554081/diff/2006/2013#newcode7 chrome/browser/autofill/autofill_dialog.cc:7: #include "views/window/window.h" On 2010/01/26 20:29:27, Ben Goodger wrote: > why? Remnant of earlier test, removed. http://codereview.chromium.org/554081/diff/2006/2009 File chrome/browser/autofill/autofill_dialog.h (right): http://codereview.chromium.org/554081/diff/2006/2009#newcode10 chrome/browser/autofill/autofill_dialog.h:10: #include "app/gfx/native_widget_types.h" On 2010/01/26 20:29:27, Ben Goodger wrote: > why? Sorry, remnant of earlier test. Removed. http://codereview.chromium.org/554081/diff/2006/2015 File chrome/browser/autofill/autofill_profiles_view.cc (right): http://codereview.chromium.org/554081/diff/2006/2015#newcode56 chrome/browser/autofill/autofill_profiles_view.cc:56: // AutoFillProfilesView, public On 2010/01/26 20:29:27, Ben Goodger wrote: > colons : on the end of all of these lines... Done everywhere in this file and the header http://codereview.chromium.org/554081/diff/2006/2015#newcode57 chrome/browser/autofill/autofill_profiles_view.cc:57: AutoFillProfilesView::AutoFillProfilesView(AutoFillDialogObserver* observer, On 2010/01/26 20:29:27, Ben Goodger wrote: > this first param should wrap to the next line Done. http://codereview.chromium.org/554081/diff/2006/2015#newcode164 chrome/browser/autofill/autofill_profiles_view.cc:164: } On 2010/01/26 20:29:27, Ben Goodger wrote: > NOTIMPLEMENTED(); Done, here there and everywhere ;) http://codereview.chromium.org/554081/diff/2006/2015#newcode175 chrome/browser/autofill/autofill_profiles_view.cc:175: if (!instance_->window()->IsVisible()) { On 2010/01/26 20:29:27, Ben Goodger wrote: > no braces needed for this one. single line contents. Done. http://codereview.chromium.org/554081/diff/2006/2015#newcode636 chrome/browser/autofill/autofill_profiles_view.cc:636: int AutoFillProfilesView::ScrollViewContents::line_height_ = 0; On 2010/01/26 20:29:27, Ben Goodger wrote: > these sort of constant/static definitions should go at the start of the file. Done for these one, what about arrays that build the dependency of text fields on AutoFill properties? Should I move them as well? http://codereview.chromium.org/554081/diff/2006/2014 File chrome/browser/autofill/autofill_profiles_view.h (right): http://codereview.chromium.org/554081/diff/2006/2014#newcode33 chrome/browser/autofill/autofill_profiles_view.h:33: // views::View methods. On 2010/01/26 20:29:27, Ben Goodger wrote: > : instead of . Done. http://codereview.chromium.org/554081/diff/2006/2014#newcode52 chrome/browser/autofill/autofill_profiles_view.h:52: // views::ButtonListener methods On 2010/01/26 20:29:27, Ben Goodger wrote: > : on the end Done. http://codereview.chromium.org/554081/diff/2006/2014#newcode58 chrome/browser/autofill/autofill_profiles_view.h:58: const std::vector<CreditCard*>& credit_cards); On 2010/01/26 20:29:27, Ben Goodger wrote: > Move your public API above the interface implementations and View overrides. > Make sure the order in the .cc matches the .h once you've done that. Done. http://codereview.chromium.org/554081/diff/2006/2014#newcode97 chrome/browser/autofill/autofill_profiles_view.h:97: const std::vector<CreditCard*>& credit_cards); On 2010/01/26 20:29:27, Ben Goodger wrote: > no need for explicit here Done. http://codereview.chromium.org/554081/diff/2006/2014#newcode106 chrome/browser/autofill/autofill_profiles_view.h:106: views::Textfield* text_phone); On 2010/01/26 20:29:27, Ben Goodger wrote: > no need for explicit here Done. http://codereview.chromium.org/554081/diff/2006/2014#newcode110 chrome/browser/autofill/autofill_profiles_view.h:110: virtual void ViewHierarchyChanged(bool is_add, views::View* parent, On 2010/01/26 20:29:27, Ben Goodger wrote: > protected: Done. http://codereview.chromium.org/554081/diff/2006/2014#newcode116 chrome/browser/autofill/autofill_profiles_view.h:116: views::Textfield* text_phone_; On 2010/01/26 20:29:27, Ben Goodger wrote: > DISALLOW_COPY_AND_ASSIGN... Done. http://codereview.chromium.org/554081/diff/2006/2014#newcode132 chrome/browser/autofill/autofill_profiles_view.h:132: views::View* child); On 2010/01/26 20:29:27, Ben Goodger wrote: > Move the views::View overrides to the end and make ViewHierarchyChanged be > protected:. Actually, all your interface implementations can be protected too. > You may as well just make all of this protected: Done. http://codereview.chromium.org/554081/diff/2006/2014#newcode195 chrome/browser/autofill/autofill_profiles_view.h:195: }; On 2010/01/26 20:29:27, Ben Goodger wrote: > DISALLOW_COPY_AND_ASSIGN... Done. http://codereview.chromium.org/554081/diff/2006/2014#newcode202 chrome/browser/autofill/autofill_profiles_view.h:202: std::vector<EditableSetInfo>* credit_cards); On 2010/01/26 20:29:27, Ben Goodger wrote: > no need for explicit here Done. http://codereview.chromium.org/554081/diff/2006/2014#newcode207 chrome/browser/autofill/autofill_profiles_view.h:207: bool is_horizontal, bool is_positive); On 2010/01/26 20:29:27, Ben Goodger wrote: > Again, just make all of this protected: Done. http://codereview.chromium.org/554081/diff/2006/2014#newcode221 chrome/browser/autofill/autofill_profiles_view.h:221: static int line_height_; On 2010/01/26 20:29:27, Ben Goodger wrote: > DISALLOW_COPY_AND_ASSIGN... Done. http://codereview.chromium.org/554081/diff/2006/2014#newcode227 chrome/browser/autofill/autofill_profiles_view.h:227: std::vector<EditableSetInfo>* credit_cards); On 2010/01/26 20:29:27, Ben Goodger wrote: > no need for explicit here. Done. http://codereview.chromium.org/554081/diff/2006/2014#newcode238 chrome/browser/autofill/autofill_profiles_view.h:238: DISALLOW_EVIL_CONSTRUCTORS(AutoFillScrollView); On 2010/01/26 20:29:27, Ben Goodger wrote: > should be DISALLOW_COPY_AND_ASSIGN Done. http://codereview.chromium.org/554081/diff/2006/2010 File chrome/browser/views/options/content_page_view.cc (right): http://codereview.chromium.org/554081/diff/2006/2010#newcode114 chrome/browser/views/options/content_page_view.cc:114: */ On 2010/01/26 20:29:27, Ben Goodger wrote: > Can you disable the button too so we don't get bug reports about a button that > does nothing? :-) Done.
http://codereview.chromium.org/554081/diff/1/10 File chrome/browser/autofill/autofill_profiles_view.cc (right): http://codereview.chromium.org/554081/diff/1/10#newcode143 chrome/browser/autofill/autofill_profiles_view.cc:143: std::vector<EditableSetInfo>::iterator it; On 2010/01/26 23:25:54, georgey wrote: > On 2010/01/26 19:35:28, James Hawkins wrote: > > Move the iterator inside the for loop. > > It is used twice, the loop after uses it as well I see that. You should still move the declaration to each of the for loops. You should always limit the scopes of your variables, esp. for loop iterators. http://codereview.chromium.org/554081/diff/1/9 File chrome/browser/autofill/autofill_profiles_view.h (right): http://codereview.chromium.org/554081/diff/1/9#newcode62 chrome/browser/autofill/autofill_profiles_view.h:62: CreditCard *credit_card; On 2010/01/26 23:25:54, georgey wrote: > On 2010/01/26 19:35:28, James Hawkins wrote: > > Addresses and credit cards are not linked. You should be able to add and > delete > > addresses and credit cards separately. > > They are not linked here, as you see it is more union, than anything else. > Which I am probably going to change it to. Can you explain what you mean by union? What I mean by 'not linked' is that there should be a list of addresses, and a list of credit cards, neither of which corresponds to the other. The UI should reflect that. http://codereview.chromium.org/554081/diff/1/9#newcode65 chrome/browser/autofill/autofill_profiles_view.h:65: explicit EditableSetInfo(const AutoFillProfile* input_address, bool opened) On 2010/01/26 23:25:54, georgey wrote: > On 2010/01/26 19:35:28, James Hawkins wrote: > > The explicit keyword is only for constructors with one parameter, or one > > parameter and one or more parameters with default values. > > Here are two constructors, that differ only in the (possibly related) pointer > parameter, so I think explicit here is apt. That's not the purpose of the explicit keyword. explicit means that no automatic conversion will happen. I'm not sure what you're trying to protect against here. http://codereview.chromium.org/554081/diff/1/9#newcode77 chrome/browser/autofill/autofill_profiles_view.h:77: void Clear() { On 2010/01/26 23:25:54, georgey wrote: > On 2010/01/26 19:35:28, James Hawkins wrote: > > This looks like you're leaking address and credit_card if you don't call > > Clear(). You should make them scoped_ptrs. > > I do not leak it, but I rearranged it some for more clear code. You should make the pointers scoped_ptr regardless, because the code can be changed in the future to not call Clear() (for whatever reason), and we'd unknowingly be leaking the data.
http://codereview.chromium.org/554081/diff/5002/29 File chrome/browser/autofill/autofill_profiles_view_win.cc (right): http://codereview.chromium.org/554081/diff/5002/29#newcode38 chrome/browser/autofill/autofill_profiles_view_win.cc:38: gfx::NativeTheme::instance()->GetThemeColorWithDefault( indent 4 spaces instead of 2. http://codereview.chromium.org/554081/diff/5002/29#newcode261 chrome/browser/autofill/autofill_profiles_view_win.cc:261: { AutoFillProfilesView::EditableSetViewContents::TEXT_MIDDLE_NAME, is none of this sort of mapping share-able with Linux? It seems even if the view types are different (views vs. gtk) that these indices etc could be shared in a common location. http://codereview.chromium.org/554081/diff/5002/29#newcode408 chrome/browser/autofill/autofill_profiles_view_win.cc:408: void AutoFillProfilesView::EditableSetViewContents::DisplayAddressFields( I would call these "InitAddressFields" etc. "Display" makes it sound like you're making them visible. http://codereview.chromium.org/554081/diff/5002/29#newcode794 chrome/browser/autofill/autofill_profiles_view_win.cc:794: // views::ButtonListener implementations Are these TBD? http://codereview.chromium.org/554081/diff/5002/28 File chrome/browser/autofill/autofill_profiles_view_win.h (right): http://codereview.chromium.org/554081/diff/5002/28#newcode61 chrome/browser/autofill/autofill_profiles_view_win.h:61: struct EditableSetInfo { You should briefly note what all this is... i.e. what is this data structure for and how does it relate to how this dialog manipulates user data? In fact, there are a lot of classes here... it'd be nice if somewhere in this file you gave a brief overview of how they all relate. http://codereview.chromium.org/554081/diff/5002/28#newcode69 chrome/browser/autofill/autofill_profiles_view_win.h:69: explicit EditableSetInfo(const AutoFillProfile* input_address, bool opened) no need for explicit on either of these ctors http://codereview.chromium.org/554081/diff/5002/28#newcode81 chrome/browser/autofill/autofill_profiles_view_win.h:81: void AddClicked(bool address); documentation needed. http://codereview.chromium.org/554081/diff/5002/28#newcode107 chrome/browser/autofill/autofill_profiles_view_win.h:107: protected: private: ? http://codereview.chromium.org/554081/diff/5002/28#newcode112 chrome/browser/autofill/autofill_profiles_view_win.h:112: DISALLOW_COPY_AND_ASSIGN(PhoneSubView); new line before DISALLOW_... http://codereview.chromium.org/554081/diff/5002/28#newcode249 chrome/browser/autofill/autofill_profiles_view_win.h:249: AutoFillScrollView *scroll_view_; * always goes with the type for the above and below lines.
http://codereview.chromium.org/554081/diff/5002/28 File chrome/browser/autofill/autofill_profiles_view_win.h (right): http://codereview.chromium.org/554081/diff/5002/28#newcode10 chrome/browser/autofill/autofill_profiles_view_win.h:10: #include "chrome/browser/autofill/autofill_profile.h" autofill_dialog.h before autofill_profile.h
http://codereview.chromium.org/554081/diff/5002/29 File chrome/browser/autofill/autofill_profiles_view_win.cc (right): http://codereview.chromium.org/554081/diff/5002/29#newcode38 chrome/browser/autofill/autofill_profiles_view_win.cc:38: gfx::NativeTheme::instance()->GetThemeColorWithDefault( On 2010/01/27 00:03:57, Ben Goodger wrote: > indent 4 spaces instead of 2. Done. http://codereview.chromium.org/554081/diff/5002/29#newcode261 chrome/browser/autofill/autofill_profiles_view_win.cc:261: { AutoFillProfilesView::EditableSetViewContents::TEXT_MIDDLE_NAME, On 2010/01/27 00:03:57, Ben Goodger wrote: > is none of this sort of mapping share-able with Linux? > > It seems even if the view types are different (views vs. gtk) that these indices > etc could be shared in a common location. In GTK case (autofill_dialog_gtk.cc) these items are named, not indexes in an array. I did it this way to automate same tasks on windows as much as possible :) http://codereview.chromium.org/554081/diff/5002/29#newcode408 chrome/browser/autofill/autofill_profiles_view_win.cc:408: void AutoFillProfilesView::EditableSetViewContents::DisplayAddressFields( On 2010/01/27 00:03:57, Ben Goodger wrote: > I would call these "InitAddressFields" etc. "Display" makes it sound like you're > making them visible. Done. http://codereview.chromium.org/554081/diff/5002/29#newcode794 chrome/browser/autofill/autofill_profiles_view_win.cc:794: // views::ButtonListener implementations On 2010/01/27 00:03:57, Ben Goodger wrote: > Are these TBD? Forgot to remove comment. Done. http://codereview.chromium.org/554081/diff/5002/28 File chrome/browser/autofill/autofill_profiles_view_win.h (right): http://codereview.chromium.org/554081/diff/5002/28#newcode10 chrome/browser/autofill/autofill_profiles_view_win.h:10: #include "chrome/browser/autofill/autofill_profile.h" On 2010/01/27 00:06:23, James Hawkins wrote: > autofill_dialog.h before autofill_profile.h Done. http://codereview.chromium.org/554081/diff/5002/28#newcode61 chrome/browser/autofill/autofill_profiles_view_win.h:61: struct EditableSetInfo { On 2010/01/27 00:03:57, Ben Goodger wrote: > You should briefly note what all this is... i.e. what is this data structure for > and how does it relate to how this dialog manipulates user data? > > In fact, there are a lot of classes here... it'd be nice if somewhere in this > file you gave a brief overview of how they all relate. Done. http://codereview.chromium.org/554081/diff/5002/28#newcode69 chrome/browser/autofill/autofill_profiles_view_win.h:69: explicit EditableSetInfo(const AutoFillProfile* input_address, bool opened) On 2010/01/27 00:03:57, Ben Goodger wrote: > no need for explicit on either of these ctors Did it when was not sure if AutoFillProfile and CreditCard are related types. It is always a good idea to mark two constructors "explicit" if they differ only by one, possibly implicitly conversible parameter :), but now I see that they completely separate - removed. http://codereview.chromium.org/554081/diff/5002/28#newcode81 chrome/browser/autofill/autofill_profiles_view_win.h:81: void AddClicked(bool address); On 2010/01/27 00:03:57, Ben Goodger wrote: > documentation needed. Done. http://codereview.chromium.org/554081/diff/5002/28#newcode107 chrome/browser/autofill/autofill_profiles_view_win.h:107: protected: On 2010/01/27 00:03:57, Ben Goodger wrote: > private: ? overtyped "protected" too much :). Fixed. http://codereview.chromium.org/554081/diff/5002/28#newcode112 chrome/browser/autofill/autofill_profiles_view_win.h:112: DISALLOW_COPY_AND_ASSIGN(PhoneSubView); On 2010/01/27 00:03:57, Ben Goodger wrote: > new line before DISALLOW_... Done. http://codereview.chromium.org/554081/diff/5002/28#newcode249 chrome/browser/autofill/autofill_profiles_view_win.h:249: AutoFillScrollView *scroll_view_; On 2010/01/27 00:03:57, Ben Goodger wrote: > * always goes with the type for the above and below lines. Old habits :). Fixed.
LGTM. On Tue, Jan 26, 2010 at 5:12 PM, <georgey@chromium.org> wrote: > > http://codereview.chromium.org/554081/diff/5002/29 > File chrome/browser/autofill/autofill_profiles_view_win.cc (right): > > http://codereview.chromium.org/554081/diff/5002/29#newcode38 > chrome/browser/autofill/autofill_profiles_view_win.cc:38: > gfx::NativeTheme::instance()->GetThemeColorWithDefault( > On 2010/01/27 00:03:57, Ben Goodger wrote: >> >> indent 4 spaces instead of 2. > > Done. > > http://codereview.chromium.org/554081/diff/5002/29#newcode261 > chrome/browser/autofill/autofill_profiles_view_win.cc:261: { > AutoFillProfilesView::EditableSetViewContents::TEXT_MIDDLE_NAME, > On 2010/01/27 00:03:57, Ben Goodger wrote: >> >> is none of this sort of mapping share-able with Linux? > >> It seems even if the view types are different (views vs. gtk) that > > these indices >> >> etc could be shared in a common location. > > In GTK case (autofill_dialog_gtk.cc) these items are named, > not indexes in an array. I did it this way to automate same tasks on > windows as much as possible :) > > http://codereview.chromium.org/554081/diff/5002/29#newcode408 > chrome/browser/autofill/autofill_profiles_view_win.cc:408: void > AutoFillProfilesView::EditableSetViewContents::DisplayAddressFields( > On 2010/01/27 00:03:57, Ben Goodger wrote: >> >> I would call these "InitAddressFields" etc. "Display" makes it sound > > like you're >> >> making them visible. > > Done. > > http://codereview.chromium.org/554081/diff/5002/29#newcode794 > chrome/browser/autofill/autofill_profiles_view_win.cc:794: // > views::ButtonListener implementations > On 2010/01/27 00:03:57, Ben Goodger wrote: >> >> Are these TBD? > > Forgot to remove comment. Done. > > http://codereview.chromium.org/554081/diff/5002/28 > File chrome/browser/autofill/autofill_profiles_view_win.h (right): > > http://codereview.chromium.org/554081/diff/5002/28#newcode10 > chrome/browser/autofill/autofill_profiles_view_win.h:10: #include > "chrome/browser/autofill/autofill_profile.h" > On 2010/01/27 00:06:23, James Hawkins wrote: >> >> autofill_dialog.h before autofill_profile.h > > Done. > > http://codereview.chromium.org/554081/diff/5002/28#newcode61 > chrome/browser/autofill/autofill_profiles_view_win.h:61: struct > EditableSetInfo { > On 2010/01/27 00:03:57, Ben Goodger wrote: >> >> You should briefly note what all this is... i.e. what is this data > > structure for >> >> and how does it relate to how this dialog manipulates user data? > >> In fact, there are a lot of classes here... it'd be nice if somewhere > > in this >> >> file you gave a brief overview of how they all relate. > > Done. > > http://codereview.chromium.org/554081/diff/5002/28#newcode69 > chrome/browser/autofill/autofill_profiles_view_win.h:69: explicit > EditableSetInfo(const AutoFillProfile* input_address, bool opened) > On 2010/01/27 00:03:57, Ben Goodger wrote: >> >> no need for explicit on either of these ctors > > Did it when was not sure if AutoFillProfile and CreditCard are related > types. It is always a good idea to mark two constructors "explicit" if > they differ only by one, possibly implicitly conversible parameter :), > but now I see that they completely separate - removed. > > http://codereview.chromium.org/554081/diff/5002/28#newcode81 > chrome/browser/autofill/autofill_profiles_view_win.h:81: void > AddClicked(bool address); > On 2010/01/27 00:03:57, Ben Goodger wrote: >> >> documentation needed. > > Done. > > http://codereview.chromium.org/554081/diff/5002/28#newcode107 > chrome/browser/autofill/autofill_profiles_view_win.h:107: protected: > On 2010/01/27 00:03:57, Ben Goodger wrote: >> >> private: ? > > overtyped "protected" too much :). Fixed. > > http://codereview.chromium.org/554081/diff/5002/28#newcode112 > chrome/browser/autofill/autofill_profiles_view_win.h:112: > DISALLOW_COPY_AND_ASSIGN(PhoneSubView); > On 2010/01/27 00:03:57, Ben Goodger wrote: >> >> new line before DISALLOW_... > > Done. > > http://codereview.chromium.org/554081/diff/5002/28#newcode249 > chrome/browser/autofill/autofill_profiles_view_win.h:249: > AutoFillScrollView *scroll_view_; > On 2010/01/27 00:03:57, Ben Goodger wrote: >> >> * always goes with the type for the above and below lines. > > Old habits :). Fixed. > > http://codereview.chromium.org/554081 >
LGTM |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
