Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
10384007:
First stab at touch optimized omnibox auto-complete per sgabriel's mocks. (Closed)
|
Created:
8 years, 7 months ago by Jói Modified:
8 years, 7 months ago CC:
chromium-reviews, James Su, varunjain, James Cook Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionFirst stab at touch optimized omnibox auto-complete per sgabriel's mocks.
This builds on varunjain@'s work from r75157 and r75414 that
refactored the auto-complete classes to allow subclasses like this,
and introduced a subclass based on older mocks. I think the code had
at some point been disconnected and it had bitrotted a little, but
mostly it still worked.
BUG=126132, 125547
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=137009
Patch Set 1 #Patch Set 2 : . #
Total comments: 21
Patch Set 3 : Address review comments. #
Total comments: 20
Patch Set 4 : Address review comments. #Patch Set 5 : Remove some redundant code. #Patch Set 6 : Use stronger color. #
Total comments: 18
Patch Set 7 : Merge to LKGR (pure merge). #Patch Set 8 : Respond to review comments. #
Total comments: 16
Patch Set 9 : Address review comments. #Messages
Total messages: 21 (0 generated)
Peter should review this one.
http://codereview.chromium.org/10384007/diff/10/chrome/browser/ui/views/autoc... File chrome/browser/ui/views/autocomplete/autocomplete_popup_contents_view.cc (right): http://codereview.chromium.org/10384007/diff/10/chrome/browser/ui/views/autoc... chrome/browser/ui/views/autocomplete/autocomplete_popup_contents_view.cc:26: #include "ui/base/layout.h" Nit: I think this goes below the next #include? http://codereview.chromium.org/10384007/diff/10/chrome/browser/ui/views/autoc... chrome/browser/ui/views/autocomplete/autocomplete_popup_contents_view.cc:83: AutocompletePopupContentsView::CreateForEnvironment( Nit: Indent 4 http://codereview.chromium.org/10384007/diff/10/chrome/browser/ui/views/autoc... chrome/browser/ui/views/autocomplete/autocomplete_popup_contents_view.cc:89: switch (ui::GetDisplayLayout()) { Nit: Use if/else rather than switch http://codereview.chromium.org/10384007/diff/10/chrome/browser/ui/views/autoc... File chrome/browser/ui/views/autocomplete/autocomplete_result_view.h (right): http://codereview.chromium.org/10384007/diff/10/chrome/browser/ui/views/autoc... chrome/browser/ui/views/autocomplete/autocomplete_result_view.h:92: // Default values cached here, may be overridden by subclasses. Nit: The Google style guide says to use overrideable accessors or else setters rather than exposing data members to subclasses directly. http://codereview.chromium.org/10384007/diff/10/chrome/browser/ui/views/autoc... File chrome/browser/ui/views/autocomplete/touch_autocomplete_popup_contents_view.cc (right): http://codereview.chromium.org/10384007/diff/10/chrome/browser/ui/views/autoc... chrome/browser/ui/views/autocomplete/touch_autocomplete_popup_contents_view.cc:18: // Colors for a light-gray 3D-looking divider made up of two lines. It's not safe to use hardcoded colors because everything else in the omnibox uses shades of system colors. You can try to adjust these based on the background color but it's going to be tricky to make this look right. Perhaps you want to use some of the predefined system colors for 3D objects? Nit: Define these in the function that uses them and use SkColorSetRGB(). http://codereview.chromium.org/10384007/diff/10/chrome/browser/ui/views/autoc... chrome/browser/ui/views/autocomplete/touch_autocomplete_popup_contents_view.cc:51: if (!description) Nit: if (match.description.empty()) { ... } else { ... } This avoids the temp and shortens this stuff a bit http://codereview.chromium.org/10384007/diff/10/chrome/browser/ui/views/autoc... chrome/browser/ui/views/autocomplete/touch_autocomplete_popup_contents_view.cc:71: // need to adjust the bounds when there are two lines. Don't we need to do this all the time, not just when there's a description? Otherwise won't the icon paint in the top half of the box when we have a single centered line? http://codereview.chromium.org/10384007/diff/10/chrome/browser/ui/views/autoc... chrome/browser/ui/views/autocomplete/touch_autocomplete_popup_contents_view.cc:74: 2 + minimum_text_vertical_padding_); Nit: The linebreak makes it look as if you're dividing by "2 + x" rather than dividing by 2, then adding. Break/indent differently? http://codereview.chromium.org/10384007/diff/10/chrome/browser/ui/views/autoc... File chrome/browser/ui/views/autocomplete/touch_autocomplete_popup_contents_view.h (right): http://codereview.chromium.org/10384007/diff/10/chrome/browser/ui/views/autoc... chrome/browser/ui/views/autocomplete/touch_autocomplete_popup_contents_view.h:31: using AutocompleteResultView::GetState; Nit: I tend to prefer avoiding using statements, and I think they may be banned entirely in headers anyway. (Plus it doesn't look like you actually need this in the .cc file?)
Peter, thanks for the thorough review. I wanted to address some of the points you raised right away by email. Will address the nits and upload a new version of the patch when I have a bit more time. Cheers, Jói http://codereview.chromium.org/10384007/diff/10/chrome/browser/ui/views/autoc... File chrome/browser/ui/views/autocomplete/autocomplete_result_view.h (right): http://codereview.chromium.org/10384007/diff/10/chrome/browser/ui/views/autoc... chrome/browser/ui/views/autocomplete/autocomplete_result_view.h:92: // Default values cached here, may be overridden by subclasses. On 2012/05/04 23:33:31, Peter Kasting wrote: > Nit: The Google style guide says to use overrideable accessors or else setters > rather than exposing data members to subclasses directly. Wanted to avoid the virtual methods since these get accessed a lot. I guess I'll make these private and add protected setters to comply with that bit of the style guide. http://codereview.chromium.org/10384007/diff/10/chrome/browser/ui/views/autoc... File chrome/browser/ui/views/autocomplete/touch_autocomplete_popup_contents_view.cc (right): http://codereview.chromium.org/10384007/diff/10/chrome/browser/ui/views/autoc... chrome/browser/ui/views/autocomplete/touch_autocomplete_popup_contents_view.cc:18: // Colors for a light-gray 3D-looking divider made up of two lines. On 2012/05/04 23:33:31, Peter Kasting wrote: > It's not safe to use hardcoded colors because everything else in the omnibox > uses shades of system colors. > > You can try to adjust these based on the background color but it's going to be > tricky to make this look right. Perhaps you want to use some of the predefined > system colors for 3D objects? > > Nit: Define these in the function that uses them and use SkColorSetRGB(). Good point, will look into using the system colors. http://codereview.chromium.org/10384007/diff/10/chrome/browser/ui/views/autoc... chrome/browser/ui/views/autocomplete/touch_autocomplete_popup_contents_view.cc:71: // need to adjust the bounds when there are two lines. On 2012/05/04 23:33:31, Peter Kasting wrote: > Don't we need to do this all the time, not just when there's a description? > Otherwise won't the icon paint in the top half of the box when we have a single > centered line? The default case is that it's centered on the whole results item, i.e. centered on GetContentBounds(). http://codereview.chromium.org/10384007/diff/10/chrome/browser/ui/views/autoc... File chrome/browser/ui/views/autocomplete/touch_autocomplete_popup_contents_view.h (right): http://codereview.chromium.org/10384007/diff/10/chrome/browser/ui/views/autoc... chrome/browser/ui/views/autocomplete/touch_autocomplete_popup_contents_view.h:31: using AutocompleteResultView::GetState; On 2012/05/04 23:33:31, Peter Kasting wrote: > Nit: I tend to prefer avoiding using statements, and I think they may be banned > entirely in headers anyway. (Plus it doesn't look like you actually need this > in the .cc file?) This one is just to make the method public in this subclass instead of protected as it is in the base. There are two other alternatives, I felt this one was neatest. The others are: a) Add a [ friend class TouchAutocompletePopupContentsView ] declaration. This exposes more of the object than necessary; b) Spell out what the using clause does, i.e. add a method like AutocompleteResultView::ResultViewState GetState() { return AutocompleteResultView::GetState(); } which I'm pretty sure is semantically identical to the using statement, just more statements of code and more work to maintain (when the function signature changes). We've been using this type of using statement in several places in the code where subclasses (especially but I think not exclusively testing subclasses) need to change the access to some members.
Please take another look, I've addressed all comments. Cheers, Jói http://codereview.chromium.org/10384007/diff/10/chrome/browser/ui/views/autoc... File chrome/browser/ui/views/autocomplete/autocomplete_popup_contents_view.cc (right): http://codereview.chromium.org/10384007/diff/10/chrome/browser/ui/views/autoc... chrome/browser/ui/views/autocomplete/autocomplete_popup_contents_view.cc:26: #include "ui/base/layout.h" On 2012/05/04 23:33:31, Peter Kasting wrote: > Nit: I think this goes below the next #include? Fixed here and checked other files to be sure there were no other instances of this. http://codereview.chromium.org/10384007/diff/10/chrome/browser/ui/views/autoc... chrome/browser/ui/views/autocomplete/autocomplete_popup_contents_view.cc:83: AutocompletePopupContentsView::CreateForEnvironment( On 2012/05/04 23:33:31, Peter Kasting wrote: > Nit: Indent 4 Done. http://codereview.chromium.org/10384007/diff/10/chrome/browser/ui/views/autoc... chrome/browser/ui/views/autocomplete/autocomplete_popup_contents_view.cc:89: switch (ui::GetDisplayLayout()) { On 2012/05/04 23:33:31, Peter Kasting wrote: > Nit: Use if/else rather than switch Done. http://codereview.chromium.org/10384007/diff/10/chrome/browser/ui/views/autoc... File chrome/browser/ui/views/autocomplete/autocomplete_result_view.h (right): http://codereview.chromium.org/10384007/diff/10/chrome/browser/ui/views/autoc... chrome/browser/ui/views/autocomplete/autocomplete_result_view.h:92: // Default values cached here, may be overridden by subclasses. On 2012/05/04 23:33:31, Peter Kasting wrote: > Nit: The Google style guide says to use overrideable accessors or else setters > rather than exposing data members to subclasses directly. Done. http://codereview.chromium.org/10384007/diff/10/chrome/browser/ui/views/autoc... File chrome/browser/ui/views/autocomplete/touch_autocomplete_popup_contents_view.cc (right): http://codereview.chromium.org/10384007/diff/10/chrome/browser/ui/views/autoc... chrome/browser/ui/views/autocomplete/touch_autocomplete_popup_contents_view.cc:18: // Colors for a light-gray 3D-looking divider made up of two lines. On 2012/05/04 23:33:31, Peter Kasting wrote: > It's not safe to use hardcoded colors because everything else in the omnibox > uses shades of system colors. > > You can try to adjust these based on the background color but it's going to be > tricky to make this look right. Perhaps you want to use some of the predefined > system colors for 3D objects? > > Nit: Define these in the function that uses them and use SkColorSetRGB(). I've switched to using system colors, am waiting to hear from sgabriel@ on which colors to use on Windows, and which hard-coded values to use on Aura. http://codereview.chromium.org/10384007/diff/10/chrome/browser/ui/views/autoc... chrome/browser/ui/views/autocomplete/touch_autocomplete_popup_contents_view.cc:51: if (!description) On 2012/05/04 23:33:31, Peter Kasting wrote: > Nit: if (match.description.empty()) { ... } else { ... } > > This avoids the temp and shortens this stuff a bit Done. http://codereview.chromium.org/10384007/diff/10/chrome/browser/ui/views/autoc... chrome/browser/ui/views/autocomplete/touch_autocomplete_popup_contents_view.cc:71: // need to adjust the bounds when there are two lines. On 2012/05/04 23:33:31, Peter Kasting wrote: > Don't we need to do this all the time, not just when there's a description? > Otherwise won't the icon paint in the top half of the box when we have a single > centered line? Added to the comment to explain why we don't need to do anything in the two-line case. http://codereview.chromium.org/10384007/diff/10/chrome/browser/ui/views/autoc... chrome/browser/ui/views/autocomplete/touch_autocomplete_popup_contents_view.cc:74: 2 + minimum_text_vertical_padding_); On 2012/05/04 23:33:31, Peter Kasting wrote: > Nit: The linebreak makes it look as if you're dividing by "2 + x" rather than > dividing by 2, then adding. Break/indent differently? Done.
http://codereview.chromium.org/10384007/diff/8001/chrome/browser/ui/views/aut... File chrome/browser/ui/views/autocomplete/autocomplete_popup_contents_view.h (right): http://codereview.chromium.org/10384007/diff/8001/chrome/browser/ui/views/aut... chrome/browser/ui/views/autocomplete/autocomplete_popup_contents_view.h:43: AutocompletePopupContentsView(const gfx::Font& font, Nit: This can become protected. http://codereview.chromium.org/10384007/diff/8001/chrome/browser/ui/views/aut... chrome/browser/ui/views/autocomplete/autocomplete_popup_contents_view.h:49: // Call immediately after construction. Nit: This can become private. http://codereview.chromium.org/10384007/diff/8001/chrome/browser/ui/views/aut... File chrome/browser/ui/views/autocomplete/autocomplete_result_view.cc (right): http://codereview.chromium.org/10384007/diff/8001/chrome/browser/ui/views/aut... chrome/browser/ui/views/autocomplete/autocomplete_result_view.cc:36: const int kMinimumTextVerticalPadding = 3; // Default value, may be overridden. Nit: Comment unnecessary given your comments on the members. http://codereview.chromium.org/10384007/diff/8001/chrome/browser/ui/views/aut... chrome/browser/ui/views/autocomplete/autocomplete_result_view.cc:549: edge_item_padding_ + ((icon->width() == default_icon_size_) ? 0 : Nit: Wrap these lines as they were before (maintains linebreak after ? instead of :, as well as more clarity that the "edge_item_padding_ +" portion is not included in the ?: statement) http://codereview.chromium.org/10384007/diff/8001/chrome/browser/ui/views/aut... chrome/browser/ui/views/autocomplete/autocomplete_result_view.cc:593: canvas->DrawBitmapInt( Nit: Better than either old or new wrapping would be: canvas->DrawBitmapInt(*GetIcon(), GetMirroredXForRect(icon_bounds_), icon_bounds_.y()); http://codereview.chromium.org/10384007/diff/8001/chrome/browser/ui/views/aut... File chrome/browser/ui/views/autocomplete/touch_autocomplete_popup_contents_view.cc (right): http://codereview.chromium.org/10384007/diff/8001/chrome/browser/ui/views/aut... chrome/browser/ui/views/autocomplete/touch_autocomplete_popup_contents_view.cc:24: top_color = color_utils::GetSysSkColor(COLOR_3DDKSHADOW); Make sure that the colors you choose are guaranteed to look good with COLOR_WINDOW as the background; I think the values you've chosen are probably only guaranteed to look good with COLOR_3DFACE as the background. In fact I may have misled you by suggesting system colors at all because I bet there's nothing that really means what you want. Instead, you may need to do this by calling LocationBarView::GetColor(ToolbarModel::NONE, LocationBarView::BACKGROUND) and tinting the results. Make sure to take into account what to do on full-white and full-black backgrounds as well as whether you want the light-vs.-dark colors you use to be swapped on light-vs.-dark backgrounds. http://codereview.chromium.org/10384007/diff/8001/chrome/browser/ui/views/aut... chrome/browser/ui/views/autocomplete/touch_autocomplete_popup_contents_view.cc:42: // Colors for a light-gray 3D-looking divider made up of two lines. Nit: This can be removed http://codereview.chromium.org/10384007/diff/8001/chrome/browser/ui/views/aut... chrome/browser/ui/views/autocomplete/touch_autocomplete_popup_contents_view.cc:92: // We show the icon centered on the first line of content, so we Nit: We show -> we want to show http://codereview.chromium.org/10384007/diff/8001/chrome/browser/ui/views/aut... chrome/browser/ui/views/autocomplete/touch_autocomplete_popup_contents_view.cc:95: // which places th eicon correctly when there are two lines. Nit: th eicon -> the icon; are two lines -> is one line I confess, though, now that I understand how you're trying to lay things out, I wonder if it will look weird. This will result in the icons not being evenly spaced from each other. It seems like it'd be better to either always center the icon within the entire view, or else always place it on the "first line", and if there's only one line of text, place that on the "first line" as well instead of centering it vertically. Either of these will result in evenly spaced icons and look better IMO. http://codereview.chromium.org/10384007/diff/8001/chrome/browser/ui/views/aut... chrome/browser/ui/views/autocomplete/touch_autocomplete_popup_contents_view.cc:131: for (std::vector<View*>::const_iterator i(visible_children.begin() + 1); Nit: I think it would be slightly better to iterate over all children, and draw lines (conditionally) at the top and bottom of the current view instead of at the (top - 1) and top of the current view. While this means we'd need to add conditionals to check for being on the first and last views, it would also make it more natural to say "don't paint either line if we're hovered/selected". Your current code paints the "top color" line in all cases, which seems like a bug, and while it's fixable without reworking the loop structure, this way seemed clearer to me. http://codereview.chromium.org/10384007/diff/8001/chrome/browser/ui/views/aut... File chrome/browser/ui/views/autocomplete/touch_autocomplete_popup_contents_view.h (right): http://codereview.chromium.org/10384007/diff/8001/chrome/browser/ui/views/aut... chrome/browser/ui/views/autocomplete/touch_autocomplete_popup_contents_view.h:31: using AutocompleteResultView::GetState; Nit: Now that I understand what you're doing here, I think you should instead make the parent class method public. This is not only the least code, but it makes it the most clear to maintainers that the method is actually publicly accessible (at least by some potential callers). I don't know if there are style rules about it but I think it's a bad idea for a subclass to change the visibility of a parent member except to make it more private.
Hi, sorry, forgot to update my auto-responder to say that I would be on a plane all day (I'm visiting MTV). Will address these tomorrow once I'm in the office. Cheers, Jói On Mon, May 7, 2012 at 9:03 PM, <pkasting@chromium.org> wrote: > > http://codereview.chromium.org/10384007/diff/8001/chrome/browser/ui/views/aut... > File > chrome/browser/ui/views/autocomplete/autocomplete_popup_contents_view.h > (right): > > http://codereview.chromium.org/10384007/diff/8001/chrome/browser/ui/views/aut... > chrome/browser/ui/views/autocomplete/autocomplete_popup_contents_view.h:43: > AutocompletePopupContentsView(const gfx::Font& font, > Nit: This can become protected. > > http://codereview.chromium.org/10384007/diff/8001/chrome/browser/ui/views/aut... > chrome/browser/ui/views/autocomplete/autocomplete_popup_contents_view.h:49: > // Call immediately after construction. > Nit: This can become private. > > http://codereview.chromium.org/10384007/diff/8001/chrome/browser/ui/views/aut... > File chrome/browser/ui/views/autocomplete/autocomplete_result_view.cc > (right): > > http://codereview.chromium.org/10384007/diff/8001/chrome/browser/ui/views/aut... > chrome/browser/ui/views/autocomplete/autocomplete_result_view.cc:36: > const int kMinimumTextVerticalPadding = 3; // Default value, may be > overridden. > Nit: Comment unnecessary given your comments on the members. > > http://codereview.chromium.org/10384007/diff/8001/chrome/browser/ui/views/aut... > chrome/browser/ui/views/autocomplete/autocomplete_result_view.cc:549: > edge_item_padding_ + ((icon->width() == default_icon_size_) ? 0 : > Nit: Wrap these lines as they were before (maintains linebreak after ? > instead of :, as well as more clarity that the "edge_item_padding_ +" > portion is not included in the ?: statement) > > http://codereview.chromium.org/10384007/diff/8001/chrome/browser/ui/views/aut... > chrome/browser/ui/views/autocomplete/autocomplete_result_view.cc:593: > canvas->DrawBitmapInt( > Nit: Better than either old or new wrapping would be: > > canvas->DrawBitmapInt(*GetIcon(), GetMirroredXForRect(icon_bounds_), > icon_bounds_.y()); > > http://codereview.chromium.org/10384007/diff/8001/chrome/browser/ui/views/aut... > File > chrome/browser/ui/views/autocomplete/touch_autocomplete_popup_contents_view.cc > (right): > > http://codereview.chromium.org/10384007/diff/8001/chrome/browser/ui/views/aut... > chrome/browser/ui/views/autocomplete/touch_autocomplete_popup_contents_view.cc:24: > top_color = color_utils::GetSysSkColor(COLOR_3DDKSHADOW); > Make sure that the colors you choose are guaranteed to look good with > COLOR_WINDOW as the background; I think the values you've chosen are > probably only guaranteed to look good with COLOR_3DFACE as the > background. In fact I may have misled you by suggesting system colors > at all because I bet there's nothing that really means what you want. > > Instead, you may need to do this by calling > LocationBarView::GetColor(ToolbarModel::NONE, > LocationBarView::BACKGROUND) and tinting the results. Make sure to take > into account what to do on full-white and full-black backgrounds as well > as whether you want the light-vs.-dark colors you use to be swapped on > light-vs.-dark backgrounds. > > http://codereview.chromium.org/10384007/diff/8001/chrome/browser/ui/views/aut... > chrome/browser/ui/views/autocomplete/touch_autocomplete_popup_contents_view.cc:42: > > // Colors for a light-gray 3D-looking divider made up of two lines. > Nit: This can be removed > > http://codereview.chromium.org/10384007/diff/8001/chrome/browser/ui/views/aut... > chrome/browser/ui/views/autocomplete/touch_autocomplete_popup_contents_view.cc:92: > // We show the icon centered on the first line of content, so we > Nit: We show -> we want to show > > http://codereview.chromium.org/10384007/diff/8001/chrome/browser/ui/views/aut... > chrome/browser/ui/views/autocomplete/touch_autocomplete_popup_contents_view.cc:95: > // which places th eicon correctly when there are two lines. > Nit: th eicon -> the icon; are two lines -> is one line > > I confess, though, now that I understand how you're trying to lay things > out, I wonder if it will look weird. This will result in the icons not > being evenly spaced from each other. It seems like it'd be better to > either always center the icon within the entire view, or else always > place it on the "first line", and if there's only one line of text, > place that on the "first line" as well instead of centering it > vertically. Either of these will result in evenly spaced icons and look > better IMO. > > http://codereview.chromium.org/10384007/diff/8001/chrome/browser/ui/views/aut... > chrome/browser/ui/views/autocomplete/touch_autocomplete_popup_contents_view.cc:131: > for (std::vector<View*>::const_iterator i(visible_children.begin() + 1); > Nit: I think it would be slightly better to iterate over all children, > and draw lines (conditionally) at the top and bottom of the current view > instead of at the (top - 1) and top of the current view. > > While this means we'd need to add conditionals to check for being on the > first and last views, it would also make it more natural to say "don't > paint either line if we're hovered/selected". Your current code paints > the "top color" line in all cases, which seems like a bug, and while > it's fixable without reworking the loop structure, this way seemed > clearer to me. > > http://codereview.chromium.org/10384007/diff/8001/chrome/browser/ui/views/aut... > File > chrome/browser/ui/views/autocomplete/touch_autocomplete_popup_contents_view.h > (right): > > http://codereview.chromium.org/10384007/diff/8001/chrome/browser/ui/views/aut... > > chrome/browser/ui/views/autocomplete/touch_autocomplete_popup_contents_view.h:31: > using AutocompleteResultView::GetState; > Nit: Now that I understand what you're doing here, I think you should > instead make the parent class method public. > > This is not only the least code, but it makes it the most clear to > maintainers that the method is actually publicly accessible (at least by > some potential callers). I don't know if there are style rules about it > but I think it's a bad idea for a subclass to change the visibility of a > parent member except to make it more private. > > http://codereview.chromium.org/10384007/
Remove some redundant code.
Hi Peter, Please take another look. I believe this addresses the stuff we talked about in person, as well as the issues listed below. One thing I'm currently doing which is different from what we talked about, is using the text color as the basis for the separators, rather than the border color. I'm doing this because (a) it turns out the border color isn't a color theme color but rather comes from a bitmap (that may be impacted by whichever theme you're using, I'm not sure), and there isn't a straight-forward way to retrieve this color. Let me know what you think. I attached screenshots to http://crbug.com/126132 and would be happy as well to show this to you in person. One note: I wasn't sure how to test the EV cert state of the omnibox, but looking at the code it seems to share layout with the tab-to-search functionality, which I did test to make sure padding around the icons is consistent. I'm hoping to get this landed soon in something close to the current state so that it makes it into the builds folks are using to play with Metro, and then continue tweaking. I will be getting updated assets (larger omnibox icons, maybe some other changes) from Sebastien today or tomorrow so some tweaks will be required at that time, but I'd rather land this as an interim step. Cheers, Jói http://codereview.chromium.org/10384007/diff/8001/chrome/browser/ui/views/aut... File chrome/browser/ui/views/autocomplete/autocomplete_popup_contents_view.h (right): http://codereview.chromium.org/10384007/diff/8001/chrome/browser/ui/views/aut... chrome/browser/ui/views/autocomplete/autocomplete_popup_contents_view.h:43: AutocompletePopupContentsView(const gfx::Font& font, On 2012/05/07 21:03:12, Peter Kasting wrote: > Nit: This can become protected. Done. http://codereview.chromium.org/10384007/diff/8001/chrome/browser/ui/views/aut... chrome/browser/ui/views/autocomplete/autocomplete_popup_contents_view.h:49: // Call immediately after construction. On 2012/05/07 21:03:12, Peter Kasting wrote: > Nit: This can become private. Done. http://codereview.chromium.org/10384007/diff/8001/chrome/browser/ui/views/aut... File chrome/browser/ui/views/autocomplete/autocomplete_result_view.cc (right): http://codereview.chromium.org/10384007/diff/8001/chrome/browser/ui/views/aut... chrome/browser/ui/views/autocomplete/autocomplete_result_view.cc:36: const int kMinimumTextVerticalPadding = 3; // Default value, may be overridden. On 2012/05/07 21:03:12, Peter Kasting wrote: > Nit: Comment unnecessary given your comments on the members. Done. http://codereview.chromium.org/10384007/diff/8001/chrome/browser/ui/views/aut... chrome/browser/ui/views/autocomplete/autocomplete_result_view.cc:549: edge_item_padding_ + ((icon->width() == default_icon_size_) ? 0 : On 2012/05/07 21:03:12, Peter Kasting wrote: > Nit: Wrap these lines as they were before (maintains linebreak after ? instead > of :, as well as more clarity that the "edge_item_padding_ +" portion is not > included in the ?: statement) Went one step further and separated it out as a temporary, it was very hard to read regardless of indentation: int icon_padding = (icon->width() == default_icon_size_) ? 0 : LocationBarView::kIconInternalPadding; icon_bounds_.SetRect(edge_item_padding_ + icon_padding, (height() - icon->height()) / 2, icon->width(), icon->height()); http://codereview.chromium.org/10384007/diff/8001/chrome/browser/ui/views/aut... chrome/browser/ui/views/autocomplete/autocomplete_result_view.cc:593: canvas->DrawBitmapInt( On 2012/05/07 21:03:12, Peter Kasting wrote: > Nit: Better than either old or new wrapping would be: > > canvas->DrawBitmapInt(*GetIcon(), GetMirroredXForRect(icon_bounds_), > icon_bounds_.y()); Done. http://codereview.chromium.org/10384007/diff/8001/chrome/browser/ui/views/aut... File chrome/browser/ui/views/autocomplete/touch_autocomplete_popup_contents_view.cc (right): http://codereview.chromium.org/10384007/diff/8001/chrome/browser/ui/views/aut... chrome/browser/ui/views/autocomplete/touch_autocomplete_popup_contents_view.cc:24: top_color = color_utils::GetSysSkColor(COLOR_3DDKSHADOW); On 2012/05/07 21:03:12, Peter Kasting wrote: > Make sure that the colors you choose are guaranteed to look good with > COLOR_WINDOW as the background; I think the values you've chosen are probably > only guaranteed to look good with COLOR_3DFACE as the background. In fact I may > have misled you by suggesting system colors at all because I bet there's nothing > that really means what you want. > > Instead, you may need to do this by calling > LocationBarView::GetColor(ToolbarModel::NONE, LocationBarView::BACKGROUND) and > tinting the results. Make sure to take into account what to do on full-white > and full-black backgrounds as well as whether you want the light-vs.-dark colors > you use to be swapped on light-vs.-dark backgrounds. Done. http://codereview.chromium.org/10384007/diff/8001/chrome/browser/ui/views/aut... chrome/browser/ui/views/autocomplete/touch_autocomplete_popup_contents_view.cc:24: top_color = color_utils::GetSysSkColor(COLOR_3DDKSHADOW); On 2012/05/07 21:03:12, Peter Kasting wrote: > Make sure that the colors you choose are guaranteed to look good with > COLOR_WINDOW as the background; I think the values you've chosen are probably > only guaranteed to look good with COLOR_3DFACE as the background. In fact I may > have misled you by suggesting system colors at all because I bet there's nothing > that really means what you want. > > Instead, you may need to do this by calling > LocationBarView::GetColor(ToolbarModel::NONE, LocationBarView::BACKGROUND) and > tinting the results. Make sure to take into account what to do on full-white > and full-black backgrounds as well as whether you want the light-vs.-dark colors > you use to be swapped on light-vs.-dark backgrounds. Done. http://codereview.chromium.org/10384007/diff/8001/chrome/browser/ui/views/aut... chrome/browser/ui/views/autocomplete/touch_autocomplete_popup_contents_view.cc:42: // Colors for a light-gray 3D-looking divider made up of two lines. On 2012/05/07 21:03:12, Peter Kasting wrote: > Nit: This can be removed Done. http://codereview.chromium.org/10384007/diff/8001/chrome/browser/ui/views/aut... File chrome/browser/ui/views/autocomplete/touch_autocomplete_popup_contents_view.h (right): http://codereview.chromium.org/10384007/diff/8001/chrome/browser/ui/views/aut... chrome/browser/ui/views/autocomplete/touch_autocomplete_popup_contents_view.h:31: using AutocompleteResultView::GetState; On 2012/05/07 21:03:12, Peter Kasting wrote: > Nit: Now that I understand what you're doing here, I think you should instead > make the parent class method public. > > This is not only the least code, but it makes it the most clear to maintainers > that the method is actually publicly accessible (at least by some potential > callers). I don't know if there are style rules about it but I think it's a bad > idea for a subclass to change the visibility of a parent member except to make > it more private. Done.
Friendly ping :) On Thu, May 10, 2012 at 11:33 AM, <joi@chromium.org> wrote: > Hi Peter, > > Please take another look. > > I believe this addresses the stuff we talked about in person, as well as the > issues listed below. One thing I'm currently doing which is different from > what > we talked about, is using the text color as the basis for the separators, > rather > than the border color. I'm doing this because (a) it turns out the border > color > isn't a color theme color but rather comes from a bitmap (that may be > impacted > by whichever theme you're using, I'm not sure), and there isn't a > straight-forward way to retrieve this color. > > Let me know what you think. I attached screenshots to > http://crbug.com/126132 > and would be happy as well to show this to you in person. > > One note: I wasn't sure how to test the EV cert state of the omnibox, but > looking at the code it seems to share layout with the tab-to-search > functionality, which I did test to make sure padding around the icons is > consistent. > > I'm hoping to get this landed soon in something close to the current state > so > that it makes it into the builds folks are using to play with Metro, and > then > continue tweaking. I will be getting updated assets (larger omnibox icons, > maybe some other changes) from Sebastien today or tomorrow so some tweaks > will > be required at that time, but I'd rather land this as an interim step. > > Cheers, > Jói > > > > > http://codereview.chromium.org/10384007/diff/8001/chrome/browser/ui/views/aut... > File > chrome/browser/ui/views/autocomplete/autocomplete_popup_contents_view.h > (right): > > http://codereview.chromium.org/10384007/diff/8001/chrome/browser/ui/views/aut... > chrome/browser/ui/views/autocomplete/autocomplete_popup_contents_view.h:43: > AutocompletePopupContentsView(const gfx::Font& font, > On 2012/05/07 21:03:12, Peter Kasting wrote: >> >> Nit: This can become protected. > > > Done. > > > http://codereview.chromium.org/10384007/diff/8001/chrome/browser/ui/views/aut... > chrome/browser/ui/views/autocomplete/autocomplete_popup_contents_view.h:49: > // Call immediately after construction. > On 2012/05/07 21:03:12, Peter Kasting wrote: >> >> Nit: This can become private. > > > Done. > > > http://codereview.chromium.org/10384007/diff/8001/chrome/browser/ui/views/aut... > File chrome/browser/ui/views/autocomplete/autocomplete_result_view.cc > (right): > > http://codereview.chromium.org/10384007/diff/8001/chrome/browser/ui/views/aut... > chrome/browser/ui/views/autocomplete/autocomplete_result_view.cc:36: > const int kMinimumTextVerticalPadding = 3; // Default value, may be > overridden. > On 2012/05/07 21:03:12, Peter Kasting wrote: >> >> Nit: Comment unnecessary given your comments on the members. > > > Done. > > > http://codereview.chromium.org/10384007/diff/8001/chrome/browser/ui/views/aut... > chrome/browser/ui/views/autocomplete/autocomplete_result_view.cc:549: > edge_item_padding_ + ((icon->width() == default_icon_size_) ? 0 : > On 2012/05/07 21:03:12, Peter Kasting wrote: >> >> Nit: Wrap these lines as they were before (maintains linebreak after ? > > instead >> >> of :, as well as more clarity that the "edge_item_padding_ +" portion > > is not >> >> included in the ?: statement) > > > Went one step further and separated it out as a temporary, it was very > hard to read regardless of indentation: > > > int icon_padding = (icon->width() == default_icon_size_) ? > 0 : LocationBarView::kIconInternalPadding; > icon_bounds_.SetRect(edge_item_padding_ + icon_padding, > (height() - icon->height()) / 2, > icon->width(), > icon->height()); > > > http://codereview.chromium.org/10384007/diff/8001/chrome/browser/ui/views/aut... > chrome/browser/ui/views/autocomplete/autocomplete_result_view.cc:593: > canvas->DrawBitmapInt( > On 2012/05/07 21:03:12, Peter Kasting wrote: >> >> Nit: Better than either old or new wrapping would be: > > >> canvas->DrawBitmapInt(*GetIcon(), > > GetMirroredXForRect(icon_bounds_), >> >> icon_bounds_.y()); > > > Done. > > > http://codereview.chromium.org/10384007/diff/8001/chrome/browser/ui/views/aut... > File > chrome/browser/ui/views/autocomplete/touch_autocomplete_popup_contents_view.cc > (right): > > http://codereview.chromium.org/10384007/diff/8001/chrome/browser/ui/views/aut... > chrome/browser/ui/views/autocomplete/touch_autocomplete_popup_contents_view.cc:24: > top_color = color_utils::GetSysSkColor(COLOR_3DDKSHADOW); > On 2012/05/07 21:03:12, Peter Kasting wrote: >> >> Make sure that the colors you choose are guaranteed to look good with >> COLOR_WINDOW as the background; I think the values you've chosen are > > probably >> >> only guaranteed to look good with COLOR_3DFACE as the background. In > > fact I may >> >> have misled you by suggesting system colors at all because I bet > > there's nothing >> >> that really means what you want. > > >> Instead, you may need to do this by calling >> LocationBarView::GetColor(ToolbarModel::NONE, > > LocationBarView::BACKGROUND) and >> >> tinting the results. Make sure to take into account what to do on > > full-white >> >> and full-black backgrounds as well as whether you want the > > light-vs.-dark colors >> >> you use to be swapped on light-vs.-dark backgrounds. > > > Done. > > > http://codereview.chromium.org/10384007/diff/8001/chrome/browser/ui/views/aut... > chrome/browser/ui/views/autocomplete/touch_autocomplete_popup_contents_view.cc:24: > top_color = color_utils::GetSysSkColor(COLOR_3DDKSHADOW); > On 2012/05/07 21:03:12, Peter Kasting wrote: >> >> Make sure that the colors you choose are guaranteed to look good with >> COLOR_WINDOW as the background; I think the values you've chosen are > > probably >> >> only guaranteed to look good with COLOR_3DFACE as the background. In > > fact I may >> >> have misled you by suggesting system colors at all because I bet > > there's nothing >> >> that really means what you want. > > >> Instead, you may need to do this by calling >> LocationBarView::GetColor(ToolbarModel::NONE, > > LocationBarView::BACKGROUND) and >> >> tinting the results. Make sure to take into account what to do on > > full-white >> >> and full-black backgrounds as well as whether you want the > > light-vs.-dark colors >> >> you use to be swapped on light-vs.-dark backgrounds. > > > Done. > > > http://codereview.chromium.org/10384007/diff/8001/chrome/browser/ui/views/aut... > chrome/browser/ui/views/autocomplete/touch_autocomplete_popup_contents_view.cc:42: > // Colors for a light-gray 3D-looking divider made up of two lines. > On 2012/05/07 21:03:12, Peter Kasting wrote: >> >> Nit: This can be removed > > > Done. > > > http://codereview.chromium.org/10384007/diff/8001/chrome/browser/ui/views/aut... > File > chrome/browser/ui/views/autocomplete/touch_autocomplete_popup_contents_view.h > (right): > > http://codereview.chromium.org/10384007/diff/8001/chrome/browser/ui/views/aut... > chrome/browser/ui/views/autocomplete/touch_autocomplete_popup_contents_view.h:31: > using AutocompleteResultView::GetState; > On 2012/05/07 21:03:12, Peter Kasting wrote: >> >> Nit: Now that I understand what you're doing here, I think you should > > instead >> >> make the parent class method public. > > >> This is not only the least code, but it makes it the most clear to > > maintainers >> >> that the method is actually publicly accessible (at least by some > > potential >> >> callers). I don't know if there are style rules about it but I think > > it's a bad >> >> idea for a subclass to change the visibility of a parent member except > > to make >> >> it more private. > > > Done. > > http://codereview.chromium.org/10384007/
Use stronger color.
http://codereview.chromium.org/10384007/diff/13017/chrome/browser/ui/views/au... File chrome/browser/ui/views/autocomplete/autocomplete_result_view.cc (right): http://codereview.chromium.org/10384007/diff/13017/chrome/browser/ui/views/au... chrome/browser/ui/views/autocomplete/autocomplete_result_view.cc:180: colors[i][DIVIDER] = color_utils::AlphaBlend(colors[i][TEXT], Nit: How about just: // TODO(joi): Programmatically draw the dropdown border using this color // as well. (Right now it's drawn as black with 25% alpha.) colors[i][DIVIDER] = color_utils::AlphaBlend(colors[i][TEXT], colors[i][BACKGROUND], 0x34); http://codereview.chromium.org/10384007/diff/13017/chrome/browser/ui/views/au... chrome/browser/ui/views/autocomplete/autocomplete_result_view.cc:551: int icon_padding = (icon->width() == default_icon_size_) ? Nit: Frankly I was OK with how it was before, just with |edge_item_padding_| http://codereview.chromium.org/10384007/diff/13017/chrome/browser/ui/views/au... chrome/browser/ui/views/autocomplete/autocomplete_result_view.cc:563: const int kw_collapsed_size = keyword_icon_->width() + Nit: Wrap after '=' instead of '+' http://codereview.chromium.org/10384007/diff/13017/chrome/browser/ui/views/au... chrome/browser/ui/views/autocomplete/autocomplete_result_view.cc:566: const int kw_x = animation_->CurrentValueBetween(max_kw_x, Nit: Wrap after '=' instead of ',' http://codereview.chromium.org/10384007/diff/13017/chrome/browser/ui/views/au... chrome/browser/ui/views/autocomplete/autocomplete_result_view.cc:571: keyword_text_bounds_.SetRect(kw_text_x, 0, std::max( Nit: Wrap before "std::max(" instead of after http://codereview.chromium.org/10384007/diff/13017/chrome/browser/ui/views/au... File chrome/browser/ui/views/autocomplete/autocomplete_result_view.h (right): http://codereview.chromium.org/10384007/diff/13017/chrome/browser/ui/views/au... chrome/browser/ui/views/autocomplete/autocomplete_result_view.h:133: // Default values cached here, may be overridden by subclasses. Nit: "...may be overridden by using the setters above." http://codereview.chromium.org/10384007/diff/13017/chrome/browser/ui/views/au... File chrome/browser/ui/views/autocomplete/touch_autocomplete_popup_contents_view.cc (right): http://codereview.chromium.org/10384007/diff/13017/chrome/browser/ui/views/au... chrome/browser/ui/views/autocomplete/touch_autocomplete_popup_contents_view.cc:87: // at the top of the next child. Uff da. Can we just do normal-colored dividers between each row regardless of selection state? This would make this code simpler, allow us to avoid making GetState() non-private, let us calculate the color here instead of adding a DIVIDER enum value, and probably still look fine (in fact, maybe better, as the current system looks a bit weird). Even if you really really like this color scheme, can we just loop over children (0, n-1) drawing a line at the bottom of each one based on the state of both children? Note that this will also address a "bug" in the current code (IMO) where the border between a selected and a hovered cell matches whichever is on the bottom, where I think there should be a SELECTED > HOVERED > NORMAL precedence order. http://codereview.chromium.org/10384007/diff/13017/chrome/browser/ui/views/lo... File chrome/browser/ui/views/location_bar/location_bar_view.cc (right): http://codereview.chromium.org/10384007/diff/13017/chrome/browser/ui/views/lo... chrome/browser/ui/views/location_bar/location_bar_view.cc:313: if (ui::GetDisplayLayout() == ui::LAYOUT_TOUCH) { Nit: No else after return; just do return (ui::GetDisplayLayout() == ui::LAYOUT_TOUCH) ? kTouchItemPadding : kDesktopItemPadding; (2 places) http://codereview.chromium.org/10384007/diff/13017/chrome/browser/ui/views/lo... File chrome/browser/ui/views/location_bar/location_bar_view.h (right): http://codereview.chromium.org/10384007/diff/13017/chrome/browser/ui/views/lo... chrome/browser/ui/views/location_bar/location_bar_view.h:323: // Space between items in the location bar. Nit: Methods above variables
Will address your nits later, but wanted to address one question right away. Hoping you will LGTM modulo fixing the nits in question so that I can commit over the weekend, right now I have to run to the 5-year GreenBorder acquisition anniversary :) Cheers, Jói http://codereview.chromium.org/10384007/diff/13017/chrome/browser/ui/views/au... File chrome/browser/ui/views/autocomplete/touch_autocomplete_popup_contents_view.cc (right): http://codereview.chromium.org/10384007/diff/13017/chrome/browser/ui/views/au... chrome/browser/ui/views/autocomplete/touch_autocomplete_popup_contents_view.cc:87: // at the top of the next child. On 2012/05/11 23:01:41, Peter Kasting wrote: > Uff da. > > Can we just do normal-colored dividers between each row regardless of selection > state? This would make this code simpler, allow us to avoid making GetState() > non-private, let us calculate the color here instead of adding a DIVIDER enum > value, and probably still look fine (in fact, maybe better, as the current > system looks a bit weird). > > Even if you really really like this color scheme, can we just loop over children > (0, n-1) drawing a line at the bottom of each one based on the state of both > children? Note that this will also address a "bug" in the current code (IMO) > where the border between a selected and a hovered cell matches whichever is on > the bottom, where I think there should be a SELECTED > HOVERED > NORMAL > precedence order. I tried with normal-colored dividers regardless of selection state, and it doesn't look as good. The current code has the precedence order you are suggesting. The reason I sometimes need to draw both top and bottom of the current child (and skip the top of the following child) is to get that precedence order, and to draw the divider on top of the right background color to blend with (if I draw it on the next child it will be blended with that child's background color, which won't look right).
Merge to LKGR (pure merge).
Respond to review comments.
http://codereview.chromium.org/10384007/diff/13017/chrome/browser/ui/views/au... File chrome/browser/ui/views/autocomplete/autocomplete_result_view.cc (right): http://codereview.chromium.org/10384007/diff/13017/chrome/browser/ui/views/au... chrome/browser/ui/views/autocomplete/autocomplete_result_view.cc:180: colors[i][DIVIDER] = color_utils::AlphaBlend(colors[i][TEXT], On 2012/05/11 23:01:41, Peter Kasting wrote: > Nit: How about just: > > // TODO(joi): Programmatically draw the dropdown border using this color > // as well. (Right now it's drawn as black with 25% alpha.) > colors[i][DIVIDER] = > color_utils::AlphaBlend(colors[i][TEXT], colors[i][BACKGROUND], 0x34); Done. http://codereview.chromium.org/10384007/diff/13017/chrome/browser/ui/views/au... chrome/browser/ui/views/autocomplete/autocomplete_result_view.cc:563: const int kw_collapsed_size = keyword_icon_->width() + On 2012/05/11 23:01:41, Peter Kasting wrote: > Nit: Wrap after '=' instead of '+' Done. http://codereview.chromium.org/10384007/diff/13017/chrome/browser/ui/views/au... chrome/browser/ui/views/autocomplete/autocomplete_result_view.cc:566: const int kw_x = animation_->CurrentValueBetween(max_kw_x, On 2012/05/11 23:01:41, Peter Kasting wrote: > Nit: Wrap after '=' instead of ',' Done. http://codereview.chromium.org/10384007/diff/13017/chrome/browser/ui/views/au... chrome/browser/ui/views/autocomplete/autocomplete_result_view.cc:571: keyword_text_bounds_.SetRect(kw_text_x, 0, std::max( On 2012/05/11 23:01:41, Peter Kasting wrote: > Nit: Wrap before "std::max(" instead of after Done. http://codereview.chromium.org/10384007/diff/13017/chrome/browser/ui/views/au... File chrome/browser/ui/views/autocomplete/autocomplete_result_view.h (right): http://codereview.chromium.org/10384007/diff/13017/chrome/browser/ui/views/au... chrome/browser/ui/views/autocomplete/autocomplete_result_view.h:133: // Default values cached here, may be overridden by subclasses. On 2012/05/11 23:01:41, Peter Kasting wrote: > Nit: "...may be overridden by using the setters above." Done. http://codereview.chromium.org/10384007/diff/13017/chrome/browser/ui/views/au... File chrome/browser/ui/views/autocomplete/touch_autocomplete_popup_contents_view.cc (right): http://codereview.chromium.org/10384007/diff/13017/chrome/browser/ui/views/au... chrome/browser/ui/views/autocomplete/touch_autocomplete_popup_contents_view.cc:87: // at the top of the next child. On 2012/05/11 23:01:41, Peter Kasting wrote: > Uff da. > > Can we just do normal-colored dividers between each row regardless of selection > state? This would make this code simpler, allow us to avoid making GetState() > non-private, let us calculate the color here instead of adding a DIVIDER enum > value, and probably still look fine (in fact, maybe better, as the current > system looks a bit weird). > > Even if you really really like this color scheme, can we just loop over children > (0, n-1) drawing a line at the bottom of each one based on the state of both > children? Note that this will also address a "bug" in the current code (IMO) > where the border between a selected and a hovered cell matches whichever is on > the bottom, where I think there should be a SELECTED > HOVERED > NORMAL > precedence order. I've changed this code as per your suggestion, it's a lot simpler now but the visual end result is the same. http://codereview.chromium.org/10384007/diff/13017/chrome/browser/ui/views/lo... File chrome/browser/ui/views/location_bar/location_bar_view.cc (right): http://codereview.chromium.org/10384007/diff/13017/chrome/browser/ui/views/lo... chrome/browser/ui/views/location_bar/location_bar_view.cc:313: if (ui::GetDisplayLayout() == ui::LAYOUT_TOUCH) { On 2012/05/11 23:01:41, Peter Kasting wrote: > Nit: No else after return; just do > > return (ui::GetDisplayLayout() == ui::LAYOUT_TOUCH) ? > kTouchItemPadding : kDesktopItemPadding; > > (2 places) Done. http://codereview.chromium.org/10384007/diff/13017/chrome/browser/ui/views/lo... File chrome/browser/ui/views/location_bar/location_bar_view.h (right): http://codereview.chromium.org/10384007/diff/13017/chrome/browser/ui/views/lo... chrome/browser/ui/views/location_bar/location_bar_view.h:323: // Space between items in the location bar. On 2012/05/11 23:01:41, Peter Kasting wrote: > Nit: Methods above variables Done.
LGTM, remaining comments are trivial formatting http://codereview.chromium.org/10384007/diff/15004/chrome/browser/ui/views/au... File chrome/browser/ui/views/autocomplete/autocomplete_result_view.cc (right): http://codereview.chromium.org/10384007/diff/15004/chrome/browser/ui/views/au... chrome/browser/ui/views/autocomplete/autocomplete_result_view.cc:180: colors[i][DIVIDER] = color_utils::AlphaBlend(colors[i][TEXT], Nit: I still think wrapping after '=' and putting the rest on one line (dropping the 20% comment, there's no magic significance to 20%) would be better. http://codereview.chromium.org/10384007/diff/15004/chrome/browser/ui/views/au... chrome/browser/ui/views/autocomplete/autocomplete_result_view.cc:551: int icon_padding = (icon->width() == default_icon_size_) ? Nit: Still prefer the old code to this http://codereview.chromium.org/10384007/diff/15004/chrome/browser/ui/views/au... chrome/browser/ui/views/autocomplete/autocomplete_result_view.cc:572: kw_text_x, 0, Nit: These two args can go on above line http://codereview.chromium.org/10384007/diff/15004/chrome/browser/ui/views/au... chrome/browser/ui/views/autocomplete/autocomplete_result_view.cc:574: text_height); Nit: This arg can go on above line http://codereview.chromium.org/10384007/diff/15004/chrome/browser/ui/views/au... File chrome/browser/ui/views/autocomplete/touch_autocomplete_popup_contents_view.cc (right): http://codereview.chromium.org/10384007/diff/15004/chrome/browser/ui/views/au... chrome/browser/ui/views/autocomplete/touch_autocomplete_popup_contents_view.cc:93: Nit: Extra newline (2 places) http://codereview.chromium.org/10384007/diff/15004/chrome/browser/ui/views/au... chrome/browser/ui/views/autocomplete/touch_autocomplete_popup_contents_view.cc:95: std::max(child->GetState(), next_child->GetState()); Nit: Can just be inlined into next statement http://codereview.chromium.org/10384007/diff/15004/chrome/browser/ui/views/au... chrome/browser/ui/views/autocomplete/touch_autocomplete_popup_contents_view.cc:102: divider_color); Nit: This arg can fit on above line
Address review comments.
Thanks, committing. http://codereview.chromium.org/10384007/diff/15004/chrome/browser/ui/views/au... File chrome/browser/ui/views/autocomplete/autocomplete_result_view.cc (right): http://codereview.chromium.org/10384007/diff/15004/chrome/browser/ui/views/au... chrome/browser/ui/views/autocomplete/autocomplete_result_view.cc:180: colors[i][DIVIDER] = color_utils::AlphaBlend(colors[i][TEXT], On 2012/05/14 20:16:21, Peter Kasting wrote: > Nit: I still think wrapping after '=' and putting the rest on one line (dropping > the 20% comment, there's no magic significance to 20%) would be better. Done, sorry didn't notice that comment earlier. http://codereview.chromium.org/10384007/diff/15004/chrome/browser/ui/views/au... chrome/browser/ui/views/autocomplete/autocomplete_result_view.cc:551: int icon_padding = (icon->width() == default_icon_size_) ? On 2012/05/14 20:16:21, Peter Kasting wrote: > Nit: Still prefer the old code to this Done. http://codereview.chromium.org/10384007/diff/15004/chrome/browser/ui/views/au... chrome/browser/ui/views/autocomplete/autocomplete_result_view.cc:572: kw_text_x, 0, On 2012/05/14 20:16:21, Peter Kasting wrote: > Nit: These two args can go on above line Done. http://codereview.chromium.org/10384007/diff/15004/chrome/browser/ui/views/au... chrome/browser/ui/views/autocomplete/autocomplete_result_view.cc:574: text_height); On 2012/05/14 20:16:21, Peter Kasting wrote: > Nit: This arg can go on above line Done. http://codereview.chromium.org/10384007/diff/15004/chrome/browser/ui/views/au... File chrome/browser/ui/views/autocomplete/touch_autocomplete_popup_contents_view.cc (right): http://codereview.chromium.org/10384007/diff/15004/chrome/browser/ui/views/au... chrome/browser/ui/views/autocomplete/touch_autocomplete_popup_contents_view.cc:93: On 2012/05/14 20:16:21, Peter Kasting wrote: > Nit: Extra newline (2 places) Done. http://codereview.chromium.org/10384007/diff/15004/chrome/browser/ui/views/au... chrome/browser/ui/views/autocomplete/touch_autocomplete_popup_contents_view.cc:93: On 2012/05/14 20:16:21, Peter Kasting wrote: > Nit: Extra newline (2 places) Done. http://codereview.chromium.org/10384007/diff/15004/chrome/browser/ui/views/au... chrome/browser/ui/views/autocomplete/touch_autocomplete_popup_contents_view.cc:95: std::max(child->GetState(), next_child->GetState()); On 2012/05/14 20:16:21, Peter Kasting wrote: > Nit: Can just be inlined into next statement Done. http://codereview.chromium.org/10384007/diff/15004/chrome/browser/ui/views/au... chrome/browser/ui/views/autocomplete/touch_autocomplete_popup_contents_view.cc:95: std::max(child->GetState(), next_child->GetState()); On 2012/05/14 20:16:21, Peter Kasting wrote: > Nit: Can just be inlined into next statement Done. http://codereview.chromium.org/10384007/diff/15004/chrome/browser/ui/views/au... chrome/browser/ui/views/autocomplete/touch_autocomplete_popup_contents_view.cc:102: divider_color); On 2012/05/14 20:16:21, Peter Kasting wrote: > Nit: This arg can fit on above line Done.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/joi@chromium.org/10384007/21001
Change committed as 137009 |
