Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
11231063:
[views] "Hello, world" autofill imperative API dialog. (Closed)
|
Created:
8 years, 2 months ago by Evan Stade Modified:
8 years, 1 month ago CC:
chromium-reviews, tfarina Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
Description[views] "Hello, world" autofill imperative API dialog.
not hooked up to anything as the API doesn't exist yet.
BUG=157270, 157273
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=164403
Patch Set 1 #
Total comments: 55
Patch Set 2 : ilya review #Patch Set 3 : couple of forgotten nits #
Total comments: 8
Patch Set 4 : another round of review #
Total comments: 8
Patch Set 5 : sky review #Patch Set 6 : . #
Total comments: 2
Patch Set 7 : fix compile I hope #Patch Set 8 : compile #
Messages
Total messages: 9 (0 generated)
Lots of little things: http://codereview.chromium.org/11231063/diff/1/chrome/browser/ui/autofill/aut... File chrome/browser/ui/autofill/autofill_dialog_controller.cc (right): http://codereview.chromium.org/11231063/diff/1/chrome/browser/ui/autofill/aut... chrome/browser/ui/autofill/autofill_dialog_controller.cc:20: return string16(ASCIIToUTF16("Pay")); nit: I think the strings should include some clear nonsense in them, so that we are less likely to forget to update them to be real l10n resources. http://codereview.chromium.org/11231063/diff/1/chrome/browser/ui/autofill/aut... chrome/browser/ui/autofill/autofill_dialog_controller.cc:46: AutofillDialogView* AutofillDialogView::CreateDialog() { nit: This should be annotated with a bug id for implementing the view on all platforms. Also, is there a NOTIMPLEMENTED() macro or something along those lines that we could add here in the meantime? http://codereview.chromium.org/11231063/diff/1/chrome/browser/ui/autofill/aut... File chrome/browser/ui/autofill/autofill_dialog_controller.h (right): http://codereview.chromium.org/11231063/diff/1/chrome/browser/ui/autofill/aut... chrome/browser/ui/autofill/autofill_dialog_controller.h:17: class AutofillDialogView { nit: Please include a comment describing this class, especially since we also have an AutofillPopupView class. http://codereview.chromium.org/11231063/diff/1/chrome/browser/ui/autofill/aut... chrome/browser/ui/autofill/autofill_dialog_controller.h:17: class AutofillDialogView { nit: Please move this class into its own file. http://codereview.chromium.org/11231063/diff/1/chrome/browser/ui/autofill/aut... chrome/browser/ui/autofill/autofill_dialog_controller.h:24: static AutofillDialogView* CreateDialog(AutofillDialogController* controller); nit: Please include a comment describing this function, including a mention of the fact that it's a static factory so that each platform can construct its own flavor of view. http://codereview.chromium.org/11231063/diff/1/chrome/browser/ui/autofill/aut... chrome/browser/ui/autofill/autofill_dialog_controller.h:27: class AutofillDialogController { nit: Please include a brief comment describing this class. http://codereview.chromium.org/11231063/diff/1/chrome/browser/ui/autofill/aut... chrome/browser/ui/autofill/autofill_dialog_controller.h:38: bool ConfirmButtonEnabled(); nit: If these are only called by the view, why not have them simply be defined on the View class? (Warning: I have never quite figured out what the point of the controller in an MVC design is. I might just be showing my ignorance here.) http://codereview.chromium.org/11231063/diff/1/chrome/browser/ui/autofill/aut... chrome/browser/ui/autofill/autofill_dialog_controller.h:38: bool ConfirmButtonEnabled(); nit: These should all be const methods, and the string16 ones should return const string16s. http://codereview.chromium.org/11231063/diff/1/chrome/browser/ui/autofill/aut... chrome/browser/ui/autofill/autofill_dialog_controller.h:41: // indicates that the autofill operation should be aborted. nit: "autofill" -> "Autofill", since this is referring to the feature name, and the feature name is capitalized. http://codereview.chromium.org/11231063/diff/1/chrome/browser/ui/autofill/aut... chrome/browser/ui/autofill/autofill_dialog_controller.h:42: void ViewClosed(bool accepted); nit: This boolean's meaning will not be clear at the call site. Please create a two-valued enumeration that can be passed in instead. http://codereview.chromium.org/11231063/diff/1/chrome/browser/ui/autofill/aut... chrome/browser/ui/autofill/autofill_dialog_controller.h:44: content::WebContents* web_contents() { return contents_; } nit: Can this be constified? http://codereview.chromium.org/11231063/diff/1/chrome/browser/ui/autofill/aut... chrome/browser/ui/autofill/autofill_dialog_controller.h:47: // The WebContents that initiated the autofill action. nit: The user initiates an Autofill action, not the WebContents. Perhaps "The WebContents that contains the form that is being filled"? (Sorry to be super pedantic, but I really did find this comment a little confusing.) http://codereview.chromium.org/11231063/diff/1/chrome/browser/ui/autofill/aut... chrome/browser/ui/autofill/autofill_dialog_controller.h:48: content::WebContents* contents_; nit: content::WebContents* const contents_; http://codereview.chromium.org/11231063/diff/1/chrome/browser/ui/views/autofi... File chrome/browser/ui/views/autofill/autofill_dialog_views.cc (right): http://codereview.chromium.org/11231063/diff/1/chrome/browser/ui/views/autofi... chrome/browser/ui/views/autofill/autofill_dialog_views.cc:20: contents_(NULL) { nit: DCHECK(controller)? http://codereview.chromium.org/11231063/diff/1/chrome/browser/ui/views/autofi... chrome/browser/ui/views/autofill/autofill_dialog_views.cc:29: // care of deleting itself after calling DeleteDelegate(). How does this pass ownership of |contents_|? |contents_| isn't one of the parameters to this method, is it? http://codereview.chromium.org/11231063/diff/1/chrome/browser/ui/views/autofi... chrome/browser/ui/views/autofill/autofill_dialog_views.cc:29: // care of deleting itself after calling DeleteDelegate(). Is DeleteDelegate() guaranteed to be called in all cases? The memory management of all this code seems way too involved :( http://codereview.chromium.org/11231063/diff/1/chrome/browser/ui/views/autofi... chrome/browser/ui/views/autofill/autofill_dialog_views.cc:39: void AutofillDialogViews::DeleteDelegate() { Should window_ be set to |null|? (Perhaps even earlier, as part of WindowClosing()?) http://codereview.chromium.org/11231063/diff/1/chrome/browser/ui/views/autofi... chrome/browser/ui/views/autofill/autofill_dialog_views.cc:76: NOTREACHED(); nit: Please add TODOs to implement this and other methods in this class. http://codereview.chromium.org/11231063/diff/1/chrome/browser/ui/views/autofi... chrome/browser/ui/views/autofill/autofill_dialog_views.cc:81: contents_ = new views::View(); It looks like this can leak if Show() is never called. Is there something I'm missing? http://codereview.chromium.org/11231063/diff/1/chrome/browser/ui/views/autofi... chrome/browser/ui/views/autofill/autofill_dialog_views.cc:82: contents_->AddChildView(new views::Label(ASCIIToUTF16("Hello, world."))); Does this pass ownership of the child view? It doesn't really look to me like it does... http://codereview.chromium.org/11231063/diff/1/chrome/browser/ui/views/autofi... File chrome/browser/ui/views/autofill/autofill_dialog_views.h (right): http://codereview.chromium.org/11231063/diff/1/chrome/browser/ui/views/autofi... chrome/browser/ui/views/autofill/autofill_dialog_views.h:13: class AutofillDialogViews : public AutofillDialogView, nit: I believe this should be named AutofillDialogViewViews. http://codereview.chromium.org/11231063/diff/1/chrome/browser/ui/views/autofi... chrome/browser/ui/views/autofill/autofill_dialog_views.h:38: AutofillDialogController* controller_; nit: AutofillDialogController* const controller_; http://codereview.chromium.org/11231063/diff/1/chrome/browser/ui/views/autofi... chrome/browser/ui/views/autofill/autofill_dialog_views.h:40: ConstrainedWindowViews* window_; nit: All class members should be documented, even if minimally just to reassure the reader that the obvious guess is indeed correct. Please describe this and the other members for this class. http://codereview.chromium.org/11231063/diff/1/chrome/chrome_browser.gypi File chrome/chrome_browser.gypi (right): http://codereview.chromium.org/11231063/diff/1/chrome/chrome_browser.gypi#new... chrome/chrome_browser.gypi:206: 'browser/autocomplete/autocomplete_classifier.cc', nit: Spurious diff?
http://codereview.chromium.org/11231063/diff/1/chrome/browser/ui/autofill/aut... File chrome/browser/ui/autofill/autofill_dialog_controller.cc (right): http://codereview.chromium.org/11231063/diff/1/chrome/browser/ui/autofill/aut... chrome/browser/ui/autofill/autofill_dialog_controller.cc:20: return string16(ASCIIToUTF16("Pay")); On 2012/10/23 00:39:00, Ilya Sherman wrote: > nit: I think the strings should include some clear nonsense in them, so that we > are less likely to forget to update them to be real l10n resources. Done. http://codereview.chromium.org/11231063/diff/1/chrome/browser/ui/autofill/aut... chrome/browser/ui/autofill/autofill_dialog_controller.cc:46: AutofillDialogView* AutofillDialogView::CreateDialog() { On 2012/10/23 00:39:00, Ilya Sherman wrote: > nit: This should be annotated with a bug id for implementing the view on all > platforms. Also, is there a NOTIMPLEMENTED() macro or something along those > lines that we could add here in the meantime? I dislike NOTIMPLEMENTED() because it's easy to be annoying if you put it somewhere that winds up getting called a lot. Safer to avoid it altogether. I'll add the bug ids. http://codereview.chromium.org/11231063/diff/1/chrome/browser/ui/autofill/aut... File chrome/browser/ui/autofill/autofill_dialog_controller.h (right): http://codereview.chromium.org/11231063/diff/1/chrome/browser/ui/autofill/aut... chrome/browser/ui/autofill/autofill_dialog_controller.h:17: class AutofillDialogView { On 2012/10/23 00:39:00, Ilya Sherman wrote: > nit: Please include a comment describing this class, especially since we also > have an AutofillPopupView class. oh yea, forgot class level comments. I'll add all those. http://codereview.chromium.org/11231063/diff/1/chrome/browser/ui/autofill/aut... chrome/browser/ui/autofill/autofill_dialog_controller.h:17: class AutofillDialogView { On 2012/10/23 00:39:00, Ilya Sherman wrote: > nit: Please move this class into its own file. I don't believe it warrants its own file, but done. http://codereview.chromium.org/11231063/diff/1/chrome/browser/ui/autofill/aut... chrome/browser/ui/autofill/autofill_dialog_controller.h:24: static AutofillDialogView* CreateDialog(AutofillDialogController* controller); On 2012/10/23 00:39:00, Ilya Sherman wrote: > nit: Please include a comment describing this function, including a mention of > the fact that it's a static factory so that each platform can construct its own > flavor of view. Done. http://codereview.chromium.org/11231063/diff/1/chrome/browser/ui/autofill/aut... chrome/browser/ui/autofill/autofill_dialog_controller.h:27: class AutofillDialogController { On 2012/10/23 00:39:00, Ilya Sherman wrote: > nit: Please include a brief comment describing this class. Done. http://codereview.chromium.org/11231063/diff/1/chrome/browser/ui/autofill/aut... chrome/browser/ui/autofill/autofill_dialog_controller.h:38: bool ConfirmButtonEnabled(); On 2012/10/23 00:39:00, Ilya Sherman wrote: > nit: These should all be const methods, and the string16 ones should return > const string16s. changed to const methods, but the return values cannot be const refs (and const without ref on an object has no meaning) http://codereview.chromium.org/11231063/diff/1/chrome/browser/ui/autofill/aut... chrome/browser/ui/autofill/autofill_dialog_controller.h:38: bool ConfirmButtonEnabled(); On 2012/10/23 00:39:00, Ilya Sherman wrote: > nit: If these are only called by the view, why not have them simply be defined > on the View class? (Warning: I have never quite figured out what the point of > the controller in an MVC design is. I might just be showing my ignorance here.) the controller contains the logic and state for the dialog. The clearest reason for its separation from the view is that it contains stuff that's shared by all views. If these were in each view class there would be duplication (bad). If we put them in the AutofillDialogView base class, that base class would cease to be pure virtual and we'd likely be dealing with multiple inheritance (also bad). Sharing code via composition is preferable to inheritance. Arguably, the functions which just look up a string from l10n_util don't need to exist. However I expect that ConfirmButtonText will change based on state, so I want that logic in the controller, and the others alongside it for consistency. http://codereview.chromium.org/11231063/diff/1/chrome/browser/ui/autofill/aut... chrome/browser/ui/autofill/autofill_dialog_controller.h:41: // indicates that the autofill operation should be aborted. On 2012/10/23 00:39:00, Ilya Sherman wrote: > nit: "autofill" -> "Autofill", since this is referring to the feature name, and > the feature name is capitalized. Done. http://codereview.chromium.org/11231063/diff/1/chrome/browser/ui/autofill/aut... chrome/browser/ui/autofill/autofill_dialog_controller.h:42: void ViewClosed(bool accepted); On 2012/10/23 00:39:00, Ilya Sherman wrote: > nit: This boolean's meaning will not be clear at the call site. Please create a > two-valued enumeration that can be passed in instead. Done. http://codereview.chromium.org/11231063/diff/1/chrome/browser/ui/autofill/aut... chrome/browser/ui/autofill/autofill_dialog_controller.h:44: content::WebContents* web_contents() { return contents_; } On 2012/10/23 00:39:00, Ilya Sherman wrote: > nit: Can this be constified? Done. http://codereview.chromium.org/11231063/diff/1/chrome/browser/ui/autofill/aut... chrome/browser/ui/autofill/autofill_dialog_controller.h:47: // The WebContents that initiated the autofill action. On 2012/10/23 00:39:00, Ilya Sherman wrote: > nit: The user initiates an Autofill action, not the WebContents. Perhaps "The > WebContents that contains the form that is being filled"? (Sorry to be super > pedantic, but I really did find this comment a little confusing.) well, it's initiated by a javascript call which is not necessarily a result of a user action. http://codereview.chromium.org/11231063/diff/1/chrome/browser/ui/autofill/aut... chrome/browser/ui/autofill/autofill_dialog_controller.h:48: content::WebContents* contents_; On 2012/10/23 00:39:00, Ilya Sherman wrote: > nit: content::WebContents* const contents_; Done. http://codereview.chromium.org/11231063/diff/1/chrome/browser/ui/views/autofi... File chrome/browser/ui/views/autofill/autofill_dialog_views.cc (right): http://codereview.chromium.org/11231063/diff/1/chrome/browser/ui/views/autofi... chrome/browser/ui/views/autofill/autofill_dialog_views.cc:20: contents_(NULL) { On 2012/10/23 00:39:00, Ilya Sherman wrote: > nit: DCHECK(controller)? Done. http://codereview.chromium.org/11231063/diff/1/chrome/browser/ui/views/autofi... chrome/browser/ui/views/autofill/autofill_dialog_views.cc:29: // care of deleting itself after calling DeleteDelegate(). On 2012/10/23 00:39:00, Ilya Sherman wrote: > How does this pass ownership of |contents_|? |contents_| isn't one of the > parameters to this method, is it? because constrainedwindowviews calls GetContentsView() and then owns the result. http://codereview.chromium.org/11231063/diff/1/chrome/browser/ui/views/autofi... chrome/browser/ui/views/autofill/autofill_dialog_views.cc:29: // care of deleting itself after calling DeleteDelegate(). On 2012/10/23 00:39:00, Ilya Sherman wrote: > Is DeleteDelegate() guaranteed to be called in all cases? The memory management > of all this code seems way too involved :( yes it is, and this memory management model is common to all constrained windows. The fact that it's not the most straightforward code ever is why there is documentation here. http://codereview.chromium.org/11231063/diff/1/chrome/browser/ui/views/autofi... chrome/browser/ui/views/autofill/autofill_dialog_views.cc:39: void AutofillDialogViews::DeleteDelegate() { On 2012/10/23 00:39:00, Ilya Sherman wrote: > Should window_ be set to |null|? (Perhaps even earlier, as part of > WindowClosing()?) what is WindowClosing? Actually it seems we don't even need window_, so I've removed it. http://codereview.chromium.org/11231063/diff/1/chrome/browser/ui/views/autofi... chrome/browser/ui/views/autofill/autofill_dialog_views.cc:76: NOTREACHED(); On 2012/10/23 00:39:00, Ilya Sherman wrote: > nit: Please add TODOs to implement this and other methods in this class. I'm not sure a TODO in every function that is not in its final form is very useful. http://codereview.chromium.org/11231063/diff/1/chrome/browser/ui/views/autofi... chrome/browser/ui/views/autofill/autofill_dialog_views.cc:81: contents_ = new views::View(); On 2012/10/23 00:39:00, Ilya Sherman wrote: > It looks like this can leak if Show() is never called. Is there something I'm > missing? This function was supposed to be private and only called from Show(). Fixed. http://codereview.chromium.org/11231063/diff/1/chrome/browser/ui/views/autofi... chrome/browser/ui/views/autofill/autofill_dialog_views.cc:82: contents_->AddChildView(new views::Label(ASCIIToUTF16("Hello, world."))); On 2012/10/23 00:39:00, Ilya Sherman wrote: > Does this pass ownership of the child view? It doesn't really look to me like > it does... yes http://codereview.chromium.org/11231063/diff/1/chrome/browser/ui/views/autofi... File chrome/browser/ui/views/autofill/autofill_dialog_views.h (right): http://codereview.chromium.org/11231063/diff/1/chrome/browser/ui/views/autofi... chrome/browser/ui/views/autofill/autofill_dialog_views.h:13: class AutofillDialogViews : public AutofillDialogView, On 2012/10/23 00:39:00, Ilya Sherman wrote: > nit: I believe this should be named AutofillDialogViewViews. that is one possible name for the class, but not the one I prefer. http://codereview.chromium.org/11231063/diff/1/chrome/browser/ui/views/autofi... chrome/browser/ui/views/autofill/autofill_dialog_views.h:38: AutofillDialogController* controller_; On 2012/10/23 00:39:00, Ilya Sherman wrote: > nit: AutofillDialogController* const controller_; Done. http://codereview.chromium.org/11231063/diff/1/chrome/browser/ui/views/autofi... chrome/browser/ui/views/autofill/autofill_dialog_views.h:40: ConstrainedWindowViews* window_; On 2012/10/23 00:39:00, Ilya Sherman wrote: > nit: All class members should be documented, even if minimally just to reassure > the reader that the obvious guess is indeed correct. Please describe this and > the other members for this class. Done. http://codereview.chromium.org/11231063/diff/1/chrome/chrome_browser.gypi File chrome/chrome_browser.gypi (right): http://codereview.chromium.org/11231063/diff/1/chrome/chrome_browser.gypi#new... chrome/chrome_browser.gypi:206: 'browser/autocomplete/autocomplete_classifier.cc', On 2012/10/23 00:39:00, Ilya Sherman wrote: > nit: Spurious diff? yes, thanks for catching.
LGTM with a few more nits. Thanks. https://chromiumcodereview.appspot.com/11231063/diff/1/chrome/browser/ui/auto... File chrome/browser/ui/autofill/autofill_dialog_controller.h (right): https://chromiumcodereview.appspot.com/11231063/diff/1/chrome/browser/ui/auto... chrome/browser/ui/autofill/autofill_dialog_controller.h:38: bool ConfirmButtonEnabled(); On 2012/10/23 01:52:04, Evan Stade wrote: > On 2012/10/23 00:39:00, Ilya Sherman wrote: > > nit: These should all be const methods, and the string16 ones should return > > const string16s. > > changed to const methods, but the return values cannot be const refs (and const > without ref on an object has no meaning) Sadly, const without a ref does have a meaning. Without const, you can do this: controller.DialogTitle() = "foobar"; // Calls operator= on the result of controller.DialogTitle() This can then result in a typo from "==" to "=" resulting in really weird broken behavior. Thus, const is recommended even though you're returning a copy. https://chromiumcodereview.appspot.com/11231063/diff/1/chrome/browser/ui/auto... chrome/browser/ui/autofill/autofill_dialog_controller.h:47: // The WebContents that initiated the autofill action. On 2012/10/23 01:52:04, Evan Stade wrote: > On 2012/10/23 00:39:00, Ilya Sherman wrote: > > nit: The user initiates an Autofill action, not the WebContents. Perhaps "The > > WebContents that contains the form that is being filled"? (Sorry to be super > > pedantic, but I really did find this comment a little confusing.) > > well, it's initiated by a javascript call which is not necessarily a result of a > user action. Fair enough, but I still think that describing this as initiated *by* the WebContents is weird. https://chromiumcodereview.appspot.com/11231063/diff/1/chrome/browser/ui/view... File chrome/browser/ui/views/autofill/autofill_dialog_views.cc (right): https://chromiumcodereview.appspot.com/11231063/diff/1/chrome/browser/ui/view... chrome/browser/ui/views/autofill/autofill_dialog_views.cc:76: NOTREACHED(); On 2012/10/23 01:52:04, Evan Stade wrote: > On 2012/10/23 00:39:00, Ilya Sherman wrote: > > nit: Please add TODOs to implement this and other methods in this class. > > I'm not sure a TODO in every function that is not in its final form is very > useful. I think it's likely to be useful when some of the functions are fully implemented, whereas others still need to be finished -- especially since the class design will likely evolve somewhat, resulting in some functions that should actually be deleted rather than implemented. Up to you, but I think including some sort of documentation indicating that the code isn't finished is helpful for keeping track of what needs to be done/cleaned up as the code evolves. https://chromiumcodereview.appspot.com/11231063/diff/1/chrome/browser/ui/view... chrome/browser/ui/views/autofill/autofill_dialog_views.cc:81: contents_ = new views::View(); On 2012/10/23 01:52:04, Evan Stade wrote: > On 2012/10/23 00:39:00, Ilya Sherman wrote: > > It looks like this can leak if Show() is never called. Is there something I'm > > missing? > > This function was supposed to be private and only called from Show(). Fixed. Ah, ok, that makes more sense. Since it's only two lines and relies on the rest of Show() to take ownership of the allocated memory, I think it might be simpler to just inline this code in Show(), rather than having the extra function. https://chromiumcodereview.appspot.com/11231063/diff/8009/chrome/browser/ui/a... File chrome/browser/ui/autofill/autofill_dialog_controller.h (right): https://chromiumcodereview.appspot.com/11231063/diff/8009/chrome/browser/ui/a... chrome/browser/ui/autofill/autofill_dialog_controller.h:18: // autofill API to fill out a form. nit: "autofill" -> either "autocomplete" (to match the HTML spec) or "Autofill" (to match the Chrome feature name) https://chromiumcodereview.appspot.com/11231063/diff/8009/chrome/browser/ui/a... File chrome/browser/ui/autofill/autofill_dialog_view.h (right): https://chromiumcodereview.appspot.com/11231063/diff/8009/chrome/browser/ui/a... chrome/browser/ui/autofill/autofill_dialog_view.h:10: // An interface for the dialog that appears when a site initiates an autofill nit: "autofill" -> "Autofill" (throughout the CL, unless there are places where you're referring to something other than the feature name) https://chromiumcodereview.appspot.com/11231063/diff/8009/chrome/browser/ui/v... File chrome/browser/ui/views/autofill/autofill_dialog_views.cc (right): https://chromiumcodereview.appspot.com/11231063/diff/8009/chrome/browser/ui/v... chrome/browser/ui/views/autofill/autofill_dialog_views.cc:29: // Ownership of |contents_| is handed off by this call. |window_| will take nit: Please update this comment not to reference |window_|, since you've removed it. https://chromiumcodereview.appspot.com/11231063/diff/8009/chrome/browser/ui/v... File chrome/browser/ui/views/autofill/autofill_dialog_views.h (right): https://chromiumcodereview.appspot.com/11231063/diff/8009/chrome/browser/ui/v... chrome/browser/ui/views/autofill/autofill_dialog_views.h:43: // The top-level View for the dialog. Owned by |window_|. nit: It looks like |window_| isn't a data member anymore?
thanks Ilya. Scott, when you have a moment, could you give an OWNERs review? (I believe your ownership extends over all the files in this CL :) https://chromiumcodereview.appspot.com/11231063/diff/1/chrome/browser/ui/auto... File chrome/browser/ui/autofill/autofill_dialog_controller.h (right): https://chromiumcodereview.appspot.com/11231063/diff/1/chrome/browser/ui/auto... chrome/browser/ui/autofill/autofill_dialog_controller.h:38: bool ConfirmButtonEnabled(); On 2012/10/23 04:52:29, Ilya Sherman wrote: > On 2012/10/23 01:52:04, Evan Stade wrote: > > On 2012/10/23 00:39:00, Ilya Sherman wrote: > > > nit: These should all be const methods, and the string16 ones should return > > > const string16s. > > > > changed to const methods, but the return values cannot be const refs (and > const > > without ref on an object has no meaning) > > Sadly, const without a ref does have a meaning. Without const, you can do this: > > controller.DialogTitle() = "foobar"; // Calls operator= on the result of > controller.DialogTitle() > > This can then result in a typo from "==" to "=" resulting in really weird broken > behavior. Thus, const is recommended even though you're returning a copy. Sorry, I'm not trying to be difficult, but... if you tried to use controller.DialogTitle() = some_string16 as a truth value, your code would not compile. gcc states: "error: cannot convert 'std::basic_string<short unsigned int, base::string16_char_traits>' to 'bool'" and I strongly suspect other compilers would complain as well. Anyway I don't think we really do this in chrome: I can find 5 instances of a const string return value in autofill/, and 0 instances in the rest of chrome/browser. https://chromiumcodereview.appspot.com/11231063/diff/1/chrome/browser/ui/auto... chrome/browser/ui/autofill/autofill_dialog_controller.h:47: // The WebContents that initiated the autofill action. On 2012/10/23 04:52:29, Ilya Sherman wrote: > On 2012/10/23 01:52:04, Evan Stade wrote: > > On 2012/10/23 00:39:00, Ilya Sherman wrote: > > > nit: The user initiates an Autofill action, not the WebContents. Perhaps > "The > > > WebContents that contains the form that is being filled"? (Sorry to be > super > > > pedantic, but I really did find this comment a little confusing.) > > > > well, it's initiated by a javascript call which is not necessarily a result of > a > > user action. > > Fair enough, but I still think that describing this as initiated *by* the > WebContents is weird. I tried to improve the wording. https://chromiumcodereview.appspot.com/11231063/diff/1/chrome/browser/ui/view... File chrome/browser/ui/views/autofill/autofill_dialog_views.cc (right): https://chromiumcodereview.appspot.com/11231063/diff/1/chrome/browser/ui/view... chrome/browser/ui/views/autofill/autofill_dialog_views.cc:81: contents_ = new views::View(); On 2012/10/23 04:52:29, Ilya Sherman wrote: > On 2012/10/23 01:52:04, Evan Stade wrote: > > On 2012/10/23 00:39:00, Ilya Sherman wrote: > > > It looks like this can leak if Show() is never called. Is there something > I'm > > > missing? > > > > This function was supposed to be private and only called from Show(). Fixed. > > Ah, ok, that makes more sense. Since it's only two lines and relies on the rest > of Show() to take ownership of the allocated memory, I think it might be simpler > to just inline this code in Show(), rather than having the extra function. right now it is two lines. In the future it will be many lines. https://chromiumcodereview.appspot.com/11231063/diff/8009/chrome/browser/ui/a... File chrome/browser/ui/autofill/autofill_dialog_controller.h (right): https://chromiumcodereview.appspot.com/11231063/diff/8009/chrome/browser/ui/a... chrome/browser/ui/autofill/autofill_dialog_controller.h:18: // autofill API to fill out a form. On 2012/10/23 04:52:29, Ilya Sherman wrote: > nit: "autofill" -> either "autocomplete" (to match the HTML spec) or "Autofill" > (to match the Chrome feature name) Done. https://chromiumcodereview.appspot.com/11231063/diff/8009/chrome/browser/ui/a... File chrome/browser/ui/autofill/autofill_dialog_view.h (right): https://chromiumcodereview.appspot.com/11231063/diff/8009/chrome/browser/ui/a... chrome/browser/ui/autofill/autofill_dialog_view.h:10: // An interface for the dialog that appears when a site initiates an autofill On 2012/10/23 04:52:29, Ilya Sherman wrote: > nit: "autofill" -> "Autofill" (throughout the CL, unless there are places where > you're referring to something other than the feature name) Done. https://chromiumcodereview.appspot.com/11231063/diff/8009/chrome/browser/ui/v... File chrome/browser/ui/views/autofill/autofill_dialog_views.cc (right): https://chromiumcodereview.appspot.com/11231063/diff/8009/chrome/browser/ui/v... chrome/browser/ui/views/autofill/autofill_dialog_views.cc:29: // Ownership of |contents_| is handed off by this call. |window_| will take On 2012/10/23 04:52:29, Ilya Sherman wrote: > nit: Please update this comment not to reference |window_|, since you've removed > it. Done. https://chromiumcodereview.appspot.com/11231063/diff/8009/chrome/browser/ui/v... File chrome/browser/ui/views/autofill/autofill_dialog_views.h (right): https://chromiumcodereview.appspot.com/11231063/diff/8009/chrome/browser/ui/v... chrome/browser/ui/views/autofill/autofill_dialog_views.h:43: // The top-level View for the dialog. Owned by |window_|. On 2012/10/23 04:52:29, Ilya Sherman wrote: > nit: It looks like |window_| isn't a data member anymore? Done.
https://chromiumcodereview.appspot.com/11231063/diff/12002/chrome/browser/ui/... File chrome/browser/ui/autofill/autofill_dialog_controller.cc (right): https://chromiumcodereview.appspot.com/11231063/diff/12002/chrome/browser/ui/... chrome/browser/ui/autofill/autofill_dialog_controller.cc:12: content::WebContents* contents) : contents_(contents) {} Since you couldn't fit it all on one line, wrap content_(contents) to a newline. https://chromiumcodereview.appspot.com/11231063/diff/12002/chrome/browser/ui/... File chrome/browser/ui/autofill/autofill_dialog_controller.h (right): https://chromiumcodereview.appspot.com/11231063/diff/12002/chrome/browser/ui/... chrome/browser/ui/autofill/autofill_dialog_controller.h:41: content::WebContents* web_contents() const { return contents_; } const content::WebContents* web_contents() const or content::WebContents* web_contents() https://chromiumcodereview.appspot.com/11231063/diff/12002/chrome/browser/ui/... File chrome/browser/ui/views/autofill/autofill_dialog_views.cc (right): https://chromiumcodereview.appspot.com/11231063/diff/12002/chrome/browser/ui/... chrome/browser/ui/views/autofill/autofill_dialog_views.cc:24: AutofillDialogViews::~AutofillDialogViews() {} If this is deleted with the constrained window open won't the ConstrainedWindowViews still have a reference to 'this' by way of the WidgetDelegate? https://chromiumcodereview.appspot.com/11231063/diff/12002/chrome/browser/ui/... File chrome/browser/ui/views/autofill/autofill_dialog_views.h (right): https://chromiumcodereview.appspot.com/11231063/diff/12002/chrome/browser/ui/... chrome/browser/ui/views/autofill/autofill_dialog_views.h:45: }; DISALLOW_...
https://chromiumcodereview.appspot.com/11231063/diff/12002/chrome/browser/ui/... File chrome/browser/ui/autofill/autofill_dialog_controller.cc (right): https://chromiumcodereview.appspot.com/11231063/diff/12002/chrome/browser/ui/... chrome/browser/ui/autofill/autofill_dialog_controller.cc:12: content::WebContents* contents) : contents_(contents) {} On 2012/10/23 19:53:54, sky wrote: > Since you couldn't fit it all on one line, wrap content_(contents) to a newline. Done. https://chromiumcodereview.appspot.com/11231063/diff/12002/chrome/browser/ui/... File chrome/browser/ui/autofill/autofill_dialog_controller.h (right): https://chromiumcodereview.appspot.com/11231063/diff/12002/chrome/browser/ui/... chrome/browser/ui/autofill/autofill_dialog_controller.h:41: content::WebContents* web_contents() const { return contents_; } On 2012/10/23 19:53:54, sky wrote: > const content::WebContents* web_contents() const or > content::WebContents* web_contents() Done. https://chromiumcodereview.appspot.com/11231063/diff/12002/chrome/browser/ui/... File chrome/browser/ui/views/autofill/autofill_dialog_views.cc (right): https://chromiumcodereview.appspot.com/11231063/diff/12002/chrome/browser/ui/... chrome/browser/ui/views/autofill/autofill_dialog_views.cc:24: AutofillDialogViews::~AutofillDialogViews() {} On 2012/10/23 19:53:54, sky wrote: > If this is deleted with the constrained window open won't the > ConstrainedWindowViews still have a reference to 'this' by way of the > WidgetDelegate? yea, but the plan is not to let that happen (i.e. it would be an error to get here if the constrained window hadn't already started closing). I've added code to catch this problem. https://chromiumcodereview.appspot.com/11231063/diff/12002/chrome/browser/ui/... File chrome/browser/ui/views/autofill/autofill_dialog_views.h (right): https://chromiumcodereview.appspot.com/11231063/diff/12002/chrome/browser/ui/... chrome/browser/ui/views/autofill/autofill_dialog_views.h:45: }; On 2012/10/23 19:53:54, sky wrote: > DISALLOW_... Done.
LGTM https://chromiumcodereview.appspot.com/11231063/diff/2010/chrome/browser/ui/a... File chrome/browser/ui/autofill/autofill_dialog_view.h (right): https://chromiumcodereview.appspot.com/11231063/diff/2010/chrome/browser/ui/a... chrome/browser/ui/autofill/autofill_dialog_view.h:21: static AutofillDialogView* CreateDialog(AutofillDialogController* controller); I think this conflicts with a win32 function. Maybe name it Create.
https://chromiumcodereview.appspot.com/11231063/diff/2010/chrome/browser/ui/a... File chrome/browser/ui/autofill/autofill_dialog_view.h (right): https://chromiumcodereview.appspot.com/11231063/diff/2010/chrome/browser/ui/a... chrome/browser/ui/autofill/autofill_dialog_view.h:21: static AutofillDialogView* CreateDialog(AutofillDialogController* controller); On 2012/10/23 23:13:12, sky wrote: > I think this conflicts with a win32 function. Maybe name it Create. seems so. Will do. |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
