Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
11817051:
Elide text in the new Autofill UI (Closed)
|
Created:
7 years, 11 months ago by csharp Modified:
7 years, 11 months ago CC:
chromium-reviews, Raman Kakilate, benquan, dhollowa+watch_chromium.org, ahutter, dbeam+watch-autofill_chromium.org, Dane Wallinga, dyu1, Albert Bodenhamer, estade+watch_chromium.org, Ilya Sherman Base URL:
http://git.chromium.org/chromium/src.git@master Visibility:
Public. |
DescriptionElide text in the new Autofill UI
If the popup would go off the edge of the screen due to the size of the data, the text parts are now elided to make the popup fit into the visible space.
BUG=156163, 164603
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=178149
Patch Set 1 #
Total comments: 26
Patch Set 2 : #
Total comments: 18
Patch Set 3 : Rebasing #Patch Set 4 : #Patch Set 5 : fixing compile errors #
Total comments: 14
Patch Set 6 : #
Total comments: 10
Patch Set 7 : rebasing #Patch Set 8 : #
Total comments: 11
Patch Set 9 : #
Total comments: 2
Patch Set 10 : Rebasing #Patch Set 11 : #Messages
Total messages: 17 (0 generated)
https://codereview.chromium.org/11817051/diff/1/chrome/browser/ui/autofill/au... File chrome/browser/ui/autofill/autofill_popup_controller_impl.cc (right): https://codereview.chromium.org/11817051/diff/1/chrome/browser/ui/autofill/au... chrome/browser/ui/autofill/autofill_popup_controller_impl.cc:125: // Don't let the popup have a smaller width then then element it is a popup nit: "then then" -> "than the" https://codereview.chromium.org/11817051/diff/1/chrome/browser/ui/autofill/au... chrome/browser/ui/autofill/autofill_popup_controller_impl.cc:127: // should follow. As I mention below, this is almost certainly wrong, for security reasons. https://codereview.chromium.org/11817051/diff/1/chrome/browser/ui/autofill/au... chrome/browser/ui/autofill/autofill_popup_controller_impl.cc:131: for (size_t i = 0; i < names_.size(); ++i) { nit: Please add a comment indicating that this loop is for eliding strings to fit in the available width. https://codereview.chromium.org/11817051/diff/1/chrome/browser/ui/autofill/au... chrome/browser/ui/autofill/autofill_popup_controller_impl.cc:132: int total_text_length = name_font().GetStringWidth(names[i])+ nit: Add a space before the plus sign. https://codereview.chromium.org/11817051/diff/1/chrome/browser/ui/autofill/au... chrome/browser/ui/autofill/autofill_popup_controller_impl.cc:139: int row_available_width = max_popup_width - RowWidthWithoutText(i); Optional nit: Drop the "row_" prefix IMO https://codereview.chromium.org/11817051/diff/1/chrome/browser/ui/autofill/au... chrome/browser/ui/autofill/autofill_popup_controller_impl.cc:141: // Each field recieves space in proportion to it length. nit: "it length" -> "its length" https://codereview.chromium.org/11817051/diff/1/chrome/browser/ui/autofill/au... chrome/browser/ui/autofill/autofill_popup_controller_impl.cc:531: // Add delete icon, if required. nit: "Add the delete icon size", or perhaps "Add space for the delete icon"? https://codereview.chromium.org/11817051/diff/1/chrome/browser/ui/autofill/au... chrome/browser/ui/autofill/autofill_popup_controller_impl.cc:541: nit: Extra newline; please remove. https://codereview.chromium.org/11817051/diff/1/chrome/browser/ui/autofill/au... chrome/browser/ui/autofill/autofill_popup_controller_impl.cc:543: gfx::Point right_corner_of_popup = element_bounds().origin() + When anchoring to the left edge of the field, we should never anchor beyond the left-most on-screen pixel of the text field. That is, we shouldn't assume that element_bounds.origin() is visible on the screen. This is important to get right for security reasons: a malicious website designer shouldn't be able to force us to show any part of the popup off screen. https://codereview.chromium.org/11817051/diff/1/chrome/browser/ui/autofill/au... chrome/browser/ui/autofill/autofill_popup_controller_impl.cc:547: gfx::Display display = screen->GetDisplayNearestPoint(right_corner_of_popup); In addition to anchoring to the left edge of the field, we should be able to anchor to the right edge. If the window happens to be positioned so that the field is near the right edge of the screen, the popup should grow to the left rather than to the right. Again, this is important for security reasons, so that an attacker can't force us to display a popup with all of the text elided by simply placing a field super near the edge of the screen. https://codereview.chromium.org/11817051/diff/1/chrome/browser/ui/autofill/au... chrome/browser/ui/autofill/autofill_popup_controller_impl.cc:549: int rightmost_display_x = display.GetSizeInPixel().width() + nit: Extra space after the equals sign. https://codereview.chromium.org/11817051/diff/1/chrome/browser/ui/autofill/au... File chrome/browser/ui/autofill/autofill_popup_controller_impl.h (right): https://codereview.chromium.org/11817051/diff/1/chrome/browser/ui/autofill/au... chrome/browser/ui/autofill/autofill_popup_controller_impl.h:132: // the size of the row that won't be reducable (since all the text can be nit: "reducable" -> "reducible" https://codereview.chromium.org/11817051/diff/1/chrome/browser/ui/autofill/au... chrome/browser/ui/autofill/autofill_popup_controller_impl.h:138: virtual int MaxVisiblePopupWidth(); nit: Why is this virtual? (Add a comment.)
https://codereview.chromium.org/11817051/diff/1/chrome/browser/ui/autofill/au... File chrome/browser/ui/autofill/autofill_popup_controller_impl.cc (right): https://codereview.chromium.org/11817051/diff/1/chrome/browser/ui/autofill/au... chrome/browser/ui/autofill/autofill_popup_controller_impl.cc:125: // Don't let the popup have a smaller width then then element it is a popup On 2013/01/10 23:34:36, Ilya Sherman wrote: > nit: "then then" -> "than the" Done. https://codereview.chromium.org/11817051/diff/1/chrome/browser/ui/autofill/au... chrome/browser/ui/autofill/autofill_popup_controller_impl.cc:127: // should follow. On 2013/01/10 23:34:36, Ilya Sherman wrote: > As I mention below, this is almost certainly wrong, for security reasons. Done. https://codereview.chromium.org/11817051/diff/1/chrome/browser/ui/autofill/au... chrome/browser/ui/autofill/autofill_popup_controller_impl.cc:131: for (size_t i = 0; i < names_.size(); ++i) { On 2013/01/10 23:34:36, Ilya Sherman wrote: > nit: Please add a comment indicating that this loop is for eliding strings to > fit in the available width. Done. https://codereview.chromium.org/11817051/diff/1/chrome/browser/ui/autofill/au... chrome/browser/ui/autofill/autofill_popup_controller_impl.cc:132: int total_text_length = name_font().GetStringWidth(names[i])+ On 2013/01/10 23:34:36, Ilya Sherman wrote: > nit: Add a space before the plus sign. Done. https://codereview.chromium.org/11817051/diff/1/chrome/browser/ui/autofill/au... chrome/browser/ui/autofill/autofill_popup_controller_impl.cc:139: int row_available_width = max_popup_width - RowWidthWithoutText(i); On 2013/01/10 23:34:36, Ilya Sherman wrote: > Optional nit: Drop the "row_" prefix IMO Done. https://codereview.chromium.org/11817051/diff/1/chrome/browser/ui/autofill/au... chrome/browser/ui/autofill/autofill_popup_controller_impl.cc:141: // Each field recieves space in proportion to it length. On 2013/01/10 23:34:36, Ilya Sherman wrote: > nit: "it length" -> "its length" Done. https://codereview.chromium.org/11817051/diff/1/chrome/browser/ui/autofill/au... chrome/browser/ui/autofill/autofill_popup_controller_impl.cc:531: // Add delete icon, if required. On 2013/01/10 23:34:36, Ilya Sherman wrote: > nit: "Add the delete icon size", or perhaps "Add space for the delete icon"? Done. https://codereview.chromium.org/11817051/diff/1/chrome/browser/ui/autofill/au... chrome/browser/ui/autofill/autofill_popup_controller_impl.cc:541: On 2013/01/10 23:34:36, Ilya Sherman wrote: > nit: Extra newline; please remove. Done. https://codereview.chromium.org/11817051/diff/1/chrome/browser/ui/autofill/au... chrome/browser/ui/autofill/autofill_popup_controller_impl.cc:543: gfx::Point right_corner_of_popup = element_bounds().origin() + On 2013/01/10 23:34:36, Ilya Sherman wrote: > When anchoring to the left edge of the field, we should never anchor beyond the > left-most on-screen pixel of the text field. That is, we shouldn't assume that > element_bounds.origin() is visible on the screen. This is important to get > right for security reasons: a malicious website designer shouldn't be able to > force us to show any part of the popup off screen. Ah, I hadn't been aware of that issue. Fixed. https://codereview.chromium.org/11817051/diff/1/chrome/browser/ui/autofill/au... chrome/browser/ui/autofill/autofill_popup_controller_impl.cc:547: gfx::Display display = screen->GetDisplayNearestPoint(right_corner_of_popup); On 2013/01/10 23:34:36, Ilya Sherman wrote: > In addition to anchoring to the left edge of the field, we should be able to > anchor to the right edge. If the window happens to be positioned so that the > field is near the right edge of the screen, the popup should grow to the left > rather than to the right. Again, this is important for security reasons, so > that an attacker can't force us to display a popup with all of the text elided > by simply placing a field super near the edge of the screen. Done. https://codereview.chromium.org/11817051/diff/1/chrome/browser/ui/autofill/au... chrome/browser/ui/autofill/autofill_popup_controller_impl.cc:549: int rightmost_display_x = display.GetSizeInPixel().width() + On 2013/01/10 23:34:36, Ilya Sherman wrote: > nit: Extra space after the equals sign. Done. https://codereview.chromium.org/11817051/diff/1/chrome/browser/ui/autofill/au... File chrome/browser/ui/autofill/autofill_popup_controller_impl.h (right): https://codereview.chromium.org/11817051/diff/1/chrome/browser/ui/autofill/au... chrome/browser/ui/autofill/autofill_popup_controller_impl.h:132: // the size of the row that won't be reducable (since all the text can be On 2013/01/10 23:34:36, Ilya Sherman wrote: > nit: "reducable" -> "reducible" Done. https://codereview.chromium.org/11817051/diff/1/chrome/browser/ui/autofill/au... chrome/browser/ui/autofill/autofill_popup_controller_impl.h:138: virtual int MaxVisiblePopupWidth(); On 2013/01/10 23:34:36, Ilya Sherman wrote: > nit: Why is this virtual? (Add a comment.) Done.
Thanks, looks much better from a security perspective :) I have some suggestions inline for how to further consolidate/clean up the popup sizing code -- let me know if you'd rather postpone some of this cleanup to a separate CL. https://codereview.chromium.org/11817051/diff/8001/chrome/browser/ui/autofill... File chrome/browser/ui/autofill/autofill_popup_controller_impl.cc (right): https://codereview.chromium.org/11817051/diff/8001/chrome/browser/ui/autofill... chrome/browser/ui/autofill/autofill_popup_controller_impl.cc:141: name_font().GetStringWidth(names_[i]) / total_text_length; nit: Might be worth caching the result of the GetStringWidth() calls within this loop (i.e. from lines 130-131), as it's probably one of the more expensive computations in this class. https://codereview.chromium.org/11817051/diff/8001/chrome/browser/ui/autofill... chrome/browser/ui/autofill/autofill_popup_controller_impl.cc:227: int AutofillPopupControllerImpl::GetPopupRequiredWidth() { Rather than exposing this method to the view, IMO it would be better to just calculate this as an implementation detail of the controller and update the popup bounds directly. Ditto for the height. https://codereview.chromium.org/11817051/diff/8001/chrome/browser/ui/autofill... chrome/browser/ui/autofill/autofill_popup_controller_impl.cc:547: element_bounds().origin()); Hmm, shouldn't this be the display that's at the left corner of the popup were it to grow from the right corner of the element bounds? https://codereview.chromium.org/11817051/diff/8001/chrome/browser/ui/autofill... chrome/browser/ui/autofill/autofill_popup_controller_impl.cc:559: leftmost_display_x); nit: indentation https://codereview.chromium.org/11817051/diff/8001/chrome/browser/ui/autofill... chrome/browser/ui/autofill/autofill_popup_controller_impl.cc:561: rightmost_display_x); nit: indentation https://codereview.chromium.org/11817051/diff/8001/chrome/browser/ui/autofill... chrome/browser/ui/autofill/autofill_popup_controller_impl.cc:567: std::max(right_available, left_available)); nit: #include <algorithm> for std::min and ::max. https://codereview.chromium.org/11817051/diff/8001/chrome/browser/ui/autofill... chrome/browser/ui/autofill/autofill_popup_controller_impl.cc:574: } nit: No need for curlies https://codereview.chromium.org/11817051/diff/8001/chrome/browser/ui/autofill... File chrome/browser/ui/autofill/autofill_popup_controller_impl.h (right): https://codereview.chromium.org/11817051/diff/8001/chrome/browser/ui/autofill... chrome/browser/ui/autofill/autofill_popup_controller_impl.h:138: virtual int CalculateAndSetPopupWidth(); IMO the code would be cleaner if this either returned the calculated width or fully updated the popup_bounds_ directly, rather than partially updating the popup_bounds_ and also returning the width. I'd probably recommend the latter option, and naming this method something like UpdatePopupWidth(). https://codereview.chromium.org/11817051/diff/8001/chrome/browser/ui/views/au... File chrome/browser/ui/views/autofill/autofill_popup_view_views.cc (right): https://codereview.chromium.org/11817051/diff/8001/chrome/browser/ui/views/au... chrome/browser/ui/views/autofill/autofill_popup_view_views.cc:244: } WDYT -- should all of this logic be moved into the controller as well?
Is it possible for the popup to get too tall to fit on the screen? I think the Autofill manager limits how many values it returns, but I'm not sure what happens if an element has a huge datalist. https://codereview.chromium.org/11817051/diff/8001/chrome/browser/ui/autofill... File chrome/browser/ui/autofill/autofill_popup_controller_impl.cc (right): https://codereview.chromium.org/11817051/diff/8001/chrome/browser/ui/autofill... chrome/browser/ui/autofill/autofill_popup_controller_impl.cc:141: name_font().GetStringWidth(names_[i]) / total_text_length; On 2013/01/11 23:05:35, Ilya Sherman wrote: > nit: Might be worth caching the result of the GetStringWidth() calls within this > loop (i.e. from lines 130-131), as it's probably one of the more expensive > computations in this class. Done. https://codereview.chromium.org/11817051/diff/8001/chrome/browser/ui/autofill... chrome/browser/ui/autofill/autofill_popup_controller_impl.cc:227: int AutofillPopupControllerImpl::GetPopupRequiredWidth() { On 2013/01/11 23:05:35, Ilya Sherman wrote: > Rather than exposing this method to the view, IMO it would be better to just > calculate this as an implementation detail of the controller and update the > popup bounds directly. Ditto for the height. Done. https://codereview.chromium.org/11817051/diff/8001/chrome/browser/ui/autofill... chrome/browser/ui/autofill/autofill_popup_controller_impl.cc:547: element_bounds().origin()); On 2013/01/11 23:05:35, Ilya Sherman wrote: > Hmm, shouldn't this be the display that's at the left corner of the popup were > it to grow from the right corner of the element bounds? Yup, fixed. https://codereview.chromium.org/11817051/diff/8001/chrome/browser/ui/autofill... chrome/browser/ui/autofill/autofill_popup_controller_impl.cc:559: leftmost_display_x); On 2013/01/11 23:05:35, Ilya Sherman wrote: > nit: indentation Done. https://codereview.chromium.org/11817051/diff/8001/chrome/browser/ui/autofill... chrome/browser/ui/autofill/autofill_popup_controller_impl.cc:561: rightmost_display_x); On 2013/01/11 23:05:35, Ilya Sherman wrote: > nit: indentation Done. https://codereview.chromium.org/11817051/diff/8001/chrome/browser/ui/autofill... chrome/browser/ui/autofill/autofill_popup_controller_impl.cc:567: std::max(right_available, left_available)); On 2013/01/11 23:05:35, Ilya Sherman wrote: > nit: #include <algorithm> for std::min and ::max. Done. https://codereview.chromium.org/11817051/diff/8001/chrome/browser/ui/autofill... chrome/browser/ui/autofill/autofill_popup_controller_impl.cc:574: } On 2013/01/11 23:05:35, Ilya Sherman wrote: > nit: No need for curlies Done. https://codereview.chromium.org/11817051/diff/8001/chrome/browser/ui/autofill... File chrome/browser/ui/autofill/autofill_popup_controller_impl.h (right): https://codereview.chromium.org/11817051/diff/8001/chrome/browser/ui/autofill... chrome/browser/ui/autofill/autofill_popup_controller_impl.h:138: virtual int CalculateAndSetPopupWidth(); On 2013/01/11 23:05:35, Ilya Sherman wrote: > IMO the code would be cleaner if this either returned the calculated width or > fully updated the popup_bounds_ directly, rather than partially updating the > popup_bounds_ and also returning the width. I'd probably recommend the latter > option, and naming this method something like UpdatePopupWidth(). Done. https://codereview.chromium.org/11817051/diff/8001/chrome/browser/ui/views/au... File chrome/browser/ui/views/autofill/autofill_popup_view_views.cc (right): https://codereview.chromium.org/11817051/diff/8001/chrome/browser/ui/views/au... chrome/browser/ui/views/autofill/autofill_popup_view_views.cc:244: } On 2013/01/11 23:05:35, Ilya Sherman wrote: > WDYT -- should all of this logic be moved into the controller as well? Done. This was just created because I didn't know a platform independent way to get the screen bounds before.
Thanks, looks pretty good! Can you add tests verifying that the popup gets positioned correctly? I would guess that you ought to be able to set up a browser_test that positions the Chrome window quite near to the screen's edge and so forces the popup to grow in one direction or the other... though maybe there's an easier way to mock out the display size info. On 2013/01/14 20:32:33, csharp wrote: > Is it possible for the popup to get too tall to fit on the screen? I think the > Autofill manager limits how many values it returns, but I'm not sure what > happens if an element has a huge datalist. Yes, this is possible. We should support scrolling for overflow, but that's probably best to leave for a later CL. https://chromiumcodereview.appspot.com/11817051/diff/15015/chrome/browser/ui/... File chrome/browser/ui/autofill/autofill_popup_controller_impl.cc (right): https://chromiumcodereview.appspot.com/11817051/diff/15015/chrome/browser/ui/... chrome/browser/ui/autofill/autofill_popup_controller_impl.cc:496: int AutofillPopupControllerImpl::GetPopupRequiredWidth() { nit: Perhaps name this "GetDesiredPopupWidth()", since the popup can actually be smaller than this in some cases? (Applies throughout) https://chromiumcodereview.appspot.com/11817051/diff/15015/chrome/browser/ui/... chrome/browser/ui/autofill/autofill_popup_controller_impl.cc:497: if (name_font_.platform_font() == NULL || nit: if (!name_font_.platform_font() || !name_subtext_font_.platform_font()) { ... } https://chromiumcodereview.appspot.com/11817051/diff/15015/chrome/browser/ui/... chrome/browser/ui/autofill/autofill_popup_controller_impl.cc:549: // popup go could). nit: It's a little weird to describe top_left_corner_of_popup in a comment but not to also describe bottom_right_corner_of_popup. https://chromiumcodereview.appspot.com/11817051/diff/15015/chrome/browser/ui/... chrome/browser/ui/autofill/autofill_popup_controller_impl.cc:560: element_bounds().origin()); Hmm, why isn't this top_left_corner_of_popup? https://chromiumcodereview.appspot.com/11817051/diff/15015/chrome/browser/ui/... chrome/browser/ui/autofill/autofill_popup_controller_impl.cc:585: // Calculate the leftmost and rightmost values the popup can have without nit: s/values/coordinates https://chromiumcodereview.appspot.com/11817051/diff/15015/chrome/browser/ui/... chrome/browser/ui/autofill/autofill_popup_controller_impl.cc:590: rightmost_display_x); Optional nit: Mebbe drop the "popup_" prefix? https://chromiumcodereview.appspot.com/11817051/diff/15015/chrome/browser/ui/... File chrome/browser/ui/views/autofill/autofill_popup_view_views.cc (right): https://chromiumcodereview.appspot.com/11817051/diff/15015/chrome/browser/ui/... chrome/browser/ui/views/autofill/autofill_popup_view_views.cc:127: widget->SetBounds(controller_->popup_bounds()); nit: Should the bounds be set before the widget is shown?
I looked a bit and it seemed a lot easy to just mock out the display screen than to try and write a browser test to handle the growth cases. The unit tests have been added now and look pretty good. https://codereview.chromium.org/11817051/diff/15015/chrome/browser/ui/autofil... File chrome/browser/ui/autofill/autofill_popup_controller_impl.cc (right): https://codereview.chromium.org/11817051/diff/15015/chrome/browser/ui/autofil... chrome/browser/ui/autofill/autofill_popup_controller_impl.cc:496: int AutofillPopupControllerImpl::GetPopupRequiredWidth() { On 2013/01/15 01:34:45, Ilya Sherman wrote: > nit: Perhaps name this "GetDesiredPopupWidth()", since the popup can actually be > smaller than this in some cases? (Applies throughout) Done. https://codereview.chromium.org/11817051/diff/15015/chrome/browser/ui/autofil... chrome/browser/ui/autofill/autofill_popup_controller_impl.cc:497: if (name_font_.platform_font() == NULL || On 2013/01/15 01:34:45, Ilya Sherman wrote: > nit: if (!name_font_.platform_font() || !name_subtext_font_.platform_font()) { > ... } Done. https://codereview.chromium.org/11817051/diff/15015/chrome/browser/ui/autofil... chrome/browser/ui/autofill/autofill_popup_controller_impl.cc:549: // popup go could). On 2013/01/15 01:34:45, Ilya Sherman wrote: > nit: It's a little weird to describe top_left_corner_of_popup in a comment but > not to also describe bottom_right_corner_of_popup. Done. https://codereview.chromium.org/11817051/diff/15015/chrome/browser/ui/autofil... chrome/browser/ui/autofill/autofill_popup_controller_impl.cc:560: element_bounds().origin()); On 2013/01/15 01:34:45, Ilya Sherman wrote: > Hmm, why isn't this top_left_corner_of_popup? I blame a brain fart :) Fixed https://codereview.chromium.org/11817051/diff/15015/chrome/browser/ui/autofil... chrome/browser/ui/autofill/autofill_popup_controller_impl.cc:585: // Calculate the leftmost and rightmost values the popup can have without On 2013/01/15 01:34:45, Ilya Sherman wrote: > nit: s/values/coordinates Done. https://codereview.chromium.org/11817051/diff/15015/chrome/browser/ui/autofil... chrome/browser/ui/autofill/autofill_popup_controller_impl.cc:590: rightmost_display_x); On 2013/01/15 01:34:45, Ilya Sherman wrote: > Optional nit: Mebbe drop the "popup_" prefix? Done. https://codereview.chromium.org/11817051/diff/15015/chrome/browser/ui/views/a... File chrome/browser/ui/views/autofill/autofill_popup_view_views.cc (right): https://codereview.chromium.org/11817051/diff/15015/chrome/browser/ui/views/a... chrome/browser/ui/views/autofill/autofill_popup_view_views.cc:127: widget->SetBounds(controller_->popup_bounds()); On 2013/01/15 01:34:45, Ilya Sherman wrote: > nit: Should the bounds be set before the widget is shown? Done.
Thanks. Just a few final comments, then should be good to go :) https://codereview.chromium.org/11817051/diff/25001/chrome/browser/ui/autofil... File chrome/browser/ui/autofill/autofill_popup_controller_impl.cc (right): https://codereview.chromium.org/11817051/diff/25001/chrome/browser/ui/autofil... chrome/browser/ui/autofill/autofill_popup_controller_impl.cc:180: popup_bounds_.set_height(GetDesiredPopupHeight()); This seems wrong. Wouldn't it cause the popup to grow off the screen if it had been shrunk to fit? https://codereview.chromium.org/11817051/diff/25001/chrome/browser/ui/autofil... File chrome/browser/ui/autofill/autofill_popup_controller_unittest.cc (right): https://codereview.chromium.org/11817051/diff/25001/chrome/browser/ui/autofil... chrome/browser/ui/autofill/autofill_popup_controller_unittest.cc:44: testing::ReturnRef(element_bounds_)); I think I recall Scott mentioning that gMock shouldn't be used in UI code. Rather than mocking out these methods, you can provide protected setters that test code can call into. https://codereview.chromium.org/11817051/diff/25001/chrome/browser/ui/autofil... chrome/browser/ui/autofill/autofill_popup_controller_unittest.cc:95: nit: Extra newline. https://codereview.chromium.org/11817051/diff/25001/chrome/browser/ui/autofil... chrome/browser/ui/autofill/autofill_popup_controller_unittest.cc:320: ).WillRepeatedly(testing::Return(display)); nit: Closing paren should be on the previous line; the rest should be indent four spaces from the first "E" of "EXPECT_CALL" https://codereview.chromium.org/11817051/diff/25001/chrome/browser/ui/autofil... chrome/browser/ui/autofill/autofill_popup_controller_unittest.cc:357: gfx::Rect(2 * desired_width, 0, 0, 0)); Hmm, this puts the element completely off screen, right? IMO it would be good to also test with it partially off screen.
https://codereview.chromium.org/11817051/diff/25001/chrome/browser/ui/autofil... File chrome/browser/ui/autofill/autofill_popup_controller_impl.cc (right): https://codereview.chromium.org/11817051/diff/25001/chrome/browser/ui/autofil... chrome/browser/ui/autofill/autofill_popup_controller_impl.cc:180: popup_bounds_.set_height(GetDesiredPopupHeight()); On 2013/01/15 22:29:55, Ilya Sherman wrote: > This seems wrong. Wouldn't it cause the popup to grow off the screen if it had > been shrunk to fit? yup, fixed. https://codereview.chromium.org/11817051/diff/25001/chrome/browser/ui/autofil... File chrome/browser/ui/autofill/autofill_popup_controller_unittest.cc (right): https://codereview.chromium.org/11817051/diff/25001/chrome/browser/ui/autofil... chrome/browser/ui/autofill/autofill_popup_controller_unittest.cc:44: testing::ReturnRef(element_bounds_)); On 2013/01/15 22:29:55, Ilya Sherman wrote: > I think I recall Scott mentioning that gMock shouldn't be used in UI code. > Rather than mocking out these methods, you can provide protected setters that > test code can call into. Removed the two new mocks I had added. https://codereview.chromium.org/11817051/diff/25001/chrome/browser/ui/autofil... chrome/browser/ui/autofill/autofill_popup_controller_unittest.cc:95: On 2013/01/15 22:29:55, Ilya Sherman wrote: > nit: Extra newline. Done. https://codereview.chromium.org/11817051/diff/25001/chrome/browser/ui/autofil... chrome/browser/ui/autofill/autofill_popup_controller_unittest.cc:320: ).WillRepeatedly(testing::Return(display)); On 2013/01/15 22:29:55, Ilya Sherman wrote: > nit: Closing paren should be on the previous line; the rest should be indent > four spaces from the first "E" of "EXPECT_CALL" Fixed. https://codereview.chromium.org/11817051/diff/25001/chrome/browser/ui/autofil... chrome/browser/ui/autofill/autofill_popup_controller_unittest.cc:357: gfx::Rect(2 * desired_width, 0, 0, 0)); On 2013/01/15 22:29:55, Ilya Sherman wrote: > Hmm, this puts the element completely off screen, right? IMO it would be good > to also test with it partially off screen. Added two tests for the popup only going partially off.
https://codereview.chromium.org/11817051/diff/46002/chrome/browser/ui/autofil... File chrome/browser/ui/autofill/autofill_popup_controller_impl.cc (right): https://codereview.chromium.org/11817051/diff/46002/chrome/browser/ui/autofil... chrome/browser/ui/autofill/autofill_popup_controller_impl.cc:208: UpdatePopupBounds(); Mightn't this cause the popup to jump w.r.t. where it's anchored to? That seems kind of awkward, though maybe it's fine. Maybe add a TODO to keep an eye on this? https://codereview.chromium.org/11817051/diff/46002/chrome/browser/ui/autofil... chrome/browser/ui/autofill/autofill_popup_controller_impl.cc:593: // Calculate the right and left cutoff positions, which are the the left and nit: "the the" -> "the" https://codereview.chromium.org/11817051/diff/46002/chrome/browser/ui/autofil... chrome/browser/ui/autofill/autofill_popup_controller_impl.cc:597: element_bounds().x())); I don't understand this std::min -- what is it for? If there's a screen to the left of where the element lives, it should be fine for the popup to grow there. https://codereview.chromium.org/11817051/diff/46002/chrome/browser/ui/autofil... chrome/browser/ui/autofill/autofill_popup_controller_impl.cc:627: // right parts of the element bounds, capped to screen space. nit: s/right and left/top and bottom/ https://codereview.chromium.org/11817051/diff/46002/chrome/browser/ui/autofil... File chrome/browser/ui/autofill/autofill_popup_controller_unittest.cc (right): https://codereview.chromium.org/11817051/diff/46002/chrome/browser/ui/autofil... chrome/browser/ui/autofill/autofill_popup_controller_unittest.cc:377: // The popup would be partial off the bottom and the right side of the screen. nit: "would be partial off" -> "would be partially off"
https://codereview.chromium.org/11817051/diff/46002/chrome/browser/ui/autofil... File chrome/browser/ui/autofill/autofill_popup_controller_impl.cc (right): https://codereview.chromium.org/11817051/diff/46002/chrome/browser/ui/autofil... chrome/browser/ui/autofill/autofill_popup_controller_impl.cc:208: UpdatePopupBounds(); On 2013/01/17 01:36:19, Ilya Sherman wrote: > Mightn't this cause the popup to jump w.r.t. where it's anchored to? That seems > kind of awkward, though maybe it's fine. Maybe add a TODO to keep an eye on > this? Ya, I'm not sure what the best option here is since it would only move it if it wasn't in the preferred position (e.g. it was forced to be above the popup). Added a TODO. https://codereview.chromium.org/11817051/diff/46002/chrome/browser/ui/autofil... chrome/browser/ui/autofill/autofill_popup_controller_impl.cc:593: // Calculate the right and left cutoff positions, which are the the left and On 2013/01/17 01:36:19, Ilya Sherman wrote: > nit: "the the" -> "the" Done. https://codereview.chromium.org/11817051/diff/46002/chrome/browser/ui/autofil... chrome/browser/ui/autofill/autofill_popup_controller_impl.cc:597: element_bounds().x())); On 2013/01/17 01:36:19, Ilya Sherman wrote: > I don't understand this std::min -- what is it for? If there's a screen to the > left of where the element lives, it should be fine for the popup to grow there. Ok, I've tried cleaning up the comment and variable names a bit, let me know if it is still confusing. Basically these values represent the "starting point" underneath the element for the popup, so we want them to be as close to the left or right side (depending on which direction we grow) as possible. I have the min and max to ensure that these values stay on the screen (otherwise if the element is completely off the screen for some reason, the popup would also end up off the page). https://codereview.chromium.org/11817051/diff/46002/chrome/browser/ui/autofil... chrome/browser/ui/autofill/autofill_popup_controller_impl.cc:627: // right parts of the element bounds, capped to screen space. On 2013/01/17 01:36:19, Ilya Sherman wrote: > nit: s/right and left/top and bottom/ Done. https://codereview.chromium.org/11817051/diff/46002/chrome/browser/ui/autofil... File chrome/browser/ui/autofill/autofill_popup_controller_unittest.cc (right): https://codereview.chromium.org/11817051/diff/46002/chrome/browser/ui/autofil... chrome/browser/ui/autofill/autofill_popup_controller_unittest.cc:377: // The popup would be partial off the bottom and the right side of the screen. On 2013/01/17 01:36:19, Ilya Sherman wrote: > nit: "would be partial off" -> "would be partially off" Done.
LGTM, thanks! https://codereview.chromium.org/11817051/diff/46002/chrome/browser/ui/autofil... File chrome/browser/ui/autofill/autofill_popup_controller_impl.cc (right): https://codereview.chromium.org/11817051/diff/46002/chrome/browser/ui/autofil... chrome/browser/ui/autofill/autofill_popup_controller_impl.cc:597: element_bounds().x())); On 2013/01/17 16:31:20, csharp wrote: > On 2013/01/17 01:36:19, Ilya Sherman wrote: > > I don't understand this std::min -- what is it for? If there's a screen to > the > > left of where the element lives, it should be fine for the popup to grow > there. > > Ok, I've tried cleaning up the comment and variable names a bit, let me know if > it is still confusing. > > Basically these values represent the "starting point" underneath the element for > the popup, so we want them to be as close to the left or right side (depending > on which direction we grow) as possible. I have the min and max to ensure that > these values stay on the screen (otherwise if the element is completely off the > screen for some reason, the popup would also end up off the page). Ah, I think I understand what you mean. Subtle. https://codereview.chromium.org/11817051/diff/44005/chrome/browser/ui/autofil... File chrome/browser/ui/autofill/autofill_popup_controller_impl.cc (right): https://codereview.chromium.org/11817051/diff/44005/chrome/browser/ui/autofil... chrome/browser/ui/autofill/autofill_popup_controller_impl.cc:208: // TODO(csharp): Since UpdatePopupBounds can the position of the popup might nit: "can the position" -> rephrase
sky@ for views owner approval estade@ for gtk owner approval https://codereview.chromium.org/11817051/diff/44005/chrome/browser/ui/autofil... File chrome/browser/ui/autofill/autofill_popup_controller_impl.cc (right): https://codereview.chromium.org/11817051/diff/44005/chrome/browser/ui/autofil... chrome/browser/ui/autofill/autofill_popup_controller_impl.cc:208: // TODO(csharp): Since UpdatePopupBounds can the position of the popup might On 2013/01/18 01:52:48, Ilya Sherman wrote: > nit: "can the position" -> rephrase Done.
views LGTM
deleting code always LGTM
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/csharp@chromium.org/11817051/66001
Message was sent while issue was closed.
Change committed as 178149 |
