Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
9187009:
Basic Drawn text for new GTK Autofill popup. (Closed)
|
Created:
8 years, 11 months ago by csharp Modified:
8 years, 11 months ago CC:
chromium-reviews, GeorgeY, dhollowa+watch_chromium.org, Ilya Sherman, dyu1 Visibility:
Public. |
DescriptionBasic Drawn text for new GTK Autofill popup.
Begins draws the text (value and label) in the popup when displayed. This
doesn't handle most of the complex issue (such as RTL) since this is more
intended to give visiable feedback while working on ohter new autofill features.
BUG=51644
TEST=
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=118497
Patch Set 1 #
Total comments: 2
Patch Set 2 : Use Cairo #
Total comments: 44
Patch Set 3 : Responding to comments #
Total comments: 7
Patch Set 4 : Responding to Comments #
Total comments: 20
Patch Set 5 : Responding to comments #Patch Set 6 : Fixing mac compile error #Patch Set 7 : Replacing header with typedefs #
Messages
Total messages: 19 (0 generated)
Most of this code is based off of omnibox_popup_view_gtk.cc so I might have misunderstood how certain things work.
http://codereview.chromium.org/9187009/diff/1/chrome/browser/ui/gtk/autofill/... File chrome/browser/ui/gtk/autofill/autofill_popup_view_gtk.cc (right): http://codereview.chromium.org/9187009/diff/1/chrome/browser/ui/gtk/autofill/... chrome/browser/ui/gtk/autofill/autofill_popup_view_gtk.cc:116: GdkGC* gc = gdk_gc_new(drawable); Please don't use GdkGC or other deprecated stuff. Please look at omnibox_popup_view_gtk.cc for how to do this in cairo. omnibox_popup_view_gtk.cc was moved off GdkGC in the beginning of December and most of my time these days is stamping out deprecated raw struct access (widget->allocation ==> gtk_widget_get_allocation(widget)) and removing the old drawing model because GdkGC/GdkDrawable/etc won't be included in GTK3, which we have to start moving to in ~April.
http://codereview.chromium.org/9187009/diff/1/chrome/browser/ui/gtk/autofill/... File chrome/browser/ui/gtk/autofill/autofill_popup_view_gtk.cc (right): http://codereview.chromium.org/9187009/diff/1/chrome/browser/ui/gtk/autofill/... chrome/browser/ui/gtk/autofill/autofill_popup_view_gtk.cc:116: GdkGC* gc = gdk_gc_new(drawable); On 2012/01/11 19:01:47, Elliot Glaysher wrote: > Please don't use GdkGC or other deprecated stuff. Please look at > omnibox_popup_view_gtk.cc for how to do this in cairo. > > omnibox_popup_view_gtk.cc was moved off GdkGC in the beginning of December and > most of my time these days is stamping out deprecated raw struct access > (widget->allocation ==> gtk_widget_get_allocation(widget)) and removing the old > drawing model because GdkGC/GdkDrawable/etc won't be included in GTK3, which we > have to start moving to in ~April. Sorry about that, I grab the code in early December and just hadn't gotten it ready to submit until now, I haven't thought of checking if it had changed. Using cairo now.
Hmm, no new tests? http://codereview.chromium.org/9187009/diff/4001/chrome/browser/autofill/auto... File chrome/browser/autofill/autofill_popup_view.h (right): http://codereview.chromium.org/9187009/diff/4001/chrome/browser/autofill/auto... chrome/browser/autofill/autofill_popup_view.h:30: // Platform independent work. nit: "Platform-independent" http://codereview.chromium.org/9187009/diff/4001/chrome/browser/autofill/auto... chrome/browser/autofill/autofill_popup_view.h:46: // Platform dependent work. nit: "Platform-dependent" http://codereview.chromium.org/9187009/diff/4001/chrome/browser/autofill/auto... chrome/browser/autofill/autofill_popup_view.h:49: const std::vector<string16>& autofill_icons) = 0; nit: Hmm, since all of these vectors are exposed via methods below, do we really need to pass them in as params? Conversely, if we do need to pass them in as params, do we really need them to be exposed via the methods below? http://codereview.chromium.org/9187009/diff/4001/chrome/browser/autofill/auto... chrome/browser/autofill/autofill_popup_view.h:54: const int separator_index() { return separator_index_; } nit: These methods should be marked const. (Ah, const, you are such a virus :) http://codereview.chromium.org/9187009/diff/4001/chrome/browser/autofill/auto... chrome/browser/autofill/autofill_popup_view.h:68: // The current autofill query values. nit: "autofill" -> "Autofill" http://codereview.chromium.org/9187009/diff/4001/chrome/browser/autofill/auto... chrome/browser/autofill/autofill_popup_view.h:74: // The location of the separator index (which separator the returned values nit: "which separator" -> "which separates" http://codereview.chromium.org/9187009/diff/4001/chrome/browser/autofill/auto... chrome/browser/autofill/autofill_popup_view.h:75: // from autofill options). nit: "from autofill options" -> "from the Autofill options" http://codereview.chromium.org/9187009/diff/4001/chrome/browser/ui/gtk/autofi... File chrome/browser/ui/gtk/autofill/autofill_popup_view_gtk.cc (right): http://codereview.chromium.org/9187009/diff/4001/chrome/browser/ui/gtk/autofi... chrome/browser/ui/gtk/autofill/autofill_popup_view_gtk.cc:19: // The amount of minimum padding between the autofill value and label. nit: You should mention that this is measured in pixels. http://codereview.chromium.org/9187009/diff/4001/chrome/browser/ui/gtk/autofi... chrome/browser/ui/gtk/autofill/autofill_popup_view_gtk.cc:25: gfx::Rect GetWindowRect(GdkWindow* window) { nit: Please include a comment for every method, even if it's something very brief for a simple/obvious method. http://codereview.chromium.org/9187009/diff/4001/chrome/browser/ui/gtk/autofi... chrome/browser/ui/gtk/autofill/autofill_popup_view_gtk.cc:31: gfx::Rect GetRectForLine(size_t line, int width, int height) { nit: In the context of "Rect", "Line" is ambiguous. Perhaps something like "GetRectForItem" or "GetRectForItemByIndex"? http://codereview.chromium.org/9187009/diff/4001/chrome/browser/ui/gtk/autofi... chrome/browser/ui/gtk/autofill/autofill_popup_view_gtk.cc:36: const string16& text) { nit: This looks like it could fit on the previous line. http://codereview.chromium.org/9187009/diff/4001/chrome/browser/ui/gtk/autofi... chrome/browser/ui/gtk/autofill/autofill_popup_view_gtk.cc:68: row_height_ = font_.GetHeight(); Hmm, where is the font_ initialized? http://codereview.chromium.org/9187009/diff/4001/chrome/browser/ui/gtk/autofi... chrome/browser/ui/gtk/autofill/autofill_popup_view_gtk.cc:95: int popup_required_width = element_bounds().width(); nit: How about just "popup_width"? http://codereview.chromium.org/9187009/diff/4001/chrome/browser/ui/gtk/autofi... chrome/browser/ui/gtk/autofill/autofill_popup_view_gtk.cc:96: CHECK_EQ(autofill_values.size(), autofill_labels.size()); nit: Can this be a DCHECK, so that we don't compile it into official builds? http://codereview.chromium.org/9187009/diff/4001/chrome/browser/ui/gtk/autofi... chrome/browser/ui/gtk/autofill/autofill_popup_view_gtk.cc:128: COMPILE_ASSERT(kBorderThickness == 1, border_1px_implied); Huh, I've never seen this macro used before. Nifty! http://codereview.chromium.org/9187009/diff/4001/chrome/browser/ui/gtk/autofi... chrome/browser/ui/gtk/autofill/autofill_popup_view_gtk.cc:160: line_rect.y() + ((row_height_ - actual_content_height) / 2)); I don't understand what this computation does -- wouldn't (row_height_ - actual_content_height) pretty much always be negative? http://codereview.chromium.org/9187009/diff/4001/chrome/browser/ui/gtk/autofi... chrome/browser/ui/gtk/autofill/autofill_popup_view_gtk.cc:172: - font_.GetStringWidth(autofill_labels()[i]); nit: The minus sign should be on the preceding line (same as boolean operators in if-stmts that wrap). http://codereview.chromium.org/9187009/diff/4001/chrome/browser/ui/gtk/autofi... chrome/browser/ui/gtk/autofill/autofill_popup_view_gtk.cc:187: const GdkColor* text_color) { nit: Can this be passed by const-reference instead? (We generally only pass a pointer if it is non-const, or if it can be NULL) http://codereview.chromium.org/9187009/diff/4001/chrome/browser/ui/gtk/autofi... File chrome/browser/ui/gtk/autofill/autofill_popup_view_gtk.h (right): http://codereview.chromium.org/9187009/diff/4001/chrome/browser/ui/gtk/autofi... chrome/browser/ui/gtk/autofill/autofill_popup_view_gtk.h:41: // The height of each individual autofill popup row. nit: "autofill" -> "Autofill" http://codereview.chromium.org/9187009/diff/4001/chrome/chrome_browser.gypi File chrome/chrome_browser.gypi (right): http://codereview.chromium.org/9187009/diff/4001/chrome/chrome_browser.gypi#n... chrome/chrome_browser.gypi:3371: 'browser/ui/views/autofill/autofill_popup_contents_view.h', Hmm, I don't see these files among the ones uploaded for the CL.
Yup, there are no new tests. I'm not sure how to add tests here, in omnibox_popup_view_gtk_unittest.cc it only really tests the SetupLayoutForMatch function, and this class doesn't really have anything similar left in it to test (as far as I can tell). http://codereview.chromium.org/9187009/diff/4001/chrome/browser/autofill/auto... File chrome/browser/autofill/autofill_popup_view.h (right): http://codereview.chromium.org/9187009/diff/4001/chrome/browser/autofill/auto... chrome/browser/autofill/autofill_popup_view.h:30: // Platform independent work. On 2012/01/11 23:36:29, Ilya Sherman wrote: > nit: "Platform-independent" Done. http://codereview.chromium.org/9187009/diff/4001/chrome/browser/autofill/auto... chrome/browser/autofill/autofill_popup_view.h:46: // Platform dependent work. On 2012/01/11 23:36:29, Ilya Sherman wrote: > nit: "Platform-dependent" Done. http://codereview.chromium.org/9187009/diff/4001/chrome/browser/autofill/auto... chrome/browser/autofill/autofill_popup_view.h:49: const std::vector<string16>& autofill_icons) = 0; On 2012/01/11 23:36:29, Ilya Sherman wrote: > nit: Hmm, since all of these vectors are exposed via methods below, do we really > need to pass them in as params? Conversely, if we do need to pass them in as > params, do we really need them to be exposed via the methods below? I removed passing them in, since they do need to be stored for them to still be valid when actually drawing the text. http://codereview.chromium.org/9187009/diff/4001/chrome/browser/autofill/auto... chrome/browser/autofill/autofill_popup_view.h:54: const int separator_index() { return separator_index_; } On 2012/01/11 23:36:29, Ilya Sherman wrote: > nit: These methods should be marked const. (Ah, const, you are such a virus :) Done. http://codereview.chromium.org/9187009/diff/4001/chrome/browser/autofill/auto... chrome/browser/autofill/autofill_popup_view.h:68: // The current autofill query values. On 2012/01/11 23:36:29, Ilya Sherman wrote: > nit: "autofill" -> "Autofill" Done. http://codereview.chromium.org/9187009/diff/4001/chrome/browser/autofill/auto... chrome/browser/autofill/autofill_popup_view.h:74: // The location of the separator index (which separator the returned values On 2012/01/11 23:36:29, Ilya Sherman wrote: > nit: "which separator" -> "which separates" Done. http://codereview.chromium.org/9187009/diff/4001/chrome/browser/autofill/auto... chrome/browser/autofill/autofill_popup_view.h:75: // from autofill options). On 2012/01/11 23:36:29, Ilya Sherman wrote: > nit: "from autofill options" -> "from the Autofill options" Done. http://codereview.chromium.org/9187009/diff/4001/chrome/browser/ui/gtk/autofi... File chrome/browser/ui/gtk/autofill/autofill_popup_view_gtk.cc (right): http://codereview.chromium.org/9187009/diff/4001/chrome/browser/ui/gtk/autofi... chrome/browser/ui/gtk/autofill/autofill_popup_view_gtk.cc:19: // The amount of minimum padding between the autofill value and label. On 2012/01/11 23:36:29, Ilya Sherman wrote: > nit: You should mention that this is measured in pixels. Done. http://codereview.chromium.org/9187009/diff/4001/chrome/browser/ui/gtk/autofi... chrome/browser/ui/gtk/autofill/autofill_popup_view_gtk.cc:25: gfx::Rect GetWindowRect(GdkWindow* window) { On 2012/01/11 23:36:29, Ilya Sherman wrote: > nit: Please include a comment for every method, even if it's something very > brief for a simple/obvious method. Done. http://codereview.chromium.org/9187009/diff/4001/chrome/browser/ui/gtk/autofi... chrome/browser/ui/gtk/autofill/autofill_popup_view_gtk.cc:31: gfx::Rect GetRectForLine(size_t line, int width, int height) { On 2012/01/11 23:36:29, Ilya Sherman wrote: > nit: In the context of "Rect", "Line" is ambiguous. Perhaps something like > "GetRectForItem" or "GetRectForItemByIndex"? Done. http://codereview.chromium.org/9187009/diff/4001/chrome/browser/ui/gtk/autofi... chrome/browser/ui/gtk/autofill/autofill_popup_view_gtk.cc:36: const string16& text) { On 2012/01/11 23:36:29, Ilya Sherman wrote: > nit: This looks like it could fit on the previous line. Done. http://codereview.chromium.org/9187009/diff/4001/chrome/browser/ui/gtk/autofi... chrome/browser/ui/gtk/autofill/autofill_popup_view_gtk.cc:68: row_height_ = font_.GetHeight(); On 2012/01/11 23:36:29, Ilya Sherman wrote: > Hmm, where is the font_ initialized? I just use the default constructor. Should I still have it listed in the initialization list as font_()? http://codereview.chromium.org/9187009/diff/4001/chrome/browser/ui/gtk/autofi... chrome/browser/ui/gtk/autofill/autofill_popup_view_gtk.cc:95: int popup_required_width = element_bounds().width(); On 2012/01/11 23:36:29, Ilya Sherman wrote: > nit: How about just "popup_width"? Done. http://codereview.chromium.org/9187009/diff/4001/chrome/browser/ui/gtk/autofi... chrome/browser/ui/gtk/autofill/autofill_popup_view_gtk.cc:96: CHECK_EQ(autofill_values.size(), autofill_labels.size()); On 2012/01/11 23:36:29, Ilya Sherman wrote: > nit: Can this be a DCHECK, so that we don't compile it into official builds? Done. http://codereview.chromium.org/9187009/diff/4001/chrome/browser/ui/gtk/autofi... chrome/browser/ui/gtk/autofill/autofill_popup_view_gtk.cc:160: line_rect.y() + ((row_height_ - actual_content_height) / 2)); On 2012/01/11 23:36:29, Ilya Sherman wrote: > I don't understand what this computation does -- wouldn't (row_height_ - > actual_content_height) pretty much always be negative? It will be positive because row_height_ >= actual_content_height. row_height_ is the size of the row and the actual_content_height is how much space the text will take. http://codereview.chromium.org/9187009/diff/4001/chrome/browser/ui/gtk/autofi... chrome/browser/ui/gtk/autofill/autofill_popup_view_gtk.cc:172: - font_.GetStringWidth(autofill_labels()[i]); On 2012/01/11 23:36:29, Ilya Sherman wrote: > nit: The minus sign should be on the preceding line (same as boolean operators > in if-stmts that wrap). Done. http://codereview.chromium.org/9187009/diff/4001/chrome/browser/ui/gtk/autofi... chrome/browser/ui/gtk/autofill/autofill_popup_view_gtk.cc:187: const GdkColor* text_color) { On 2012/01/11 23:36:29, Ilya Sherman wrote: > nit: Can this be passed by const-reference instead? (We generally only pass a > pointer if it is non-const, or if it can be NULL) Done. http://codereview.chromium.org/9187009/diff/4001/chrome/browser/ui/gtk/autofi... File chrome/browser/ui/gtk/autofill/autofill_popup_view_gtk.h (right): http://codereview.chromium.org/9187009/diff/4001/chrome/browser/ui/gtk/autofi... chrome/browser/ui/gtk/autofill/autofill_popup_view_gtk.h:41: // The height of each individual autofill popup row. On 2012/01/11 23:36:29, Ilya Sherman wrote: > nit: "autofill" -> "Autofill" Done. http://codereview.chromium.org/9187009/diff/4001/chrome/chrome_browser.gypi File chrome/chrome_browser.gypi (right): http://codereview.chromium.org/9187009/diff/4001/chrome/chrome_browser.gypi#n... chrome/chrome_browser.gypi:3371: 'browser/ui/views/autofill/autofill_popup_contents_view.h', On 2012/01/11 23:36:29, Ilya Sherman wrote: > Hmm, I don't see these files among the ones uploaded for the CL. Oops, removed.
one more thing. http://codereview.chromium.org/9187009/diff/4001/chrome/browser/ui/gtk/autofi... File chrome/browser/ui/gtk/autofill/autofill_popup_view_gtk.cc (right): http://codereview.chromium.org/9187009/diff/4001/chrome/browser/ui/gtk/autofi... chrome/browser/ui/gtk/autofill/autofill_popup_view_gtk.cc:128: COMPILE_ASSERT(kBorderThickness == 1, border_1px_implied); On 2012/01/11 23:36:29, Ilya Sherman wrote: > Huh, I've never seen this macro used before. Nifty! OT: I'm using it in profile_impl to make sure that object's size doesn't regress. http://codereview.chromium.org/9187009/diff/7001/chrome/browser/ui/gtk/autofi... File chrome/browser/ui/gtk/autofill/autofill_popup_view_gtk.cc (right): http://codereview.chromium.org/9187009/diff/7001/chrome/browser/ui/gtk/autofi... chrome/browser/ui/gtk/autofill/autofill_popup_view_gtk.cc:28: gdk_drawable_get_size(GDK_DRAWABLE(window), &width, &height); include gtk_compat.h and use gdk_window_get_width/height(). GDK_DRAWABLE is going away. (Also, these method looks copypastad from the omnibox version. While GetRectForItemByIndex is specialized enough, GetWindowRect and SetTextToDraw probably either have common implementations in gtk_util.h, or should be moved there. You may want to rename SetTextToDraw to be more descriptive.) http://codereview.chromium.org/9187009/diff/7001/chrome/browser/ui/gtk/autofi... chrome/browser/ui/gtk/autofill/autofill_popup_view_gtk.cc:120: cairo_t* cr = gdk_cairo_create(GDK_DRAWABLE(gtk_widget_get_window(widget))); When I said above to cut GDK_DRAWABLE, please don't cut this cast. The drawable type still exists today. (FYI: GTK 3 will rework things so expose methods receive a cairo_t clipped to the expose area instead of a GdkEventExpose* that we have to manually take care of.)
http://codereview.chromium.org/9187009/diff/4001/chrome/browser/ui/gtk/autofi... File chrome/browser/ui/gtk/autofill/autofill_popup_view_gtk.cc (right): http://codereview.chromium.org/9187009/diff/4001/chrome/browser/ui/gtk/autofi... chrome/browser/ui/gtk/autofill/autofill_popup_view_gtk.cc:68: row_height_ = font_.GetHeight(); On 2012/01/12 19:39:11, csharp wrote: > On 2012/01/11 23:36:29, Ilya Sherman wrote: > > Hmm, where is the font_ initialized? > > I just use the default constructor. Should I still have it listed in the > initialization list as font_()? No, this is fine as is. It just seemed a little weird to me that you were caching a property of the default font -- wanted to make sure that's what you intended :) http://codereview.chromium.org/9187009/diff/4001/chrome/browser/ui/gtk/autofi... chrome/browser/ui/gtk/autofill/autofill_popup_view_gtk.cc:160: line_rect.y() + ((row_height_ - actual_content_height) / 2)); On 2012/01/12 19:39:11, csharp wrote: > On 2012/01/11 23:36:29, Ilya Sherman wrote: > > I don't understand what this computation does -- wouldn't (row_height_ - > > actual_content_height) pretty much always be negative? > > It will be positive because row_height_ >= actual_content_height. row_height_ is > the size of the row and the actual_content_height is how much space the text > will take. Hmm, where is actual_content_height updated to be the size of the text? It looks to me like actual_content_width and actual_content_height are set to the dimensions of the entire frame... is that not so? http://codereview.chromium.org/9187009/diff/7001/chrome/browser/autofill/auto... File chrome/browser/autofill/autofill_popup_view.h (right): http://codereview.chromium.org/9187009/diff/7001/chrome/browser/autofill/auto... chrome/browser/autofill/autofill_popup_view.h:30: // Platform-Independent work. nit: "Independent" -> "independent" http://codereview.chromium.org/9187009/diff/7001/chrome/browser/ui/gtk/autofi... File chrome/browser/ui/gtk/autofill/autofill_popup_view_gtk.cc (right): http://codereview.chromium.org/9187009/diff/7001/chrome/browser/ui/gtk/autofi... chrome/browser/ui/gtk/autofill/autofill_popup_view_gtk.cc:33: gfx::Rect GetRectForItemByIndex(size_t index, int width, int height) { nit: Another possible (shorter!) name: GetRectForRow(). (Up to you if you prefer this or the existing name.)
http://codereview.chromium.org/9187009/diff/4001/chrome/browser/ui/gtk/autofi... File chrome/browser/ui/gtk/autofill/autofill_popup_view_gtk.cc (right): http://codereview.chromium.org/9187009/diff/4001/chrome/browser/ui/gtk/autofi... chrome/browser/ui/gtk/autofill/autofill_popup_view_gtk.cc:160: line_rect.y() + ((row_height_ - actual_content_height) / 2)); On 2012/01/13 02:45:36, Ilya Sherman wrote: > On 2012/01/12 19:39:11, csharp wrote: > > On 2012/01/11 23:36:29, Ilya Sherman wrote: > > > I don't understand what this computation does -- wouldn't (row_height_ - > > > actual_content_height) pretty much always be negative? > > > > It will be positive because row_height_ >= actual_content_height. row_height_ > is > > the size of the row and the actual_content_height is how much space the text > > will take. > > Hmm, where is actual_content_height updated to be the size of the text? It > looks to me like actual_content_width and actual_content_height are set to the > dimensions of the entire frame... is that not so? When I checked with the debugger the value for height was not the entire frame, just a single line. I tried looking into the docs more to be sure but they don't seem too explicit on this. If it ever does become the whole frame and make the result negative, it shouldn't have an effect, since we'd then just take line_rect.y() Maybe Elliot knows why? http://codereview.chromium.org/9187009/diff/7001/chrome/browser/autofill/auto... File chrome/browser/autofill/autofill_popup_view.h (right): http://codereview.chromium.org/9187009/diff/7001/chrome/browser/autofill/auto... chrome/browser/autofill/autofill_popup_view.h:30: // Platform-Independent work. On 2012/01/13 02:45:37, Ilya Sherman wrote: > nit: "Independent" -> "independent" Done. http://codereview.chromium.org/9187009/diff/7001/chrome/browser/ui/gtk/autofi... File chrome/browser/ui/gtk/autofill/autofill_popup_view_gtk.cc (right): http://codereview.chromium.org/9187009/diff/7001/chrome/browser/ui/gtk/autofi... chrome/browser/ui/gtk/autofill/autofill_popup_view_gtk.cc:28: gdk_drawable_get_size(GDK_DRAWABLE(window), &width, &height); On 2012/01/12 20:34:11, Elliot Glaysher wrote: > include gtk_compat.h and use gdk_window_get_width/height(). GDK_DRAWABLE is > going away. > > (Also, these method looks copypastad from the omnibox version. While > GetRectForItemByIndex is specialized enough, GetWindowRect and SetTextToDraw > probably either have common implementations in gtk_util.h, or should be moved > there. You may want to rename SetTextToDraw to be more descriptive.) Done. I moved SetTextToDraw to gtk_util and renamed it. I tried to move GetWindowRect there too, but got an error. In this file GtkWindow* is actually GdkDrawable, and in gtk_util it is _GtkWindow. I figure leaving the code here is better then adding an explicit reference to GdkDrawable in gtk_util http://codereview.chromium.org/9187009/diff/7001/chrome/browser/ui/gtk/autofi... chrome/browser/ui/gtk/autofill/autofill_popup_view_gtk.cc:33: gfx::Rect GetRectForItemByIndex(size_t index, int width, int height) { On 2012/01/13 02:45:37, Ilya Sherman wrote: > nit: Another possible (shorter!) name: GetRectForRow(). (Up to you if you > prefer this or the existing name.) Done.
lgtm http://codereview.chromium.org/9187009/diff/4001/chrome/browser/ui/gtk/autofi... File chrome/browser/ui/gtk/autofill/autofill_popup_view_gtk.cc (right): http://codereview.chromium.org/9187009/diff/4001/chrome/browser/ui/gtk/autofi... chrome/browser/ui/gtk/autofill/autofill_popup_view_gtk.cc:160: line_rect.y() + ((row_height_ - actual_content_height) / 2)); On 2012/01/13 18:09:03, csharp wrote: > On 2012/01/13 02:45:36, Ilya Sherman wrote: > > On 2012/01/12 19:39:11, csharp wrote: > > > On 2012/01/11 23:36:29, Ilya Sherman wrote: > > > > I don't understand what this computation does -- wouldn't (row_height_ - > > > > actual_content_height) pretty much always be negative? > > > > > > It will be positive because row_height_ >= actual_content_height. > row_height_ > > is > > > the size of the row and the actual_content_height is how much space the text > > > will take. > > > > Hmm, where is actual_content_height updated to be the size of the text? It > > looks to me like actual_content_width and actual_content_height are set to the > > dimensions of the entire frame... is that not so? > > When I checked with the debugger the value for height was not the entire frame, > just a single line. I tried looking into the docs more to be sure but they don't > seem too explicit on this. If it ever does become the whole frame and make the > result negative, it shouldn't have an effect, since we'd then just take > line_rect.y() > > Maybe Elliot knows why? I unfortunately don't. And the original author of this code has left Google in the interim.
LGTM. Thanks for your patience on this review. http://codereview.chromium.org/9187009/diff/15001/chrome/browser/ui/gtk/autof... File chrome/browser/ui/gtk/autofill/autofill_popup_view_gtk.cc (right): http://codereview.chromium.org/9187009/diff/15001/chrome/browser/ui/gtk/autof... chrome/browser/ui/gtk/autofill/autofill_popup_view_gtk.cc:21: // The amount of minimum padding between the autofill value and label in pixels. nit: "autofill" -> "Autofill" http://codereview.chromium.org/9187009/diff/15001/chrome/browser/ui/gtk/autof... chrome/browser/ui/gtk/autofill/autofill_popup_view_gtk.cc:80: // size so it will need to be include in the calculation. nit: "include" -> "included" http://codereview.chromium.org/9187009/diff/15001/chrome/browser/ui/gtk/autof... chrome/browser/ui/gtk/autofill/autofill_popup_view_gtk.cc:148: // Draw the autofill value. nit: "autofill" -> "Autofill" (or just omit "Autofill" entirely) http://codereview.chromium.org/9187009/diff/15001/chrome/browser/ui/gtk/autof... chrome/browser/ui/gtk/autofill/autofill_popup_view_gtk.cc:156: // Draw the autofill label. nit: Ditto http://codereview.chromium.org/9187009/diff/15001/chrome/browser/ui/gtk/autof... File chrome/browser/ui/gtk/autofill/autofill_popup_view_gtk.h (right): http://codereview.chromium.org/9187009/diff/15001/chrome/browser/ui/gtk/autof... chrome/browser/ui/gtk/autofill/autofill_popup_view_gtk.h:9: #include <gtk/gtk.h> nit: Just noticed -- can the GTK classes be forward-declared rather than #included here? http://codereview.chromium.org/9187009/diff/15001/chrome/browser/ui/gtk/autof... chrome/browser/ui/gtk/autofill/autofill_popup_view_gtk.h:12: #include "ui/base/gtk/gtk_signal.h" Ditto http://codereview.chromium.org/9187009/diff/15001/chrome/browser/ui/gtk/autof... chrome/browser/ui/gtk/autofill/autofill_popup_view_gtk.h:14: #include "ui/gfx/rect.h" nit: Ditto (for rect.h; font.h clearly needs to be #included) http://codereview.chromium.org/9187009/diff/15001/chrome/browser/ui/gtk/autof... chrome/browser/ui/gtk/autofill/autofill_popup_view_gtk.h:19: GtkWidget* parent); nit: If this still fits on one line, no need to wrap the line. (It's kind of a weird bit of the style guide, but if *all* the parameters fit on one line, it's fine to keep them all on one line. Otherwise, they should be one per line [but in header files only; either way is fine in implementation files].) http://codereview.chromium.org/9187009/diff/15001/chrome/browser/ui/gtk/gtk_u... File chrome/browser/ui/gtk/gtk_util.h (right): http://codereview.chromium.org/9187009/diff/15001/chrome/browser/ui/gtk/gtk_u... chrome/browser/ui/gtk/gtk_util.h:210: void SetLayoutText(PangoLayout* layout, const string16& text); nit: Are you adding this here so that the Omnibox code can use it as well? If so, let's go ahead and update the Omnibox code in this CL as well. Otherwise, if the Autofill popup code is the only client, it's probably not worth having this live in gtk_util.
http://codereview.chromium.org/9187009/diff/15001/chrome/browser/ui/gtk/autof... File chrome/browser/ui/gtk/autofill/autofill_popup_view_gtk.cc (right): http://codereview.chromium.org/9187009/diff/15001/chrome/browser/ui/gtk/autof... chrome/browser/ui/gtk/autofill/autofill_popup_view_gtk.cc:21: // The amount of minimum padding between the autofill value and label in pixels. On 2012/01/20 07:26:47, Ilya Sherman wrote: > nit: "autofill" -> "Autofill" Done. http://codereview.chromium.org/9187009/diff/15001/chrome/browser/ui/gtk/autof... chrome/browser/ui/gtk/autofill/autofill_popup_view_gtk.cc:80: // size so it will need to be include in the calculation. On 2012/01/20 07:26:47, Ilya Sherman wrote: > nit: "include" -> "included" Done. http://codereview.chromium.org/9187009/diff/15001/chrome/browser/ui/gtk/autof... chrome/browser/ui/gtk/autofill/autofill_popup_view_gtk.cc:148: // Draw the autofill value. On 2012/01/20 07:26:47, Ilya Sherman wrote: > nit: "autofill" -> "Autofill" (or just omit "Autofill" entirely) Done. http://codereview.chromium.org/9187009/diff/15001/chrome/browser/ui/gtk/autof... chrome/browser/ui/gtk/autofill/autofill_popup_view_gtk.cc:156: // Draw the autofill label. On 2012/01/20 07:26:47, Ilya Sherman wrote: > nit: Ditto Done. http://codereview.chromium.org/9187009/diff/15001/chrome/browser/ui/gtk/autof... File chrome/browser/ui/gtk/autofill/autofill_popup_view_gtk.h (right): http://codereview.chromium.org/9187009/diff/15001/chrome/browser/ui/gtk/autof... chrome/browser/ui/gtk/autofill/autofill_popup_view_gtk.h:9: #include <gtk/gtk.h> On 2012/01/20 07:26:47, Ilya Sherman wrote: > nit: Just noticed -- can the GTK classes be forward-declared rather than > #included here? I don't think they can, I tried and ran into error messages like this, /usr/include/gtk-2.0/gdk/gdkevents.h:48: error: 'GdkEventExpose' has a previous declaration here Not exactly sure why, but I suspect it is because I had to include the font header http://codereview.chromium.org/9187009/diff/15001/chrome/browser/ui/gtk/autof... chrome/browser/ui/gtk/autofill/autofill_popup_view_gtk.h:12: #include "ui/base/gtk/gtk_signal.h" On 2012/01/20 07:26:47, Ilya Sherman wrote: > Ditto This is needed to include the define for CHROMEGTK_CALLBACK_1 http://codereview.chromium.org/9187009/diff/15001/chrome/browser/ui/gtk/autof... chrome/browser/ui/gtk/autofill/autofill_popup_view_gtk.h:14: #include "ui/gfx/rect.h" On 2012/01/20 07:26:47, Ilya Sherman wrote: > nit: Ditto (for rect.h; font.h clearly needs to be #included) Done. http://codereview.chromium.org/9187009/diff/15001/chrome/browser/ui/gtk/autof... chrome/browser/ui/gtk/autofill/autofill_popup_view_gtk.h:19: GtkWidget* parent); On 2012/01/20 07:26:47, Ilya Sherman wrote: > nit: If this still fits on one line, no need to wrap the line. (It's kind of a > weird bit of the style guide, but if *all* the parameters fit on one line, it's > fine to keep them all on one line. Otherwise, they should be one per line [but > in header files only; either way is fine in implementation files].) Done. I just forget to clean this up after removing some parameters. http://codereview.chromium.org/9187009/diff/15001/chrome/browser/ui/gtk/gtk_u... File chrome/browser/ui/gtk/gtk_util.h (right): http://codereview.chromium.org/9187009/diff/15001/chrome/browser/ui/gtk/gtk_u... chrome/browser/ui/gtk/gtk_util.h:210: void SetLayoutText(PangoLayout* layout, const string16& text); On 2012/01/20 07:26:47, Ilya Sherman wrote: > nit: Are you adding this here so that the Omnibox code can use it as well? If > so, let's go ahead and update the Omnibox code in this CL as well. Otherwise, > if the Autofill popup code is the only client, it's probably not worth having > this live in gtk_util. The omnibox code is slightly different and I'm not familiar enough with it to be sure that I can modify it to use this code. I moved this function here because Elliot felt it was generic enough to move to the utility class (even with just the one use).
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/csharp@chromium.org/9187009/23001
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/csharp@chromium.org/9187009/24003
Try job failure for 9187009-24003 on mac_rel for step "update". http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=mac_rel&nu... Step "update" is always a major failure. Look at the try server FAQ for more details.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/csharp@chromium.org/9187009/24003
http://codereview.chromium.org/9187009/diff/15001/chrome/browser/ui/gtk/autof... File chrome/browser/ui/gtk/autofill/autofill_popup_view_gtk.h (right): http://codereview.chromium.org/9187009/diff/15001/chrome/browser/ui/gtk/autof... chrome/browser/ui/gtk/autofill/autofill_popup_view_gtk.h:9: #include <gtk/gtk.h> On 2012/01/20 16:20:44, csharp wrote: > On 2012/01/20 07:26:47, Ilya Sherman wrote: > > nit: Just noticed -- can the GTK classes be forward-declared rather than > > #included here? > > I don't think they can, I tried and ran into error messages like this, > /usr/include/gtk-2.0/gdk/gdkevents.h:48: error: 'GdkEventExpose' has a previous > declaration here > > Not exactly sure why, but I suspect it is because I had to include the font > header So: typedef struct _GdkEventExpose GdkEventExpose; typedef struct _GtkWidget GtkWidget; don't work?
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/csharp@chromium.org/9187009/19008
Change committed as 118497
Forgot to publish this draft, the change did make it in to the code just submitted. http://codereview.chromium.org/9187009/diff/15001/chrome/browser/ui/gtk/autof... File chrome/browser/ui/gtk/autofill/autofill_popup_view_gtk.h (right): http://codereview.chromium.org/9187009/diff/15001/chrome/browser/ui/gtk/autof... chrome/browser/ui/gtk/autofill/autofill_popup_view_gtk.h:9: #include <gtk/gtk.h> On 2012/01/20 18:43:59, Elliot Glaysher wrote: > On 2012/01/20 16:20:44, csharp wrote: > > On 2012/01/20 07:26:47, Ilya Sherman wrote: > > > nit: Just noticed -- can the GTK classes be forward-declared rather than > > > #included here? > > > > I don't think they can, I tried and ran into error messages like this, > > /usr/include/gtk-2.0/gdk/gdkevents.h:48: error: 'GdkEventExpose' has a > previous > > declaration here > > > > Not exactly sure why, but I suspect it is because I had to include the font > > header > > So: > > typedef struct _GdkEventExpose GdkEventExpose; > typedef struct _GtkWidget GtkWidget; > > don't work? Oops, they do. I was just using class GtkWidget. Updated the code and resubmitting |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
