Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
6349101:
Create a new autocomplete popup for touch. This CL depends on (Closed)
|
Created:
9 years, 10 months ago by varunjain Modified:
9 years, 7 months ago CC:
chromium-reviews Visibility:
Public. |
DescriptionCreate a new autocomplete popup for touch. This CL depends on
http://codereview.chromium.org/6286092/
BUG=none
TEST=none
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=75414
Patch Set 1 #Patch Set 2 : minor changes #Patch Set 3 : merged changes from refactoring CL #Patch Set 4 : addressed review comments #Patch Set 5 : The previous patch got messed up. Please ignore that one. #Patch Set 6 : fixed minor compile issue #Patch Set 7 : merged changes from refactoring CL #
Total comments: 24
Patch Set 8 : addressed reviewer comments #
Total comments: 34
Patch Set 9 : modified according to comments #Patch Set 10 : minor changes #
Total comments: 8
Patch Set 11 : addressed comments #
Total comments: 28
Patch Set 12 : modified according to comments #Patch Set 13 : changed according to comments #
Total comments: 2
Patch Set 14 : minor changes #Messages
Total messages: 20 (0 generated)
Hi Oshima san This is a followup CL to the refactoring CL I sent earlier. This one actually creates the new autocomplete popup.
Please also see this CL that adds the new touch specific autocomplete popup view
http://codereview.chromium.org/6349101/diff/12001/chrome/browser/ui/views/aut... File chrome/browser/ui/views/autocomplete/autocomplete_popup_contents_view.h (right): http://codereview.chromium.org/6349101/diff/12001/chrome/browser/ui/views/aut... chrome/browser/ui/views/autocomplete/autocomplete_popup_contents_view.h:48: AutocompletePopupModel* model() { return model_.get(); } Do we need these public accessors? Wouldn't simply making the members be protected suffice? http://codereview.chromium.org/6349101/diff/12001/chrome/browser/ui/views/aut... File chrome/browser/ui/views/autocomplete/touch_autocomplete_popup_contents_view.cc (right): http://codereview.chromium.org/6349101/diff/12001/chrome/browser/ui/views/aut... chrome/browser/ui/views/autocomplete/touch_autocomplete_popup_contents_view.cc:20: //////////////////////////////////////////////////////////////////////////////// Nit: In new code, I ask people to avoid these big banners over each different access-type section. I suggest just putting a single banner over the entire class, like: // TouchAutocompleteResultView ----------------------- [79 chars] ...and then two blank lines between the end of the class and the banner for the next one. http://codereview.chromium.org/6349101/diff/12001/chrome/browser/ui/views/aut... chrome/browser/ui/views/autocomplete/touch_autocomplete_popup_contents_view.cc:37: 2 * std::max(normal_font().GetHeight(), bold_font().GetHeight()); Why "2 *"? http://codereview.chromium.org/6349101/diff/12001/chrome/browser/ui/views/aut... chrome/browser/ui/views/autocomplete/touch_autocomplete_popup_contents_view.cc:64: + std::max(normal_font().GetHeight(), bold_font().GetHeight()); Nit: + goes at end of previous line http://codereview.chromium.org/6349101/diff/12001/chrome/browser/ui/views/aut... chrome/browser/ui/views/autocomplete/touch_autocomplete_popup_contents_view.cc:87: Layout(); Why is this necessary when it's not necessary in the base class? http://codereview.chromium.org/6349101/diff/12001/chrome/browser/ui/views/aut... chrome/browser/ui/views/autocomplete/touch_autocomplete_popup_contents_view.cc:121: if (max_child_height < child_height) Nit: How about just: max_child_height = std::max(max_child_height, child_height); http://codereview.chromium.org/6349101/diff/12001/chrome/browser/ui/views/aut... chrome/browser/ui/views/autocomplete/touch_autocomplete_popup_contents_view.cc:132: int result_size = model()->result().size(); You'll want to update this function to look like the newest version of the non-touch code, once you check in that other change. http://codereview.chromium.org/6349101/diff/12001/chrome/browser/ui/views/aut... chrome/browser/ui/views/autocomplete/touch_autocomplete_popup_contents_view.cc:146: AutocompleteResultView* TouchAutocompletePopupContentsView::CreateResultView( So, how come we use a factory function in this case, but an #if in AutocompleteEditViewViews' constructor where we actually create the popup view? Shouldn't we be consistent? http://codereview.chromium.org/6349101/diff/12001/chrome/browser/ui/views/aut... chrome/browser/ui/views/autocomplete/touch_autocomplete_popup_contents_view.cc:162: if (visible_child_count > kMaxResultViews) Whoah there. You don't want to be clamping the visible results here. You want to do this clamping in the AutocompleteController instead by modifying the existing global clamp value (which is 6) and probably also the per-provider clamp value (which is 3). Once you do this, this function can disappear and your callers can all get simpler, because they can simply walk through the childredn skipping non-visible ones, not having to keep two loop iterators.
http://codereview.chromium.org/6349101/diff/12001/chrome/browser/ui/views/aut... File chrome/browser/ui/views/autocomplete/autocomplete_popup_contents_view.h (right): http://codereview.chromium.org/6349101/diff/12001/chrome/browser/ui/views/aut... chrome/browser/ui/views/autocomplete/autocomplete_popup_contents_view.h:48: AutocompletePopupModel* model() { return model_.get(); } On 2011/02/16 00:50:35, Peter Kasting wrote: > Do we need these public accessors? Wouldn't simply making the members be > protected suffice? Done. http://codereview.chromium.org/6349101/diff/12001/chrome/browser/ui/views/aut... File chrome/browser/ui/views/autocomplete/touch_autocomplete_popup_contents_view.cc (right): http://codereview.chromium.org/6349101/diff/12001/chrome/browser/ui/views/aut... chrome/browser/ui/views/autocomplete/touch_autocomplete_popup_contents_view.cc:20: //////////////////////////////////////////////////////////////////////////////// On 2011/02/16 00:50:35, Peter Kasting wrote: > Nit: In new code, I ask people to avoid these big banners over each different > access-type section. I suggest just putting a single banner over the entire > class, like: > > // TouchAutocompleteResultView ----------------------- [79 chars] > > ...and then two blank lines between the end of the class and the banner for the > next one. Done. http://codereview.chromium.org/6349101/diff/12001/chrome/browser/ui/views/aut... chrome/browser/ui/views/autocomplete/touch_autocomplete_popup_contents_view.cc:37: 2 * std::max(normal_font().GetHeight(), bold_font().GetHeight()); On 2011/02/16 00:50:35, Peter Kasting wrote: > Why "2 *"? added comment to explain. http://codereview.chromium.org/6349101/diff/12001/chrome/browser/ui/views/aut... chrome/browser/ui/views/autocomplete/touch_autocomplete_popup_contents_view.cc:64: + std::max(normal_font().GetHeight(), bold_font().GetHeight()); On 2011/02/16 00:50:35, Peter Kasting wrote: > Nit: + goes at end of previous line not required anymore http://codereview.chromium.org/6349101/diff/12001/chrome/browser/ui/views/aut... chrome/browser/ui/views/autocomplete/touch_autocomplete_popup_contents_view.cc:87: Layout(); On 2011/02/16 00:50:35, Peter Kasting wrote: > Why is this necessary when it's not necessary in the base class? Layout happens in the base class implicitly because whenever the number of suggestions change, the popup changes bounds (height) resulting in a bounds changed event (that causes layout). In touch case, the height of the popup never changes as the suggestions are added horizontally. Hence, bounds never change... so I have to explicitly call layout. http://codereview.chromium.org/6349101/diff/12001/chrome/browser/ui/views/aut... chrome/browser/ui/views/autocomplete/touch_autocomplete_popup_contents_view.cc:121: if (max_child_height < child_height) On 2011/02/16 00:50:35, Peter Kasting wrote: > Nit: How about just: > > max_child_height = std::max(max_child_height, child_height); Done. http://codereview.chromium.org/6349101/diff/12001/chrome/browser/ui/views/aut... chrome/browser/ui/views/autocomplete/touch_autocomplete_popup_contents_view.cc:132: int result_size = model()->result().size(); On 2011/02/16 00:50:35, Peter Kasting wrote: > You'll want to update this function to look like the newest version of the > non-touch code, once you check in that other change. Done. http://codereview.chromium.org/6349101/diff/12001/chrome/browser/ui/views/aut... chrome/browser/ui/views/autocomplete/touch_autocomplete_popup_contents_view.cc:146: AutocompleteResultView* TouchAutocompletePopupContentsView::CreateResultView( On 2011/02/16 00:50:35, Peter Kasting wrote: > So, how come we use a factory function in this case, but an #if in > AutocompleteEditViewViews' constructor where we actually create the popup view? > Shouldn't we be consistent? we cannot use a factory function in AutocompleteEditViewViews' constructor because we are not subclassing AutocompleteEditViewViews so there is no way to decide whether we want the touch implementation or the normal one. I did not use the #if here in the greater effort of minimizing #if usage in chrome. Another option I considered instead of using #if in AutocompleteEditViewViews, we create a PopupViewProvider and call PopupViewProvider.provide() in AutocompleteEditViewViews' constructor. PopupViewProvider will have separate implementation for touch and normal and the correct implementation can be chose through restrictions in gyp files. But I think thats a total overkill for this purpose. I welcome other ideas you may have. http://codereview.chromium.org/6349101/diff/12001/chrome/browser/ui/views/aut... chrome/browser/ui/views/autocomplete/touch_autocomplete_popup_contents_view.cc:162: if (visible_child_count > kMaxResultViews) On 2011/02/16 00:50:35, Peter Kasting wrote: > Whoah there. You don't want to be clamping the visible results here. You want > to do this clamping in the AutocompleteController instead by modifying the > existing global clamp value (which is 6) and probably also the per-provider > clamp value (which is 3). > > Once you do this, this function can disappear and your callers can all get > simpler, because they can simply walk through the childredn skipping non-visible > ones, not having to keep two loop iterators. yes... I realized that clamping here doesnot work very well as the controller still thinks that the clamp is at 6. Removing the clamping here. But I still need the visible child count to determine the width of a single result view and also when drawing lines in PaintChildren()
http://codereview.chromium.org/6349101/diff/12001/chrome/browser/ui/views/aut... File chrome/browser/ui/views/autocomplete/touch_autocomplete_popup_contents_view.cc (right): http://codereview.chromium.org/6349101/diff/12001/chrome/browser/ui/views/aut... chrome/browser/ui/views/autocomplete/touch_autocomplete_popup_contents_view.cc:146: AutocompleteResultView* TouchAutocompletePopupContentsView::CreateResultView( On 2011/02/16 18:48:10, varunjain wrote: > On 2011/02/16 00:50:35, Peter Kasting wrote: > > So, how come we use a factory function in this case, but an #if in > > AutocompleteEditViewViews' constructor where we actually create the popup > view? > > Shouldn't we be consistent? > > we cannot use a factory function in AutocompleteEditViewViews' constructor > because we are not subclassing AutocompleteEditViewViews so there is no way to > decide whether we want the touch implementation or the normal one. > I did not use the #if here in the greater effort of minimizing #if usage in > chrome. > Another option I considered instead of using #if in AutocompleteEditViewViews, > we create a PopupViewProvider and call PopupViewProvider.provide() in > AutocompleteEditViewViews' constructor. PopupViewProvider will have separate > implementation for touch and normal and the correct implementation can be chose > through restrictions in gyp files. But I think thats a total overkill for this > purpose. I welcome other ideas you may have. At some point, we need to merge touch and chromeos build (so that we have one image, and possibly a user can switch between two modes), and we will need a factory method/object to create one of contents views. Here are a couple of options. 1) Create a factory method and uses #ifdef TOUCH_UI inside factory method. This still requires if/def but it's better than putting #ifdef in initializer list IMO. 2) Create a factory method, and have one implementation for touch and another for touch. We switch the implementation using gyp file. 3) Create two subclass for touch and non-touch version of ContentsView, and include the impl of factory function there. This is probably overkill though. My preference at this point is 1), but don't have strong feeling about it. http://codereview.chromium.org/6349101/diff/12002/chrome/browser/autocomplete... File chrome/browser/autocomplete/autocomplete_edit_view_views.cc (right): http://codereview.chromium.org/6349101/diff/12002/chrome/browser/autocomplete... chrome/browser/autocomplete/autocomplete_edit_view_views.cc:118: #endif I'd like to avoid having if/defs in initializer list. See my comment about factory method. http://codereview.chromium.org/6349101/diff/12002/chrome/browser/ui/views/aut... File chrome/browser/ui/views/autocomplete/autocomplete_popup_contents_view.h (right): http://codereview.chromium.org/6349101/diff/12002/chrome/browser/ui/views/aut... chrome/browser/ui/views/autocomplete/autocomplete_popup_contents_view.h:15: #include "ui/gfx/canvas_skia.h" isn't forward decl enough? http://codereview.chromium.org/6349101/diff/12002/chrome/browser/ui/views/aut... File chrome/browser/ui/views/autocomplete/autocomplete_result_view.h (right): http://codereview.chromium.org/6349101/diff/12002/chrome/browser/ui/views/aut... chrome/browser/ui/views/autocomplete/autocomplete_result_view.h:47: gfx::Font normal_font() const { return normal_font_; } const gfx::Font& same for the rest. http://codereview.chromium.org/6349101/diff/12002/chrome/browser/ui/views/aut... chrome/browser/ui/views/autocomplete/autocomplete_result_view.h:61: static int icon_size_; is this const? yes, remove _ and use const naming. if not, use accessor method instead. http://codereview.chromium.org/6349101/diff/12002/chrome/browser/ui/views/aut... File chrome/browser/ui/views/autocomplete/touch_autocomplete_popup_contents_view.cc (right): http://codereview.chromium.org/6349101/diff/12002/chrome/browser/ui/views/aut... chrome/browser/ui/views/autocomplete/touch_autocomplete_popup_contents_view.cc:19: // TouchAutocompleteResultView ------------------------------------------------ nit: //////////////////// // TouchAutocompl.. is more common. http://codereview.chromium.org/6349101/diff/12002/chrome/browser/ui/views/aut... chrome/browser/ui/views/autocomplete/touch_autocomplete_popup_contents_view.cc:96: ++i) { indent http://codereview.chromium.org/6349101/diff/12002/chrome/browser/ui/views/aut... chrome/browser/ui/views/autocomplete/touch_autocomplete_popup_contents_view.cc:113: int max_child_height = 0; is this used? http://codereview.chromium.org/6349101/diff/12002/chrome/browser/ui/views/aut... chrome/browser/ui/views/autocomplete/touch_autocomplete_popup_contents_view.cc:134: opt_in_view_->GetPreferredSize().height()); {} for two or more lines http://codereview.chromium.org/6349101/diff/12002/chrome/browser/ui/views/aut... chrome/browser/ui/views/autocomplete/touch_autocomplete_popup_contents_view.cc:148: for (int i = 0; i < child_count(); ++i) i think you need {} in this case. http://codereview.chromium.org/6349101/diff/12002/chrome/browser/ui/views/aut... File chrome/browser/ui/views/autocomplete/touch_autocomplete_popup_contents_view.h (right): http://codereview.chromium.org/6349101/diff/12002/chrome/browser/ui/views/aut... chrome/browser/ui/views/autocomplete/touch_autocomplete_popup_contents_view.h:14: class Profile; add new line between namespaces. http://codereview.chromium.org/6349101/diff/12002/chrome/browser/ui/views/aut... chrome/browser/ui/views/autocomplete/touch_autocomplete_popup_contents_view.h:41: int GetFontHeight(); can this be const? DISALLOW_COPY_AND_ASSIGN http://codereview.chromium.org/6349101/diff/12002/chrome/browser/ui/views/aut... chrome/browser/ui/views/autocomplete/touch_autocomplete_popup_contents_view.h:68: int GetVisibleChildCount(); const? http://codereview.chromium.org/6349101/diff/12002/chrome/browser/ui/views/aut... chrome/browser/ui/views/autocomplete/touch_autocomplete_popup_contents_view.h:69: DISALLOW_COPY_AND_ASSIGN
http://codereview.chromium.org/6349101/diff/12001/chrome/browser/ui/views/aut... File chrome/browser/ui/views/autocomplete/touch_autocomplete_popup_contents_view.cc (right): http://codereview.chromium.org/6349101/diff/12001/chrome/browser/ui/views/aut... chrome/browser/ui/views/autocomplete/touch_autocomplete_popup_contents_view.cc:87: Layout(); On 2011/02/16 18:48:10, varunjain wrote: > On 2011/02/16 00:50:35, Peter Kasting wrote: > > Why is this necessary when it's not necessary in the base class? > > Layout happens in the base class implicitly because whenever the number of > suggestions change, the popup changes bounds (height) resulting in a bounds > changed event (that causes layout). Then you're not doing things correctly in the Touch version. Since you're adding views horizontally, you should change CalculatePopupHeight() to calculate a rect instead, and then have the animation code animate you horizontally instead of vertically. This way, you'll get the same logical animation (just in a different physical direction) and have the same "bounds changed" event sequences. http://codereview.chromium.org/6349101/diff/12001/chrome/browser/ui/views/aut... chrome/browser/ui/views/autocomplete/touch_autocomplete_popup_contents_view.cc:162: if (visible_child_count > kMaxResultViews) On 2011/02/16 18:48:10, varunjain wrote: > But I still > need the visible child count to determine the width of a single result view and > also when drawing lines in PaintChildren() No, you don't. In both cases, you can simply keep the current X coordinate as a function-scope temp and add to it every time you encounter a visible child. That will kill off this function and noticeably simplify both those callers. http://codereview.chromium.org/6349101/diff/12002/chrome/browser/ui/views/aut... File chrome/browser/ui/views/autocomplete/autocomplete_popup_contents_view.h (right): http://codereview.chromium.org/6349101/diff/12002/chrome/browser/ui/views/aut... chrome/browser/ui/views/autocomplete/autocomplete_popup_contents_view.h:93: // The provider of our result set. Nit: No need for this comment http://codereview.chromium.org/6349101/diff/12002/chrome/browser/ui/views/aut... chrome/browser/ui/views/autocomplete/autocomplete_popup_contents_view.h:96: // If non-NULL the instant opt-in-view is visible. Nit: How about: // The "Opt-in to Instant" promo view, if there is one. http://codereview.chromium.org/6349101/diff/12002/chrome/browser/ui/views/aut... File chrome/browser/ui/views/autocomplete/touch_autocomplete_popup_contents_view.cc (right): http://codereview.chromium.org/6349101/diff/12002/chrome/browser/ui/views/aut... chrome/browser/ui/views/autocomplete/touch_autocomplete_popup_contents_view.cc:19: // TouchAutocompleteResultView ------------------------------------------------ On 2011/02/16 20:00:27, oshima wrote: > nit: > > //////////////////// > // TouchAutocompl.. > > is more common. I explicitly directed this syntax because it is what I'm trying to standardize things on, and the other is needlessly verbose (especially with a new section for each different access type).
http://codereview.chromium.org/6349101/diff/12001/chrome/browser/ui/views/aut... File chrome/browser/ui/views/autocomplete/touch_autocomplete_popup_contents_view.cc (right): http://codereview.chromium.org/6349101/diff/12001/chrome/browser/ui/views/aut... chrome/browser/ui/views/autocomplete/touch_autocomplete_popup_contents_view.cc:87: Layout(); On 2011/02/16 21:50:22, Peter Kasting wrote: > On 2011/02/16 18:48:10, varunjain wrote: > > On 2011/02/16 00:50:35, Peter Kasting wrote: > > > Why is this necessary when it's not necessary in the base class? > > > > Layout happens in the base class implicitly because whenever the number of > > suggestions change, the popup changes bounds (height) resulting in a bounds > > changed event (that causes layout). > > Then you're not doing things correctly in the Touch version. Since you're > adding views horizontally, you should change CalculatePopupHeight() to calculate > a rect instead, and then have the animation code animate you horizontally > instead of vertically. This way, you'll get the same logical animation (just in > a different physical direction) and have the same "bounds changed" event > sequences. Not quiet. The touch popup never changes size (unless the height of a result somehow changes). The width of the popup is constant (equal to the width of location bar) and the width of the results vary based on the number of results. So there is no animation required. http://codereview.chromium.org/6349101/diff/12001/chrome/browser/ui/views/aut... chrome/browser/ui/views/autocomplete/touch_autocomplete_popup_contents_view.cc:146: AutocompleteResultView* TouchAutocompletePopupContentsView::CreateResultView( On 2011/02/16 20:00:27, oshima wrote: > On 2011/02/16 18:48:10, varunjain wrote: > > On 2011/02/16 00:50:35, Peter Kasting wrote: > > > So, how come we use a factory function in this case, but an #if in > > > AutocompleteEditViewViews' constructor where we actually create the popup > > view? > > > Shouldn't we be consistent? > > > > we cannot use a factory function in AutocompleteEditViewViews' constructor > > because we are not subclassing AutocompleteEditViewViews so there is no way to > > decide whether we want the touch implementation or the normal one. > > I did not use the #if here in the greater effort of minimizing #if usage in > > chrome. > > Another option I considered instead of using #if in AutocompleteEditViewViews, > > we create a PopupViewProvider and call PopupViewProvider.provide() in > > AutocompleteEditViewViews' constructor. PopupViewProvider will have separate > > implementation for touch and normal and the correct implementation can be > chose > > through restrictions in gyp files. But I think thats a total overkill for this > > purpose. I welcome other ideas you may have. > > At some point, we need to merge touch and chromeos build (so that we have one > image, > and possibly a user can switch between two modes), and we will need a factory > method/object to create one of contents views. Here are a couple of options. > 1) Create a factory method and uses #ifdef TOUCH_UI inside factory method. This > still requires if/def but it's better than putting #ifdef in initializer list > IMO. > 2) Create a factory method, and have one implementation for touch and another > for > touch. We switch the implementation using gyp file. > 3) Create two subclass for touch and non-touch version of ContentsView, and > include the impl of factory function there. This is probably overkill though. > > My preference at this point is 1), but don't have strong feeling about it. > Done. http://codereview.chromium.org/6349101/diff/12001/chrome/browser/ui/views/aut... chrome/browser/ui/views/autocomplete/touch_autocomplete_popup_contents_view.cc:162: if (visible_child_count > kMaxResultViews) On 2011/02/16 21:50:22, Peter Kasting wrote: > On 2011/02/16 18:48:10, varunjain wrote: > > But I still > > need the visible child count to determine the width of a single result view > and > > also when drawing lines in PaintChildren() > > No, you don't. In both cases, you can simply keep the current X coordinate as a > function-scope temp and add to it every time you encounter a visible child. > That will kill off this function and noticeably simplify both those callers. Sry... I am really not getting this :( as the width of a visible child is not constant (its essentially total_width_of_popup/number_of_chilren), I cannot simply keep a running X cordinate and add to it (because I will not know what to add to it) http://codereview.chromium.org/6349101/diff/12002/chrome/browser/autocomplete... File chrome/browser/autocomplete/autocomplete_edit_view_views.cc (right): http://codereview.chromium.org/6349101/diff/12002/chrome/browser/autocomplete... chrome/browser/autocomplete/autocomplete_edit_view_views.cc:118: #endif On 2011/02/16 20:00:27, oshima wrote: > I'd like to avoid having if/defs in initializer list. > See my comment about factory method. Done. http://codereview.chromium.org/6349101/diff/12002/chrome/browser/ui/views/aut... File chrome/browser/ui/views/autocomplete/autocomplete_popup_contents_view.h (right): http://codereview.chromium.org/6349101/diff/12002/chrome/browser/ui/views/aut... chrome/browser/ui/views/autocomplete/autocomplete_popup_contents_view.h:15: #include "ui/gfx/canvas_skia.h" On 2011/02/16 20:00:27, oshima wrote: > isn't forward decl enough? Done. http://codereview.chromium.org/6349101/diff/12002/chrome/browser/ui/views/aut... chrome/browser/ui/views/autocomplete/autocomplete_popup_contents_view.h:93: // The provider of our result set. On 2011/02/16 21:50:22, Peter Kasting wrote: > Nit: No need for this comment Done. http://codereview.chromium.org/6349101/diff/12002/chrome/browser/ui/views/aut... chrome/browser/ui/views/autocomplete/autocomplete_popup_contents_view.h:96: // If non-NULL the instant opt-in-view is visible. On 2011/02/16 21:50:22, Peter Kasting wrote: > Nit: How about: > > // The "Opt-in to Instant" promo view, if there is one. Done. http://codereview.chromium.org/6349101/diff/12002/chrome/browser/ui/views/aut... File chrome/browser/ui/views/autocomplete/autocomplete_result_view.h (right): http://codereview.chromium.org/6349101/diff/12002/chrome/browser/ui/views/aut... chrome/browser/ui/views/autocomplete/autocomplete_result_view.h:47: gfx::Font normal_font() const { return normal_font_; } On 2011/02/16 20:00:27, oshima wrote: > const gfx::Font& same for the rest. Done. http://codereview.chromium.org/6349101/diff/12002/chrome/browser/ui/views/aut... chrome/browser/ui/views/autocomplete/autocomplete_result_view.h:61: static int icon_size_; On 2011/02/16 20:00:27, oshima wrote: > is this const? yes, remove _ and use const naming. if not, use accessor method > instead. not a const http://codereview.chromium.org/6349101/diff/12002/chrome/browser/ui/views/aut... File chrome/browser/ui/views/autocomplete/touch_autocomplete_popup_contents_view.cc (right): http://codereview.chromium.org/6349101/diff/12002/chrome/browser/ui/views/aut... chrome/browser/ui/views/autocomplete/touch_autocomplete_popup_contents_view.cc:96: ++i) { On 2011/02/16 20:00:27, oshima wrote: > indent its already indented 4 spaces. Do you want to indent more? less? http://codereview.chromium.org/6349101/diff/12002/chrome/browser/ui/views/aut... chrome/browser/ui/views/autocomplete/touch_autocomplete_popup_contents_view.cc:113: int max_child_height = 0; On 2011/02/16 20:00:27, oshima wrote: > is this used? it is now :) http://codereview.chromium.org/6349101/diff/12002/chrome/browser/ui/views/aut... chrome/browser/ui/views/autocomplete/touch_autocomplete_popup_contents_view.cc:134: opt_in_view_->GetPreferredSize().height()); On 2011/02/16 20:00:27, oshima wrote: > {} for two or more lines Done. http://codereview.chromium.org/6349101/diff/12002/chrome/browser/ui/views/aut... chrome/browser/ui/views/autocomplete/touch_autocomplete_popup_contents_view.cc:148: for (int i = 0; i < child_count(); ++i) On 2011/02/16 20:00:27, oshima wrote: > i think you need {} in this case. Done. http://codereview.chromium.org/6349101/diff/12002/chrome/browser/ui/views/aut... File chrome/browser/ui/views/autocomplete/touch_autocomplete_popup_contents_view.h (right): http://codereview.chromium.org/6349101/diff/12002/chrome/browser/ui/views/aut... chrome/browser/ui/views/autocomplete/touch_autocomplete_popup_contents_view.h:14: class Profile; On 2011/02/16 20:00:27, oshima wrote: > add new line between namespaces. Done. http://codereview.chromium.org/6349101/diff/12002/chrome/browser/ui/views/aut... chrome/browser/ui/views/autocomplete/touch_autocomplete_popup_contents_view.h:41: int GetFontHeight(); On 2011/02/16 20:00:27, oshima wrote: > can this be const? > > DISALLOW_COPY_AND_ASSIGN Done. http://codereview.chromium.org/6349101/diff/12002/chrome/browser/ui/views/aut... chrome/browser/ui/views/autocomplete/touch_autocomplete_popup_contents_view.h:68: int GetVisibleChildCount(); On 2011/02/16 20:00:27, oshima wrote: > const? Done. http://codereview.chromium.org/6349101/diff/12002/chrome/browser/ui/views/aut... chrome/browser/ui/views/autocomplete/touch_autocomplete_popup_contents_view.h:69: On 2011/02/16 20:00:27, oshima wrote: > DISALLOW_COPY_AND_ASSIGN Done.
http://codereview.chromium.org/6349101/diff/12002/chrome/browser/ui/views/aut... File chrome/browser/ui/views/autocomplete/touch_autocomplete_popup_contents_view.cc (right): http://codereview.chromium.org/6349101/diff/12002/chrome/browser/ui/views/aut... chrome/browser/ui/views/autocomplete/touch_autocomplete_popup_contents_view.cc:113: int max_child_height = 0; On 2011/02/16 20:00:27, oshima wrote: > is this used? you are right... this is not required. the change I did earlier is not correct. removed.
http://codereview.chromium.org/6349101/diff/12002/chrome/browser/ui/views/aut... File chrome/browser/ui/views/autocomplete/touch_autocomplete_popup_contents_view.cc (right): http://codereview.chromium.org/6349101/diff/12002/chrome/browser/ui/views/aut... chrome/browser/ui/views/autocomplete/touch_autocomplete_popup_contents_view.cc:19: // TouchAutocompleteResultView ------------------------------------------------ On 2011/02/16 21:50:22, Peter Kasting wrote: > On 2011/02/16 20:00:27, oshima wrote: > > nit: > > > > //////////////////// > > // TouchAutocompl.. > > > > is more common. > > I explicitly directed this syntax because it is what I'm trying to standardize > things on, and the other is needlessly verbose (especially with a new section > for each different access type). Good to know. Thanks for explanation. http://codereview.chromium.org/6349101/diff/12002/chrome/browser/ui/views/aut... chrome/browser/ui/views/autocomplete/touch_autocomplete_popup_contents_view.cc:96: ++i) { On 2011/02/16 23:12:37, varunjain wrote: > On 2011/02/16 20:00:27, oshima wrote: > > indent > > its already indented 4 spaces. Do you want to indent more? less? indent for "for" should be for (...; ...) { } http://codereview.chromium.org/6349101/diff/28015/chrome/browser/autocomplete... File chrome/browser/autocomplete/autocomplete_edit_view_views.cc (right): http://codereview.chromium.org/6349101/diff/28015/chrome/browser/autocomplete... chrome/browser/autocomplete/autocomplete_edit_view_views.cc:644: // static http://codereview.chromium.org/6349101/diff/28015/chrome/browser/ui/views/aut... File chrome/browser/ui/views/autocomplete/autocomplete_result_view.h (right): http://codereview.chromium.org/6349101/diff/28015/chrome/browser/ui/views/aut... chrome/browser/ui/views/autocomplete/autocomplete_result_view.h:61: static int icon_size_; if this isn't const, anyone can change this value. make it accessor, and hide it in anonymous namespace. http://codereview.chromium.org/6349101/diff/28015/chrome/browser/ui/views/aut... File chrome/browser/ui/views/autocomplete/touch_autocomplete_popup_contents_view.cc (right): http://codereview.chromium.org/6349101/diff/28015/chrome/browser/ui/views/aut... chrome/browser/ui/views/autocomplete/touch_autocomplete_popup_contents_view.cc:64: const int TouchAutocompleteResultView::GetFontHeight() { I meant int TouchAutocompleteResultView::GetFontHeight() const{ http://codereview.chromium.org/6349101/diff/28015/chrome/browser/ui/views/aut... chrome/browser/ui/views/autocomplete/touch_autocomplete_popup_contents_view.cc:145: const int TouchAutocompletePopupContentsView::GetVisibleChildCount() { I meant int TouchAutocompletePopupContentsView::GetVisibleChildCount() const {
http://codereview.chromium.org/6349101/diff/12002/chrome/browser/ui/views/aut... File chrome/browser/ui/views/autocomplete/touch_autocomplete_popup_contents_view.cc (right): http://codereview.chromium.org/6349101/diff/12002/chrome/browser/ui/views/aut... chrome/browser/ui/views/autocomplete/touch_autocomplete_popup_contents_view.cc:96: ++i) { On 2011/02/16 23:51:19, oshima wrote: > On 2011/02/16 23:12:37, varunjain wrote: > > On 2011/02/16 20:00:27, oshima wrote: > > > indent > > > > its already indented 4 spaces. Do you want to indent more? less? > > indent for "for" should be > for (...; > ...) { > } Done. http://codereview.chromium.org/6349101/diff/28015/chrome/browser/autocomplete... File chrome/browser/autocomplete/autocomplete_edit_view_views.cc (right): http://codereview.chromium.org/6349101/diff/28015/chrome/browser/autocomplete... chrome/browser/autocomplete/autocomplete_edit_view_views.cc:644: On 2011/02/16 23:51:19, oshima wrote: > // static this is not static http://codereview.chromium.org/6349101/diff/28015/chrome/browser/ui/views/aut... File chrome/browser/ui/views/autocomplete/autocomplete_result_view.h (right): http://codereview.chromium.org/6349101/diff/28015/chrome/browser/ui/views/aut... chrome/browser/ui/views/autocomplete/autocomplete_result_view.h:61: static int icon_size_; On 2011/02/16 23:51:19, oshima wrote: > if this isn't const, anyone can change this value. > make it accessor, and hide it in anonymous namespace. Moved to private as discussed http://codereview.chromium.org/6349101/diff/28015/chrome/browser/ui/views/aut... File chrome/browser/ui/views/autocomplete/touch_autocomplete_popup_contents_view.cc (right): http://codereview.chromium.org/6349101/diff/28015/chrome/browser/ui/views/aut... chrome/browser/ui/views/autocomplete/touch_autocomplete_popup_contents_view.cc:64: const int TouchAutocompleteResultView::GetFontHeight() { On 2011/02/16 23:51:19, oshima wrote: > I meant > int TouchAutocompleteResultView::GetFontHeight() const{ I meant that too... I think I need more coffee :) http://codereview.chromium.org/6349101/diff/28015/chrome/browser/ui/views/aut... chrome/browser/ui/views/autocomplete/touch_autocomplete_popup_contents_view.cc:145: const int TouchAutocompletePopupContentsView::GetVisibleChildCount() { On 2011/02/16 23:51:19, oshima wrote: > I meant > int TouchAutocompletePopupContentsView::GetVisibleChildCount() const { Done.
LGTM on my bits. I'll leave the final decision to Peter. - oshima On Wed, Feb 16, 2011 at 4:29 PM, <varunjain@chromium.org> wrote: > > > http://codereview.chromium.org/6349101/diff/12002/chrome/browser/ui/views/aut... > File > > chrome/browser/ui/views/autocomplete/touch_autocomplete_popup_contents_view.cc > (right): > > > http://codereview.chromium.org/6349101/diff/12002/chrome/browser/ui/views/aut... > > chrome/browser/ui/views/autocomplete/touch_autocomplete_popup_contents_view.cc:96: > ++i) { > On 2011/02/16 23:51:19, oshima wrote: > >> On 2011/02/16 23:12:37, varunjain wrote: >> > On 2011/02/16 20:00:27, oshima wrote: >> > > indent >> > >> > its already indented 4 spaces. Do you want to indent more? less? >> > > indent for "for" should be >> for (...; >> ...) { >> } >> > > Done. > > > > http://codereview.chromium.org/6349101/diff/28015/chrome/browser/autocomplete... > File chrome/browser/autocomplete/autocomplete_edit_view_views.cc > (right): > > > http://codereview.chromium.org/6349101/diff/28015/chrome/browser/autocomplete... > chrome/browser/autocomplete/autocomplete_edit_view_views.cc:644: > On 2011/02/16 23:51:19, oshima wrote: > >> // static >> > > this is not static > > > > http://codereview.chromium.org/6349101/diff/28015/chrome/browser/ui/views/aut... > File chrome/browser/ui/views/autocomplete/autocomplete_result_view.h > (right): > > > http://codereview.chromium.org/6349101/diff/28015/chrome/browser/ui/views/aut... > chrome/browser/ui/views/autocomplete/autocomplete_result_view.h:61: > static int icon_size_; > On 2011/02/16 23:51:19, oshima wrote: > >> if this isn't const, anyone can change this value. >> make it accessor, and hide it in anonymous namespace. >> > > Moved to private as discussed > > > > http://codereview.chromium.org/6349101/diff/28015/chrome/browser/ui/views/aut... > File > > chrome/browser/ui/views/autocomplete/touch_autocomplete_popup_contents_view.cc > (right): > > > http://codereview.chromium.org/6349101/diff/28015/chrome/browser/ui/views/aut... > > chrome/browser/ui/views/autocomplete/touch_autocomplete_popup_contents_view.cc:64: > const int TouchAutocompleteResultView::GetFontHeight() { > On 2011/02/16 23:51:19, oshima wrote: > >> I meant >> int TouchAutocompleteResultView::GetFontHeight() const{ >> > > I meant that too... I think I need more coffee :) > > > > http://codereview.chromium.org/6349101/diff/28015/chrome/browser/ui/views/aut... > > chrome/browser/ui/views/autocomplete/touch_autocomplete_popup_contents_view.cc:145: > const int TouchAutocompletePopupContentsView::GetVisibleChildCount() { > On 2011/02/16 23:51:19, oshima wrote: > >> I meant >> int TouchAutocompletePopupContentsView::GetVisibleChildCount() const { >> > > Done. > > > http://codereview.chromium.org/6349101/ >
I will take another look here tomorrow.
http://codereview.chromium.org/6349101/diff/28024/chrome/browser/autocomplete... File chrome/browser/autocomplete/autocomplete_edit_view_views.cc (right): http://codereview.chromium.org/6349101/diff/28024/chrome/browser/autocomplete... chrome/browser/autocomplete/autocomplete_edit_view_views.cc:646: Profile* profile, const views::View* location_bar) { Nit: One arg per line http://codereview.chromium.org/6349101/diff/28024/chrome/browser/autocomplete... chrome/browser/autocomplete/autocomplete_edit_view_views.cc:649: gfx::Font(), this, model_.get(), profile, location_bar); Nit: Why not bring this second line out of the #if? http://codereview.chromium.org/6349101/diff/28024/chrome/browser/autocomplete... File chrome/browser/autocomplete/autocomplete_edit_view_views.h (right): http://codereview.chromium.org/6349101/diff/28024/chrome/browser/autocomplete... chrome/browser/autocomplete/autocomplete_edit_view_views.h:152: const views::View* location_bar); Nit: No need for "views::" qualifier on View* here (or other places in this class' declaration or definition, except on return types in function definitions) http://codereview.chromium.org/6349101/diff/28024/chrome/browser/ui/views/aut... File chrome/browser/ui/views/autocomplete/touch_autocomplete_popup_contents_view.cc (right): http://codereview.chromium.org/6349101/diff/28024/chrome/browser/ui/views/aut... chrome/browser/ui/views/autocomplete/touch_autocomplete_popup_contents_view.cc:41: text_bounds().width(), Nit: Feel free to collapse arg 4 onto this line http://codereview.chromium.org/6349101/diff/28024/chrome/browser/ui/views/aut... chrome/browser/ui/views/autocomplete/touch_autocomplete_popup_contents_view.cc:49: return gfx::Size(0, std::max(icon_height, text_height)); Nit: I'd just collapse the temps into this statement http://codereview.chromium.org/6349101/diff/28024/chrome/browser/ui/views/aut... chrome/browser/ui/views/autocomplete/touch_autocomplete_popup_contents_view.cc:60: DrawString(canvas, match.description, match.description_class, true, x, y); Nit: I'd just collapse |y| into this statement http://codereview.chromium.org/6349101/diff/28024/chrome/browser/ui/views/aut... chrome/browser/ui/views/autocomplete/touch_autocomplete_popup_contents_view.cc:94: int visible_child_count = GetVisibleChildCount(); Here's how to do this without GetVisibleChildCount(). This also fixes a potential issue with unevenly-sized dividers if the views are not all the same height. gfx::Rect divider(GetContentsBounds()); divider.set_x(0); SkColor color = AutocompleteResultView::GetColor( AutocompleteResultView::NORMAL, AutocompleteResultView::DIMMED_TEXT); for (int i = 0; i < child_count(); ++i) { View* v = GetChildViewAt(i); if (v->IsVisible()) { int x = divider.x(); if (x) canvas->DrawLineInt(color, x, divider.y(), x, divider.bottom()); divider.set_x(v->bounds().right()); } } http://codereview.chromium.org/6349101/diff/28024/chrome/browser/ui/views/aut... chrome/browser/ui/views/autocomplete/touch_autocomplete_popup_contents_view.cc:108: void TouchAutocompletePopupContentsView::LayoutChildren() { This algorithm suffers from rounding error accumulation. Since after applying the above comment this will be the only consumer of GetVisibleChildCount(), we can inline that here and rewrite without the rounding error as follows: std::vector<View*> visible_children; for (int i = 0; i < child_count(); ++i) { View* v = GetChildViewAt(i); if (GetChildViewAt(i)->IsVisible()) visible_children.push_back(v); } gfx::Rect bounds(GetContentsBounds()); double child_width = static_cast<double>(bounds.width()) / visible_children.size(); int x = bounds.x(); for (size_t i = 0; i < visible_children.size(); ++i) { int next_x = bounds.x() + static_cast<int>(((i + 1) * child_width) + 0.5); visible_children[i]->SetBounds(x, bounds.y(), next_x - x, bounds.height()); x = next_x; } This would be even easier if View.h exposed iterators and child vector typedefs... I'm working on getting that stuff into the V2 API :) http://codereview.chromium.org/6349101/diff/28024/chrome/browser/ui/views/aut... chrome/browser/ui/views/autocomplete/touch_autocomplete_popup_contents_view.cc:129: if (opt_in_view_) { Nit: Shorter: popup_height = std::max(popup_height, opt_in_view_ ? opt_in_view_->GetPreferredSize().height() : 0); http://codereview.chromium.org/6349101/diff/28024/chrome/browser/ui/views/aut... File chrome/browser/ui/views/autocomplete/touch_autocomplete_popup_contents_view.h (right): http://codereview.chromium.org/6349101/diff/28024/chrome/browser/ui/views/aut... chrome/browser/ui/views/autocomplete/touch_autocomplete_popup_contents_view.h:33: // AutocompleteResultView overrides. Nit: In new code I prefer just the classname plus a colon: // AutocompleteResultView: (3 places) http://codereview.chromium.org/6349101/diff/28024/chrome/browser/ui/views/aut... chrome/browser/ui/views/autocomplete/touch_autocomplete_popup_contents_view.h:34: virtual void Layout(); Nit: Can all the public and protected functions in this file (except the constructors, but including the destructors) be private? http://codereview.chromium.org/6349101/diff/28024/chrome/browser/ui/views/aut... chrome/browser/ui/views/autocomplete/touch_autocomplete_popup_contents_view.h:48: class TouchAutocompletePopupContentsView: public AutocompletePopupContentsView { Nit: Space before ':' http://codereview.chromium.org/6349101/diff/28024/chrome/browser/ui/views/aut... chrome/browser/ui/views/autocomplete/touch_autocomplete_popup_contents_view.h:72: int GetVisibleChildCount() const; Nit: These two lines are indented too far
http://codereview.chromium.org/6349101/diff/28024/chrome/browser/autocomplete... File chrome/browser/autocomplete/autocomplete_edit_view_views.cc (right): http://codereview.chromium.org/6349101/diff/28024/chrome/browser/autocomplete... chrome/browser/autocomplete/autocomplete_edit_view_views.cc:646: Profile* profile, const views::View* location_bar) { On 2011/02/17 23:08:57, Peter Kasting wrote: > Nit: One arg per line Done. http://codereview.chromium.org/6349101/diff/28024/chrome/browser/autocomplete... chrome/browser/autocomplete/autocomplete_edit_view_views.cc:649: gfx::Font(), this, model_.get(), profile, location_bar); On 2011/02/17 23:08:57, Peter Kasting wrote: > Nit: Why not bring this second line out of the #if? It looked better to me that way... but anyways. Done. http://codereview.chromium.org/6349101/diff/28024/chrome/browser/autocomplete... File chrome/browser/autocomplete/autocomplete_edit_view_views.h (right): http://codereview.chromium.org/6349101/diff/28024/chrome/browser/autocomplete... chrome/browser/autocomplete/autocomplete_edit_view_views.h:152: const views::View* location_bar); On 2011/02/17 23:08:57, Peter Kasting wrote: > Nit: No need for "views::" qualifier on View* here (or other places in this > class' declaration or definition, except on return types in function > definitions) Done. http://codereview.chromium.org/6349101/diff/28024/chrome/browser/ui/views/aut... File chrome/browser/ui/views/autocomplete/touch_autocomplete_popup_contents_view.cc (right): http://codereview.chromium.org/6349101/diff/28024/chrome/browser/ui/views/aut... chrome/browser/ui/views/autocomplete/touch_autocomplete_popup_contents_view.cc:41: text_bounds().width(), On 2011/02/17 23:08:57, Peter Kasting wrote: > Nit: Feel free to collapse arg 4 onto this line Done. http://codereview.chromium.org/6349101/diff/28024/chrome/browser/ui/views/aut... chrome/browser/ui/views/autocomplete/touch_autocomplete_popup_contents_view.cc:49: return gfx::Size(0, std::max(icon_height, text_height)); On 2011/02/17 23:08:57, Peter Kasting wrote: > Nit: I'd just collapse the temps into this statement Done. http://codereview.chromium.org/6349101/diff/28024/chrome/browser/ui/views/aut... chrome/browser/ui/views/autocomplete/touch_autocomplete_popup_contents_view.cc:60: DrawString(canvas, match.description, match.description_class, true, x, y); On 2011/02/17 23:08:57, Peter Kasting wrote: > Nit: I'd just collapse |y| into this statement Done. http://codereview.chromium.org/6349101/diff/28024/chrome/browser/ui/views/aut... chrome/browser/ui/views/autocomplete/touch_autocomplete_popup_contents_view.cc:94: int visible_child_count = GetVisibleChildCount(); On 2011/02/17 23:08:57, Peter Kasting wrote: > Here's how to do this without GetVisibleChildCount(). This also fixes a > potential issue with unevenly-sized dividers if the views are not all the same > height. > > gfx::Rect divider(GetContentsBounds()); > divider.set_x(0); > SkColor color = AutocompleteResultView::GetColor( > AutocompleteResultView::NORMAL, AutocompleteResultView::DIMMED_TEXT); > for (int i = 0; i < child_count(); ++i) { > View* v = GetChildViewAt(i); > if (v->IsVisible()) { > int x = divider.x(); > if (x) > canvas->DrawLineInt(color, x, divider.y(), x, divider.bottom()); > divider.set_x(v->bounds().right()); > } > } Done. This is fine but IMO, this only makes the code hard to read without gaining much. Earlier the loop was simple to read. Now the reader has to stop and think for a second as to why there is an if (x) and how do visible_count-1 lines get drawn when the loop runs to visible_count. The term premature-optimization comes to mind when I see this :) http://codereview.chromium.org/6349101/diff/28024/chrome/browser/ui/views/aut... chrome/browser/ui/views/autocomplete/touch_autocomplete_popup_contents_view.cc:108: void TouchAutocompletePopupContentsView::LayoutChildren() { On 2011/02/17 23:08:57, Peter Kasting wrote: > This algorithm suffers from rounding error accumulation. > > Since after applying the above comment this will be the only consumer of > GetVisibleChildCount(), we can inline that here and rewrite without the rounding > error as follows: > > std::vector<View*> visible_children; > for (int i = 0; i < child_count(); ++i) { > View* v = GetChildViewAt(i); > if (GetChildViewAt(i)->IsVisible()) > visible_children.push_back(v); > } > gfx::Rect bounds(GetContentsBounds()); > double child_width = > static_cast<double>(bounds.width()) / visible_children.size(); > int x = bounds.x(); > for (size_t i = 0; i < visible_children.size(); ++i) { > int next_x = bounds.x() + static_cast<int>(((i + 1) * child_width) + 0.5); > visible_children[i]->SetBounds(x, bounds.y(), next_x - x, bounds.height()); > x = next_x; > } > > This would be even easier if View.h exposed iterators and child vector > typedefs... I'm working on getting that stuff into the V2 API :) Done. http://codereview.chromium.org/6349101/diff/28024/chrome/browser/ui/views/aut... chrome/browser/ui/views/autocomplete/touch_autocomplete_popup_contents_view.cc:129: if (opt_in_view_) { On 2011/02/17 23:08:57, Peter Kasting wrote: > Nit: Shorter: > > popup_height = std::max(popup_height, opt_in_view_ ? > opt_in_view_->GetPreferredSize().height() : 0); Done. http://codereview.chromium.org/6349101/diff/28024/chrome/browser/ui/views/aut... File chrome/browser/ui/views/autocomplete/touch_autocomplete_popup_contents_view.h (right): http://codereview.chromium.org/6349101/diff/28024/chrome/browser/ui/views/aut... chrome/browser/ui/views/autocomplete/touch_autocomplete_popup_contents_view.h:33: // AutocompleteResultView overrides. On 2011/02/17 23:08:57, Peter Kasting wrote: > Nit: In new code I prefer just the classname plus a colon: > > // AutocompleteResultView: > > (3 places) Done. http://codereview.chromium.org/6349101/diff/28024/chrome/browser/ui/views/aut... chrome/browser/ui/views/autocomplete/touch_autocomplete_popup_contents_view.h:34: virtual void Layout(); On 2011/02/17 23:08:57, Peter Kasting wrote: > Nit: Can all the public and protected functions in this file (except the > constructors, but including the destructors) be private? Done. http://codereview.chromium.org/6349101/diff/28024/chrome/browser/ui/views/aut... chrome/browser/ui/views/autocomplete/touch_autocomplete_popup_contents_view.h:48: class TouchAutocompletePopupContentsView: public AutocompletePopupContentsView { On 2011/02/17 23:08:57, Peter Kasting wrote: > Nit: Space before ':' Done. http://codereview.chromium.org/6349101/diff/28024/chrome/browser/ui/views/aut... chrome/browser/ui/views/autocomplete/touch_autocomplete_popup_contents_view.h:72: int GetVisibleChildCount() const; On 2011/02/17 23:08:57, Peter Kasting wrote: > Nit: These two lines are indented too far Done.
http://codereview.chromium.org/6349101/diff/28024/chrome/browser/ui/views/aut... File chrome/browser/ui/views/autocomplete/touch_autocomplete_popup_contents_view.cc (right): http://codereview.chromium.org/6349101/diff/28024/chrome/browser/ui/views/aut... chrome/browser/ui/views/autocomplete/touch_autocomplete_popup_contents_view.cc:94: int visible_child_count = GetVisibleChildCount(); On 2011/02/18 00:16:34, varunjain wrote: > This is fine but IMO, this only makes the code hard to read without > gaining much. All right, then how about this: Take what are currently the first six lines of LayoutChildren() and factor them back out to a private helper, "std::vector<Views*> GetVisibleChildren();". Then rewrite this block as: std::vector<Views*> visible_children(GetVisibleChildren()); if (visible_children.size() < 2) return; SkColor color = AutocompleteResultView::GetColor( AutocompleteResultView::NORMAL, AutocompleteResultView::DIMMED_TEXT); gfx::Rect bounds(GetContentsBounds()); for (std::vector<Views*>::const_iterator i(visible_children.begin() + 1); i != visible_children.end(); ++i) canvas->DrawLineInt(color, i->x(), bounds.y(), i->x(), bounds.bottom()); Hopefully, that makes both these functions simpler than they used to be.
http://codereview.chromium.org/6349101/diff/28024/chrome/browser/ui/views/aut... File chrome/browser/ui/views/autocomplete/touch_autocomplete_popup_contents_view.cc (right): http://codereview.chromium.org/6349101/diff/28024/chrome/browser/ui/views/aut... chrome/browser/ui/views/autocomplete/touch_autocomplete_popup_contents_view.cc:94: int visible_child_count = GetVisibleChildCount(); On 2011/02/18 01:07:59, Peter Kasting wrote: > On 2011/02/18 00:16:34, varunjain wrote: > > This is fine but IMO, this only makes the code hard to read without > > gaining much. > > All right, then how about this: > > Take what are currently the first six lines of LayoutChildren() and factor them > back out to a private helper, "std::vector<Views*> GetVisibleChildren();". Then > rewrite this block as: > > std::vector<Views*> visible_children(GetVisibleChildren()); > if (visible_children.size() < 2) > return; > SkColor color = AutocompleteResultView::GetColor( > AutocompleteResultView::NORMAL, AutocompleteResultView::DIMMED_TEXT); > gfx::Rect bounds(GetContentsBounds()); > for (std::vector<Views*>::const_iterator i(visible_children.begin() + 1); > i != visible_children.end(); ++i) > canvas->DrawLineInt(color, i->x(), bounds.y(), i->x(), bounds.bottom()); > > Hopefully, that makes both these functions simpler than they used to be. Done.
LGTM http://codereview.chromium.org/6349101/diff/35001/chrome/browser/ui/views/aut... File chrome/browser/ui/views/autocomplete/touch_autocomplete_popup_contents_view.cc (right): http://codereview.chromium.org/6349101/diff/35001/chrome/browser/ui/views/aut... chrome/browser/ui/views/autocomplete/touch_autocomplete_popup_contents_view.cc:101: bounds.bottom()); Nit: Line this up with the (, since you have the room to do so without taking any extra lines.
http://codereview.chromium.org/6349101/diff/35001/chrome/browser/ui/views/aut... File chrome/browser/ui/views/autocomplete/touch_autocomplete_popup_contents_view.cc (right): http://codereview.chromium.org/6349101/diff/35001/chrome/browser/ui/views/aut... chrome/browser/ui/views/autocomplete/touch_autocomplete_popup_contents_view.cc:101: bounds.bottom()); On 2011/02/18 18:18:08, Peter Kasting wrote: > Nit: Line this up with the (, since you have the room to do so without taking > any extra lines. Done. |
