Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
6286092:
Refactor AutocompletePopupContentView so that it can be extended to create (Closed)
|
Created:
9 years, 10 months ago by varunjain Modified:
9 years, 7 months ago CC:
chromium-reviews Visibility:
Public. |
DescriptionRefactor AutocompletePopupContentView so that it can be extended to create
popup for touch. There are mainly two refactoring tasks in this CL:
1. Move class definitions from .cc file to .h file so they can be used elsewhere
2. Refactor out some code parts of AutocompletePopupContentView that deal with
drawing individual autocomplete results.
BUG=none
TEST=none
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=75157
Patch Set 1 #Patch Set 2 : modified as discussed with reviewer #Patch Set 3 : minor changes #
Total comments: 8
Patch Set 4 : addressed reviewer comments #
Total comments: 26
Patch Set 5 : addressed reviewer comments. #Patch Set 6 : refactored result view in separate file #Patch Set 7 : minor changes #
Total comments: 4
Patch Set 8 : addressed minor comments #
Total comments: 2
Patch Set 9 : refactored result_view_model into seperate file. Also addressed reviewer comment #
Total comments: 4
Patch Set 10 : modified according to comments #Patch Set 11 : fix merge conflicts #
Messages
Total messages: 18 (0 generated)
My apologies for not putting comments I made over chat. Please include the person who knows this code as well. I guess it's either ben or pkasting, but couldn't tell from log because it's moved... http://codereview.chromium.org/6286092/diff/1005/chrome/browser/ui/views/auto... File chrome/browser/ui/views/autocomplete/autocomplete_popup_contents_view.h (right): http://codereview.chromium.org/6286092/diff/1005/chrome/browser/ui/views/auto... chrome/browser/ui/views/autocomplete/autocomplete_popup_contents_view.h:52: }; Move these enum into AutocompleteResultView http://codereview.chromium.org/6286092/diff/1005/chrome/browser/ui/views/auto... chrome/browser/ui/views/autocomplete/autocomplete_popup_contents_view.h:63: }; // namespace no anonymous namespace in header. Move this into .cc if this is for implementation, or move to protected in AutocompleteResultView if this is to share with subclass. http://codereview.chromium.org/6286092/diff/1005/chrome/browser/ui/views/auto... chrome/browser/ui/views/autocomplete/autocomplete_popup_contents_view.h:80: class AutocompleteResultView : public views::View { I thought we're going to have separate file for this?
http://codereview.chromium.org/6286092/diff/1005/chrome/browser/ui/views/auto... File chrome/browser/ui/views/autocomplete/autocomplete_popup_contents_view.h (right): http://codereview.chromium.org/6286092/diff/1005/chrome/browser/ui/views/auto... chrome/browser/ui/views/autocomplete/autocomplete_popup_contents_view.h:52: }; On 2011/02/09 19:18:37, oshima wrote: > Move these enum into AutocompleteResultView Done. http://codereview.chromium.org/6286092/diff/1005/chrome/browser/ui/views/auto... chrome/browser/ui/views/autocomplete/autocomplete_popup_contents_view.h:63: }; // namespace On 2011/02/09 19:18:37, oshima wrote: > no anonymous namespace in header. Move this into .cc if this is for > implementation, or move to protected in AutocompleteResultView if this is to > share with subclass. Done. http://codereview.chromium.org/6286092/diff/1005/chrome/browser/ui/views/auto... chrome/browser/ui/views/autocomplete/autocomplete_popup_contents_view.h:80: class AutocompleteResultView : public views::View { On 2011/02/09 19:18:37, oshima wrote: > I thought we're going to have separate file for this? I thought it would be easier for reviewing the cc file if I extract this out in a new file in a separate CL. I have added a todo for that.
http://codereview.chromium.org/6286092/diff/1005/chrome/browser/ui/views/auto... File chrome/browser/ui/views/autocomplete/autocomplete_popup_contents_view.h (right): http://codereview.chromium.org/6286092/diff/1005/chrome/browser/ui/views/auto... chrome/browser/ui/views/autocomplete/autocomplete_popup_contents_view.h:80: class AutocompleteResultView : public views::View { On 2011/02/14 20:48:15, varunjain wrote: > On 2011/02/09 19:18:37, oshima wrote: > > I thought we're going to have separate file for this? > > I thought it would be easier for reviewing the cc file if I extract this out in > a new file in a separate CL. I have added a todo for that. If we're going to have separate file, there is no benefit to put this definition in this header now. It'll also make it easy to diff potential changes that is necessary to subclass your own.
http://codereview.chromium.org/6286092/diff/1005/chrome/browser/ui/views/auto... File chrome/browser/ui/views/autocomplete/autocomplete_popup_contents_view.h (right): http://codereview.chromium.org/6286092/diff/1005/chrome/browser/ui/views/auto... chrome/browser/ui/views/autocomplete/autocomplete_popup_contents_view.h:80: class AutocompleteResultView : public views::View { On 2011/02/15 00:09:41, oshima wrote: > On 2011/02/14 20:48:15, varunjain wrote: > > On 2011/02/09 19:18:37, oshima wrote: > > > I thought we're going to have separate file for this? > > > > I thought it would be easier for reviewing the cc file if I extract this out > in > > a new file in a separate CL. I have added a todo for that. > > If we're going to have separate file, there is no benefit to put this definition > in this header now. It'll also make it easy to diff potential changes that > is necessary to subclass your own. I need it out of the cc file for sure to be able to subclass it. The question is whether to put it here or a separate file. I picked here because that way, the definition can stay in its current place and need not be moved to separate file as well resulting in smaller CL. Also seemed like moving to separate file is an independent unit of change that can be handled in a separate CL. This CL is more about refactoring out the code for painting of individual autocomplete results so I can extend it. That said, I dont really have strong feelings about this issue... so I'll move it to separate file if you revert back with another comment :)
On 2011/02/15 16:18:37, varunjain wrote: > http://codereview.chromium.org/6286092/diff/1005/chrome/browser/ui/views/auto... > File chrome/browser/ui/views/autocomplete/autocomplete_popup_contents_view.h > (right): > > http://codereview.chromium.org/6286092/diff/1005/chrome/browser/ui/views/auto... > chrome/browser/ui/views/autocomplete/autocomplete_popup_contents_view.h:80: > class AutocompleteResultView : public views::View { > On 2011/02/15 00:09:41, oshima wrote: > > On 2011/02/14 20:48:15, varunjain wrote: > > > On 2011/02/09 19:18:37, oshima wrote: > > > > I thought we're going to have separate file for this? > > > > > > I thought it would be easier for reviewing the cc file if I extract this out > > in > > > a new file in a separate CL. I have added a todo for that. > > > > If we're going to have separate file, there is no benefit to put this > definition > > in this header now. It'll also make it easy to diff potential changes that > > is necessary to subclass your own. > > I need it out of the cc file for sure to be able to subclass it. The question is > whether to put it here or a separate file. I picked here because that way, the > definition can stay in its current place and need not be moved to separate file > as well resulting in smaller CL. > independent unit of change that can be handled in a separate CL. This CL is more > about refactoring out the code for painting of individual autocomplete results > so I can extend it. That said, I dont really have strong feelings about this > issue... so I'll move it to separate file if you revert back with another > comment :) I still think this class should be in separate file. If you put this class in this header, all .cc|.h that include this header will also import this class definition. This class should be "private" to AutocompletePopupContentsView (or package protected in Java) and user of AutocompletePopupContentsView shouldn't see the class.
http://codereview.chromium.org/6286092/diff/9001/chrome/browser/ui/views/auto... File chrome/browser/ui/views/autocomplete/autocomplete_popup_contents_view.cc (right): http://codereview.chromium.org/6286092/diff/9001/chrome/browser/ui/views/auto... chrome/browser/ui/views/autocomplete/autocomplete_popup_contents_view.cc:12: #include "chrome/browser/autocomplete/autocomplete_match.h" Nit: This can go away http://codereview.chromium.org/6286092/diff/9001/chrome/browser/ui/views/auto... chrome/browser/ui/views/autocomplete/autocomplete_popup_contents_view.cc:392: AutocompleteResultView::ResultViewState AutocompleteResultView::GetState() Nit: Wrap between the return type and the function name instead of splitting off "const". http://codereview.chromium.org/6286092/diff/9001/chrome/browser/ui/views/auto... chrome/browser/ui/views/autocomplete/autocomplete_popup_contents_view.cc:961: int result_size = model_->result().size(); Nit: Don't pull this temp out of the loop definition http://codereview.chromium.org/6286092/diff/9001/chrome/browser/ui/views/auto... chrome/browser/ui/views/autocomplete/autocomplete_popup_contents_view.cc:962: for(int i = 0;i < result_size && i < child_count(); ++i) { Nit: Space after "for" and ";", make |i| a size_t, and don't check against child_count() (instead, put a DCHECK_GE(static_cast<size_t>(child_count()), model_->result().size()); atop the function) http://codereview.chromium.org/6286092/diff/9001/chrome/browser/ui/views/auto... chrome/browser/ui/views/autocomplete/autocomplete_popup_contents_view.cc:963: AutocompleteResultView* result_view = Nit: Just combine these two lines, and no need to cast: popup_height += GetChildViewAt(i)->GetPreferredSize().height(); http://codereview.chromium.org/6286092/diff/9001/chrome/browser/ui/views/auto... chrome/browser/ui/views/autocomplete/autocomplete_popup_contents_view.cc:967: if (opt_in_view_) Nit: How about just: return popup_height + (opt_in_view_ ? opt_in_view_->GetPreferredSize().height() : 0); http://codereview.chromium.org/6286092/diff/9001/chrome/browser/ui/views/auto... File chrome/browser/ui/views/autocomplete/autocomplete_popup_contents_view.h (right): http://codereview.chromium.org/6286092/diff/9001/chrome/browser/ui/views/auto... chrome/browser/ui/views/autocomplete/autocomplete_popup_contents_view.h:93: static const int kIconVerticalPadding = 2; Don't define these here. Declare here, define them in the .cc file. Nit: Just name these kMinimum{Icon,Text}VerticalPadding and use a single comment above both. http://codereview.chromium.org/6286092/diff/9001/chrome/browser/ui/views/auto... chrome/browser/ui/views/autocomplete/autocomplete_popup_contents_view.h:100: // Paints a AutocompleteMatch |match| at a specified position |x| on |canvas|. Nit: Just remove this comment, it doesn't say anything that isn't already obvious. http://codereview.chromium.org/6286092/diff/9001/chrome/browser/ui/views/auto... chrome/browser/ui/views/autocomplete/autocomplete_popup_contents_view.h:105: int icon_vertical_padding_; Why do we have both constants and member variables? We can use the constants directly, we don't need these. http://codereview.chromium.org/6286092/diff/9001/chrome/browser/ui/views/auto... chrome/browser/ui/views/autocomplete/autocomplete_popup_contents_view.h:242: // Aggregates the height of all child AutocompleteResultView to calculate the Nit: This comment lies. How about: // Calculates the height needed to show all the results in the model.
http://codereview.chromium.org/6286092/diff/9001/chrome/browser/ui/views/auto... File chrome/browser/ui/views/autocomplete/autocomplete_popup_contents_view.cc (right): http://codereview.chromium.org/6286092/diff/9001/chrome/browser/ui/views/auto... chrome/browser/ui/views/autocomplete/autocomplete_popup_contents_view.cc:12: #include "chrome/browser/autocomplete/autocomplete_match.h" On 2011/02/15 19:10:19, Peter Kasting wrote: > Nit: This can go away Done. http://codereview.chromium.org/6286092/diff/9001/chrome/browser/ui/views/auto... chrome/browser/ui/views/autocomplete/autocomplete_popup_contents_view.cc:392: AutocompleteResultView::ResultViewState AutocompleteResultView::GetState() On 2011/02/15 19:10:19, Peter Kasting wrote: > Nit: Wrap between the return type and the function name instead of splitting off > "const". Done. http://codereview.chromium.org/6286092/diff/9001/chrome/browser/ui/views/auto... chrome/browser/ui/views/autocomplete/autocomplete_popup_contents_view.cc:961: int result_size = model_->result().size(); On 2011/02/15 19:10:19, Peter Kasting wrote: > Nit: Don't pull this temp out of the loop definition Done. http://codereview.chromium.org/6286092/diff/9001/chrome/browser/ui/views/auto... chrome/browser/ui/views/autocomplete/autocomplete_popup_contents_view.cc:962: for(int i = 0;i < result_size && i < child_count(); ++i) { On 2011/02/15 19:10:19, Peter Kasting wrote: > Nit: Space after "for" and ";", make |i| a size_t, and don't check against > child_count() (instead, put a DCHECK_GE(static_cast<size_t>(child_count()), > model_->result().size()); atop the function) Done. http://codereview.chromium.org/6286092/diff/9001/chrome/browser/ui/views/auto... chrome/browser/ui/views/autocomplete/autocomplete_popup_contents_view.cc:963: AutocompleteResultView* result_view = On 2011/02/15 19:10:19, Peter Kasting wrote: > Nit: Just combine these two lines, and no need to cast: > > popup_height += GetChildViewAt(i)->GetPreferredSize().height(); Done. http://codereview.chromium.org/6286092/diff/9001/chrome/browser/ui/views/auto... chrome/browser/ui/views/autocomplete/autocomplete_popup_contents_view.cc:967: if (opt_in_view_) On 2011/02/15 19:10:19, Peter Kasting wrote: > Nit: How about just: > > return popup_height + > (opt_in_view_ ? opt_in_view_->GetPreferredSize().height() : 0); Done. http://codereview.chromium.org/6286092/diff/9001/chrome/browser/ui/views/auto... File chrome/browser/ui/views/autocomplete/autocomplete_popup_contents_view.h (right): http://codereview.chromium.org/6286092/diff/9001/chrome/browser/ui/views/auto... chrome/browser/ui/views/autocomplete/autocomplete_popup_contents_view.h:93: static const int kIconVerticalPadding = 2; On 2011/02/15 19:10:19, Peter Kasting wrote: > Don't define these here. Declare here, define them in the .cc file. > > Nit: Just name these kMinimum{Icon,Text}VerticalPadding and use a single comment > above both. Done. http://codereview.chromium.org/6286092/diff/9001/chrome/browser/ui/views/auto... chrome/browser/ui/views/autocomplete/autocomplete_popup_contents_view.h:100: // Paints a AutocompleteMatch |match| at a specified position |x| on |canvas|. On 2011/02/15 19:10:19, Peter Kasting wrote: > Nit: Just remove this comment, it doesn't say anything that isn't already > obvious. Done. http://codereview.chromium.org/6286092/diff/9001/chrome/browser/ui/views/auto... chrome/browser/ui/views/autocomplete/autocomplete_popup_contents_view.h:105: int icon_vertical_padding_; On 2011/02/15 19:10:19, Peter Kasting wrote: > Why do we have both constants and member variables? We can use the constants > directly, we don't need these. The padding will vary for the touch version that I plan to subclass. Hence the member variables. http://codereview.chromium.org/6286092/diff/9001/chrome/browser/ui/views/auto... chrome/browser/ui/views/autocomplete/autocomplete_popup_contents_view.h:242: // Aggregates the height of all child AutocompleteResultView to calculate the On 2011/02/15 19:10:19, Peter Kasting wrote: > Nit: This comment lies. How about: > > // Calculates the height needed to show all the results in the model. Done.
http://codereview.chromium.org/6286092/diff/9001/chrome/browser/ui/views/auto... File chrome/browser/ui/views/autocomplete/autocomplete_popup_contents_view.cc (right): http://codereview.chromium.org/6286092/diff/9001/chrome/browser/ui/views/auto... chrome/browser/ui/views/autocomplete/autocomplete_popup_contents_view.cc:962: for(int i = 0;i < result_size && i < child_count(); ++i) { On 2011/02/15 22:55:03, varunjain wrote: > On 2011/02/15 19:10:19, Peter Kasting wrote: > > Nit: Space after "for" and ";", make |i| a size_t, and don't check against > > child_count() (instead, put a DCHECK_GE(static_cast<size_t>(child_count()), > > model_->result().size()); atop the function) > > Done. You only addressed the first comment, none of the rest. http://codereview.chromium.org/6286092/diff/9001/chrome/browser/ui/views/auto... File chrome/browser/ui/views/autocomplete/autocomplete_popup_contents_view.h (right): http://codereview.chromium.org/6286092/diff/9001/chrome/browser/ui/views/auto... chrome/browser/ui/views/autocomplete/autocomplete_popup_contents_view.h:93: static const int kIconVerticalPadding = 2; On 2011/02/15 22:55:03, varunjain wrote: > On 2011/02/15 19:10:19, Peter Kasting wrote: > > Don't define these here. Declare here, define them in the .cc file. > > > > Nit: Just name these kMinimum{Icon,Text}VerticalPadding and use a single > comment > > above both. > > Done. You only addressed the first comment, not the second. http://codereview.chromium.org/6286092/diff/9001/chrome/browser/ui/views/auto... chrome/browser/ui/views/autocomplete/autocomplete_popup_contents_view.h:105: int icon_vertical_padding_; On 2011/02/15 22:55:03, varunjain wrote: > On 2011/02/15 19:10:19, Peter Kasting wrote: > > Why do we have both constants and member variables? We can use the constants > > directly, we don't need these. > > The padding will vary for the touch version that I plan to subclass. Hence the > member variables. Then do we need public constants? Can't these constants instead be local to the file or constructor?
On 2011/02/15 18:17:01, oshima wrote: > On 2011/02/15 16:18:37, varunjain wrote: > > > http://codereview.chromium.org/6286092/diff/1005/chrome/browser/ui/views/auto... > > File chrome/browser/ui/views/autocomplete/autocomplete_popup_contents_view.h > > (right): > > > > > http://codereview.chromium.org/6286092/diff/1005/chrome/browser/ui/views/auto... > > chrome/browser/ui/views/autocomplete/autocomplete_popup_contents_view.h:80: > > class AutocompleteResultView : public views::View { > > On 2011/02/15 00:09:41, oshima wrote: > > > On 2011/02/14 20:48:15, varunjain wrote: > > > > On 2011/02/09 19:18:37, oshima wrote: > > > > > I thought we're going to have separate file for this? > > > > > > > > I thought it would be easier for reviewing the cc file if I extract this > out > > > in > > > > a new file in a separate CL. I have added a todo for that. > > > > > > If we're going to have separate file, there is no benefit to put this > > definition > > > in this header now. It'll also make it easy to diff potential changes that > > > is necessary to subclass your own. > > > > I need it out of the cc file for sure to be able to subclass it. The question > is > > whether to put it here or a separate file. I picked here because that way, the > > definition can stay in its current place and need not be moved to separate > file > > as well resulting in smaller CL. > > independent unit of change that can be handled in a separate CL. This CL is > more > > about refactoring out the code for painting of individual autocomplete results > > so I can extend it. That said, I dont really have strong feelings about this > > issue... so I'll move it to separate file if you revert back with another > > comment :) > > I still think this class should be in separate file. > If you put this class in this header, all .cc|.h that include this header > will also import this class definition. This class should be "private" to > AutocompletePopupContentsView (or package protected in Java) > and user of AutocompletePopupContentsView shouldn't see the class. done
http://codereview.chromium.org/6286092/diff/9001/chrome/browser/ui/views/auto... File chrome/browser/ui/views/autocomplete/autocomplete_popup_contents_view.cc (right): http://codereview.chromium.org/6286092/diff/9001/chrome/browser/ui/views/auto... chrome/browser/ui/views/autocomplete/autocomplete_popup_contents_view.cc:962: for(int i = 0;i < result_size && i < child_count(); ++i) { On 2011/02/15 23:09:59, Peter Kasting wrote: > On 2011/02/15 22:55:03, varunjain wrote: > > On 2011/02/15 19:10:19, Peter Kasting wrote: > > > Nit: Space after "for" and ";", make |i| a size_t, and don't check against > > > child_count() (instead, put a DCHECK_GE(static_cast<size_t>(child_count()), > > > model_->result().size()); atop the function) > > > > Done. > > You only addressed the first comment, none of the rest. sry... I am not sure how I missed the second part. Done now. http://codereview.chromium.org/6286092/diff/9001/chrome/browser/ui/views/auto... File chrome/browser/ui/views/autocomplete/autocomplete_popup_contents_view.h (right): http://codereview.chromium.org/6286092/diff/9001/chrome/browser/ui/views/auto... chrome/browser/ui/views/autocomplete/autocomplete_popup_contents_view.h:93: static const int kIconVerticalPadding = 2; On 2011/02/15 23:09:59, Peter Kasting wrote: > On 2011/02/15 22:55:03, varunjain wrote: > > On 2011/02/15 19:10:19, Peter Kasting wrote: > > > Don't define these here. Declare here, define them in the .cc file. > > > > > > Nit: Just name these kMinimum{Icon,Text}VerticalPadding and use a single > > comment > > > above both. > > > > Done. > > You only addressed the first comment, not the second. Done now. http://codereview.chromium.org/6286092/diff/9001/chrome/browser/ui/views/auto... chrome/browser/ui/views/autocomplete/autocomplete_popup_contents_view.h:105: int icon_vertical_padding_; On 2011/02/15 23:09:59, Peter Kasting wrote: > On 2011/02/15 22:55:03, varunjain wrote: > > On 2011/02/15 19:10:19, Peter Kasting wrote: > > > Why do we have both constants and member variables? We can use the > constants > > > directly, we don't need these. > > > > The padding will vary for the touch version that I plan to subclass. Hence the > > member variables. > > Then do we need public constants? Can't these constants instead be local to the > file or constructor? made them local to the file.
Were there any changes in the code you moved to your new files, other than what was already covered above? If not, LGTM. http://codereview.chromium.org/6286092/diff/19003/chrome/browser/ui/views/aut... File chrome/browser/ui/views/autocomplete/autocomplete_result_view.cc (right): http://codereview.chromium.org/6286092/diff/19003/chrome/browser/ui/views/aut... chrome/browser/ui/views/autocomplete/autocomplete_result_view.cc:29: const int kIconVerticalPadding = 2; Nit: I'd still like to see these named with "Minimal" after "k", which will let you eliminate the second half of the comment. http://codereview.chromium.org/6286092/diff/19003/chrome/browser/ui/views/aut... File chrome/browser/ui/views/autocomplete/autocomplete_result_view.h (right): http://codereview.chromium.org/6286092/diff/19003/chrome/browser/ui/views/aut... chrome/browser/ui/views/autocomplete/autocomplete_result_view.h:16: class Canvas; Nit: No indent here
>Were there any changes in the code you moved to your new >files, other than what >was already covered above? If not, LGTM. only changes were addition of headers in the new file and removing the unwanted ones from the old one. http://codereview.chromium.org/6286092/diff/19003/chrome/browser/ui/views/aut... File chrome/browser/ui/views/autocomplete/autocomplete_result_view.cc (right): http://codereview.chromium.org/6286092/diff/19003/chrome/browser/ui/views/aut... chrome/browser/ui/views/autocomplete/autocomplete_result_view.cc:29: const int kIconVerticalPadding = 2; On 2011/02/15 23:49:22, Peter Kasting wrote: > Nit: I'd still like to see these named with "Minimal" after "k", which will let > you eliminate the second half of the comment. I dont think the comment should be modified... but adding the "Minimal" does make the variable name more meaningful. So, Done. http://codereview.chromium.org/6286092/diff/19003/chrome/browser/ui/views/aut... File chrome/browser/ui/views/autocomplete/autocomplete_result_view.h (right): http://codereview.chromium.org/6286092/diff/19003/chrome/browser/ui/views/aut... chrome/browser/ui/views/autocomplete/autocomplete_result_view.h:16: class Canvas; On 2011/02/15 23:49:22, Peter Kasting wrote: > Nit: No indent here Done.
http://codereview.chromium.org/6286092/diff/20001/chrome/browser/ui/views/aut... File chrome/browser/ui/views/autocomplete/autocomplete_result_view.cc (right): http://codereview.chromium.org/6286092/diff/20001/chrome/browser/ui/views/aut... chrome/browser/ui/views/autocomplete/autocomplete_result_view.cc:29: const int kMinimalIconVerticalPadding = 2; Sorry, I meant "Minimum" instead of "Minimal", my bad. Also, it really is obvious what "minimum" means. Please don't keep comments in that are redundant with the code, they decrease rather than increase readability.
Also refactored autocomplete_result_view_model interface to a seperate file as discussed with Oshima offline. http://codereview.chromium.org/6286092/diff/20001/chrome/browser/ui/views/aut... File chrome/browser/ui/views/autocomplete/autocomplete_result_view.cc (right): http://codereview.chromium.org/6286092/diff/20001/chrome/browser/ui/views/aut... chrome/browser/ui/views/autocomplete/autocomplete_result_view.cc:29: const int kMinimalIconVerticalPadding = 2; On 2011/02/16 00:33:47, Peter Kasting wrote: > Sorry, I meant "Minimum" instead of "Minimal", my bad. > > Also, it really is obvious what "minimum" means. Please don't keep comments in > that are redundant with the code, they decrease rather than increase > readability. Done.
Still LGTM http://codereview.chromium.org/6286092/diff/17011/chrome/browser/ui/views/aut... File chrome/browser/ui/views/autocomplete/autocomplete_result_view.cc (right): http://codereview.chromium.org/6286092/diff/17011/chrome/browser/ui/views/aut... chrome/browser/ui/views/autocomplete/autocomplete_result_view.cc:32: }; Nit: I think you don't need ';' and I think you do want " // namespace".
LGTM http://codereview.chromium.org/6286092/diff/17011/chrome/browser/ui/views/aut... File chrome/browser/ui/views/autocomplete/autocomplete_result_view.cc (right): http://codereview.chromium.org/6286092/diff/17011/chrome/browser/ui/views/aut... chrome/browser/ui/views/autocomplete/autocomplete_result_view.cc:90: CHECK(model_index >= 0); CHECK_GE
http://codereview.chromium.org/6286092/diff/17011/chrome/browser/ui/views/aut... File chrome/browser/ui/views/autocomplete/autocomplete_result_view.cc (right): http://codereview.chromium.org/6286092/diff/17011/chrome/browser/ui/views/aut... chrome/browser/ui/views/autocomplete/autocomplete_result_view.cc:32: }; On 2011/02/16 01:35:02, Peter Kasting wrote: > Nit: I think you don't need ';' and I think you do want " // namespace". Done. http://codereview.chromium.org/6286092/diff/17011/chrome/browser/ui/views/aut... chrome/browser/ui/views/autocomplete/autocomplete_result_view.cc:90: CHECK(model_index >= 0); On 2011/02/16 01:45:35, oshima wrote: > CHECK_GE Done. |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
