Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
9432029:
GTK Keyboard Support for New Autofill (Closed)
|
Created:
8 years, 10 months ago by csharp Modified:
8 years, 8 months ago CC:
chromium-reviews, GeorgeY, yusukes+watch_chromium.org, dhollowa+watch_chromium.org, penghuang+watch_chromium.org, joi+watch-content_chromium.org, darin-cc_chromium.org, dyu1, James Su, Ilya Sherman Base URL:
http://git.chromium.org/chromium/src.git@master Visibility:
Public. |
DescriptionGTK Keyboard Support for New Autofill
Adds keyboard support to the new browser autofill.
BUG=51644
TEST=When using the new autofill, keyboard presses should work as they do in the old autofill
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=129553
Patch Set 1 #Patch Set 2 : Using keyboard listener to get keypresses #
Total comments: 30
Patch Set 3 : #
Total comments: 18
Patch Set 4 : #
Total comments: 6
Patch Set 5 : Modifying wraparound #
Total comments: 6
Patch Set 6 : Fixing Shift #
Total comments: 2
Patch Set 7 : Fixing switch case #
Total comments: 12
Patch Set 8 : #
Total comments: 1
Patch Set 9 : Removing unneeded defn #Patch Set 10 : #Messages
Total messages: 28 (0 generated)
Here is quick a quick change I made to allow the Autofill popup to get keypresses with out stealing focus from the render widget. Basically I just added some functionality to the render widget to allow anyone to request a chance to handle key presses before they are sent to the renderer. I'm not sure if this is the best way to do this, but I figured I'd see if I could make it work quickly and see what people think. I do plan to clean this up, but I figured I'd just get a sanity check on the idea before doing that.
+suzhe / +jam for comments on modifying the RWH key handling stuff.
On 2012/02/22 22:05:11, Elliot Glaysher wrote: > +suzhe / +jam for comments on modifying the RWH key handling stuff. first thing that comes to mind is that the RWHV implementations are an implementation detail of content, which is why they're not exposed to embedders (you'll get a checkdeps failure about this). are you eventually planning on having this functionality on mac/win as well? if so we can consider putting an api to filter these events on the RenderWidgetHost or RenderWidgetHostView interfaces
On 2012/02/23 19:23:44, John Abd-El-Malek wrote: > On 2012/02/22 22:05:11, Elliot Glaysher wrote: > > +suzhe / +jam for comments on modifying the RWH key handling stuff. > > first thing that comes to mind is that the RWHV implementations are an > implementation detail of content, which is why they're not exposed to embedders > (you'll get a checkdeps failure about this). > > are you eventually planning on having this functionality on mac/win as well? if > so we can consider putting an api to filter these events on the RenderWidgetHost > or RenderWidgetHostView interfaces I'm not sure if this will be needed on mac and windows (I think it probably will be needed, but I'm not sure if they allow unfocused element to get keypress), but I would expect that we probably would need this functionality there as well. Is there any preference as to having this live in RenderWidgetHost or RenderWidgetHostView?
If no one has any issues with it I'll add this functionality to render_widget_host_view (since it already has some mouse related functions so it seems the best fit). I'll upload that change in another review and leave this one to just focus on the autofill component after it is submitted.
This code now uses the keyboard listener I add to take the key presses it cares about without requiring focus. All the keys but the delete key should be working properly. I get the delete keys inputs and call an empty function right now because the code was getting a little more complex then I would have liked to add to this review.
https://chromiumcodereview.appspot.com/9432029/diff/7001/chrome/browser/autof... File chrome/browser/autofill/autofill_popup_view.cc (right): https://chromiumcodereview.appspot.com/9432029/diff/7001/chrome/browser/autof... chrome/browser/autofill/autofill_popup_view.cc:24: if (web_contents) { nit: How about "if (!web_contents) return;" at the top of this method instead? https://chromiumcodereview.appspot.com/9432029/diff/7001/chrome/browser/autof... chrome/browser/autofill/autofill_popup_view.cc:79: new_selected_line = -1; nit: Why -1 and not 0? https://chromiumcodereview.appspot.com/9432029/diff/7001/chrome/browser/autof... chrome/browser/autofill/autofill_popup_view.cc:94: if (line < 0 || line >= static_cast<int>(autofill_values_.size())) { Hmm, why is this code reachable? (I.e., what is the user flow that would reach this code?) https://chromiumcodereview.appspot.com/9432029/diff/7001/chrome/browser/autof... File chrome/browser/autofill/autofill_popup_view.h (right): https://chromiumcodereview.appspot.com/9432029/diff/7001/chrome/browser/autof... chrome/browser/autofill/autofill_popup_view.h:58: virtual void UpdatePopupSize() = 0; This method does not seem to be used. If the motivation is to be able to resize the popup once RemoveLine() is implemented, let's hold off on adding it until then. https://chromiumcodereview.appspot.com/9432029/diff/7001/chrome/browser/autof... chrome/browser/autofill/autofill_popup_view.h:82: void IncreaseSelectedLine(); nit: Perhaps "SelectNextLine()"? https://chromiumcodereview.appspot.com/9432029/diff/7001/chrome/browser/autof... chrome/browser/autofill/autofill_popup_view.h:85: void DecreaseSelectedLine(); nit: Perhaps "SelectPreviousLine()"? https://chromiumcodereview.appspot.com/9432029/diff/7001/chrome/browser/autof... chrome/browser/autofill/autofill_popup_view.h:88: void ChooseLine(int line); nit: Perhaps "AcceptLine()", to match the use of "select" and "accept" elsewhere. https://chromiumcodereview.appspot.com/9432029/diff/7001/chrome/browser/autof... chrome/browser/autofill/autofill_popup_view.h:88: void ChooseLine(int line); Can we remove the |line| parameter, and always accept the currently selected line? https://chromiumcodereview.appspot.com/9432029/diff/7001/chrome/browser/ui/gt... File chrome/browser/ui/gtk/autofill/autofill_popup_view_gtk.cc (right): https://chromiumcodereview.appspot.com/9432029/diff/7001/chrome/browser/ui/gt... chrome/browser/ui/gtk/autofill/autofill_popup_view_gtk.cc:19: #include "content/public/browser/web_contents.h" nit: Alpha-order https://chromiumcodereview.appspot.com/9432029/diff/7001/chrome/browser/ui/gt... chrome/browser/ui/gtk/autofill/autofill_popup_view_gtk.cc:86: } nit: No need for curly braces for these case stmts, since you are not declaring any local variables. https://chromiumcodereview.appspot.com/9432029/diff/7001/chrome/browser/ui/gt... chrome/browser/ui/gtk/autofill/autofill_popup_view_gtk.cc:105: RemoveLine(selected_line()); We should only remove the line if the user types shift-delete. As with Enter below, the Autofill popup should only consume the event if there is a line selected. https://chromiumcodereview.appspot.com/9432029/diff/7001/chrome/browser/ui/gt... chrome/browser/ui/gtk/autofill/autofill_popup_view_gtk.cc:111: return true; This should only consume the event if the popup has a line selected. Otherwise, this should bubble normally, which will typically result in the form getting submitted. https://chromiumcodereview.appspot.com/9432029/diff/7001/chrome/browser/ui/gt... File chrome/browser/ui/gtk/autofill/autofill_popup_view_gtk.h (right): https://chromiumcodereview.appspot.com/9432029/diff/7001/chrome/browser/ui/gt... chrome/browser/ui/gtk/autofill/autofill_popup_view_gtk.h:40: virtual bool HandleKeyPressEvent(GdkEventKey* event) OVERRIDE; nit: Can this have private visibility instead? https://chromiumcodereview.appspot.com/9432029/diff/7001/chrome/browser/ui/gt... chrome/browser/ui/gtk/autofill/autofill_popup_view_gtk.h:40: virtual bool HandleKeyPressEvent(GdkEventKey* event) OVERRIDE; nit: Please add a comment like "// KeyboardListener:" above this method declaration to indicate which interface it is implementing.
Apologies for the delay in reviewing this, by the way. For some reason I thought it was more GTK-y and less Autofill-y, so was waiting on Elliot's review.
https://chromiumcodereview.appspot.com/9432029/diff/7001/chrome/browser/autof... File chrome/browser/autofill/autofill_popup_view.cc (right): https://chromiumcodereview.appspot.com/9432029/diff/7001/chrome/browser/autof... chrome/browser/autofill/autofill_popup_view.cc:24: if (web_contents) { On 2012/03/15 20:10:56, Ilya Sherman wrote: > nit: How about "if (!web_contents) return;" at the top of this method instead? Done. https://chromiumcodereview.appspot.com/9432029/diff/7001/chrome/browser/autof... chrome/browser/autofill/autofill_popup_view.cc:79: new_selected_line = -1; On 2012/03/15 20:10:56, Ilya Sherman wrote: > nit: Why -1 and not 0? Because when you go off either edge you have no selection before starting at the other side. https://chromiumcodereview.appspot.com/9432029/diff/7001/chrome/browser/autof... chrome/browser/autofill/autofill_popup_view.cc:94: if (line < 0 || line >= static_cast<int>(autofill_values_.size())) { On 2012/03/15 20:10:56, Ilya Sherman wrote: > Hmm, why is this code reachable? (I.e., what is the user flow that would reach > this code?) Shouldn't be, but I've added a DCHECK to just make sure that the selected line doesn't get out of bounds at some point (there shouldn't be any way for this to happen at the moment). https://chromiumcodereview.appspot.com/9432029/diff/7001/chrome/browser/autof... File chrome/browser/autofill/autofill_popup_view.h (right): https://chromiumcodereview.appspot.com/9432029/diff/7001/chrome/browser/autof... chrome/browser/autofill/autofill_popup_view.h:58: virtual void UpdatePopupSize() = 0; On 2012/03/15 20:10:56, Ilya Sherman wrote: > This method does not seem to be used. If the motivation is to be able to resize > the popup once RemoveLine() is implemented, let's hold off on adding it until > then. Done. https://chromiumcodereview.appspot.com/9432029/diff/7001/chrome/browser/autof... chrome/browser/autofill/autofill_popup_view.h:82: void IncreaseSelectedLine(); On 2012/03/15 20:10:56, Ilya Sherman wrote: > nit: Perhaps "SelectNextLine()"? Done. https://chromiumcodereview.appspot.com/9432029/diff/7001/chrome/browser/autof... chrome/browser/autofill/autofill_popup_view.h:85: void DecreaseSelectedLine(); On 2012/03/15 20:10:56, Ilya Sherman wrote: > nit: Perhaps "SelectPreviousLine()"? Done. https://chromiumcodereview.appspot.com/9432029/diff/7001/chrome/browser/autof... chrome/browser/autofill/autofill_popup_view.h:88: void ChooseLine(int line); On 2012/03/15 20:10:56, Ilya Sherman wrote: > Can we remove the |line| parameter, and always accept the currently selected > line? Done. https://chromiumcodereview.appspot.com/9432029/diff/7001/chrome/browser/ui/gt... File chrome/browser/ui/gtk/autofill/autofill_popup_view_gtk.cc (right): https://chromiumcodereview.appspot.com/9432029/diff/7001/chrome/browser/ui/gt... chrome/browser/ui/gtk/autofill/autofill_popup_view_gtk.cc:19: #include "content/public/browser/web_contents.h" On 2012/03/15 20:10:56, Ilya Sherman wrote: > nit: Alpha-order Done. https://chromiumcodereview.appspot.com/9432029/diff/7001/chrome/browser/ui/gt... chrome/browser/ui/gtk/autofill/autofill_popup_view_gtk.cc:86: } On 2012/03/15 20:10:56, Ilya Sherman wrote: > nit: No need for curly braces for these case stmts, since you are not declaring > any local variables. Done. https://chromiumcodereview.appspot.com/9432029/diff/7001/chrome/browser/ui/gt... chrome/browser/ui/gtk/autofill/autofill_popup_view_gtk.cc:105: RemoveLine(selected_line()); On 2012/03/15 20:10:56, Ilya Sherman wrote: > We should only remove the line if the user types shift-delete. As with Enter > below, the Autofill popup should only consume the event if there is a line > selected. Ok, in my playing around I see that Autofill profiles need shift-delete to be removed, but regular Autocomplete entries just need a delete. Is this the behaviour we want or should both actions require a shift-delete? https://chromiumcodereview.appspot.com/9432029/diff/7001/chrome/browser/ui/gt... chrome/browser/ui/gtk/autofill/autofill_popup_view_gtk.cc:111: return true; On 2012/03/15 20:10:56, Ilya Sherman wrote: > This should only consume the event if the popup has a line selected. Otherwise, > this should bubble normally, which will typically result in the form getting > submitted. Done. https://chromiumcodereview.appspot.com/9432029/diff/7001/chrome/browser/ui/gt... File chrome/browser/ui/gtk/autofill/autofill_popup_view_gtk.h (right): https://chromiumcodereview.appspot.com/9432029/diff/7001/chrome/browser/ui/gt... chrome/browser/ui/gtk/autofill/autofill_popup_view_gtk.h:40: virtual bool HandleKeyPressEvent(GdkEventKey* event) OVERRIDE; On 2012/03/15 20:10:56, Ilya Sherman wrote: > nit: Please add a comment like "// KeyboardListener:" above this method > declaration to indicate which interface it is implementing. Done.
https://chromiumcodereview.appspot.com/9432029/diff/7001/chrome/browser/autof... File chrome/browser/autofill/autofill_popup_view.cc (right): https://chromiumcodereview.appspot.com/9432029/diff/7001/chrome/browser/autof... chrome/browser/autofill/autofill_popup_view.cc:79: new_selected_line = -1; On 2012/03/21 15:32:28, csharp wrote: > On 2012/03/15 20:10:56, Ilya Sherman wrote: > > nit: Why -1 and not 0? > > Because when you go off either edge you have no selection before starting at the > other side. Are you matching a system UI convention, or the existing WebKit popup, or something else? I would not expect to have no selection prior to starting at the other side. https://chromiumcodereview.appspot.com/9432029/diff/7001/chrome/browser/ui/gt... File chrome/browser/ui/gtk/autofill/autofill_popup_view_gtk.cc (right): https://chromiumcodereview.appspot.com/9432029/diff/7001/chrome/browser/ui/gt... chrome/browser/ui/gtk/autofill/autofill_popup_view_gtk.cc:105: RemoveLine(selected_line()); On 2012/03/21 15:32:28, csharp wrote: > On 2012/03/15 20:10:56, Ilya Sherman wrote: > > We should only remove the line if the user types shift-delete. As with Enter > > below, the Autofill popup should only consume the event if there is a line > > selected. > > Ok, in my playing around I see that Autofill profiles need shift-delete to be > removed, but regular Autocomplete entries just need a delete. Is this the > behaviour we want or should both actions require a shift-delete? I wasn't aware that Autocomplete entries behaved differently. I'm pretty sure we want shift-delete to consistently be the way to remove items from our popups (e.g. including the Omnibox popup). https://chromiumcodereview.appspot.com/9432029/diff/14001/chrome/browser/auto... File chrome/browser/autofill/autofill_popup_view.cc (right): https://chromiumcodereview.appspot.com/9432029/diff/14001/chrome/browser/auto... chrome/browser/autofill/autofill_popup_view.cc:19: selected_line_(-1) { nit: Please use the named constant here. https://chromiumcodereview.appspot.com/9432029/diff/14001/chrome/browser/auto... chrome/browser/autofill/autofill_popup_view.cc:95: selected_line_ < static_cast<int>(autofill_values_.size())); nit: Please write this as a DCHECK_GE and DCHECK_LT so that the logged failures will be more detailed in case this expectation ever fails. https://chromiumcodereview.appspot.com/9432029/diff/14001/chrome/browser/ui/g... File chrome/browser/ui/gtk/autofill/autofill_popup_view_gtk.cc (right): https://chromiumcodereview.appspot.com/9432029/diff/14001/chrome/browser/ui/g... chrome/browser/ui/gtk/autofill/autofill_popup_view_gtk.cc:130: SetSelectedLine(line); Is it actually possible that this line was not already selected? If so, how? https://chromiumcodereview.appspot.com/9432029/diff/14001/chrome/browser/ui/g... chrome/browser/ui/gtk/autofill/autofill_popup_view_gtk.cc:234: SetSelectedLine(autofill_values().size() - 1); Is this intentionally limited only to the suggestions, and not to e.g. the "Chrome Autofill settings" entry? https://chromiumcodereview.appspot.com/9432029/diff/14001/chrome/browser/ui/g... chrome/browser/ui/gtk/autofill/autofill_popup_view_gtk.cc:241: if (selected_line() == kNoSelection) { Should this be "!="? https://chromiumcodereview.appspot.com/9432029/diff/14001/chrome/browser/ui/g... chrome/browser/ui/gtk/autofill/autofill_popup_view_gtk.cc:242: RemoveLine(selected_line()); How about instead "return RemoveSelectedLine()", and have the RemoveSelectedLine() method figure out if the line can be removed? https://chromiumcodereview.appspot.com/9432029/diff/14001/chrome/browser/ui/g... chrome/browser/ui/gtk/autofill/autofill_popup_view_gtk.cc:244: } This currently incorrectly falls through if the if-stmt condition is not satisfied. https://chromiumcodereview.appspot.com/9432029/diff/14001/chrome/browser/ui/g... chrome/browser/ui/gtk/autofill/autofill_popup_view_gtk.cc:248: AcceptSelectedLine(); Likewise, how about instead "return AcceptSelectedLine()", and have the AcceptSelectedLine() method figure out if the line can be removed? https://chromiumcodereview.appspot.com/9432029/diff/14001/chrome/browser/ui/g... File chrome/browser/ui/gtk/autofill/autofill_popup_view_gtk.h (right): https://chromiumcodereview.appspot.com/9432029/diff/14001/chrome/browser/ui/g... chrome/browser/ui/gtk/autofill/autofill_popup_view_gtk.h:54: // KeyboardListener implementations. nit: "implementations" -> "implementation"
https://chromiumcodereview.appspot.com/9432029/diff/14001/chrome/browser/auto... File chrome/browser/autofill/autofill_popup_view.cc (right): https://chromiumcodereview.appspot.com/9432029/diff/14001/chrome/browser/auto... chrome/browser/autofill/autofill_popup_view.cc:19: selected_line_(-1) { On 2012/03/21 23:35:30, Ilya Sherman wrote: > nit: Please use the named constant here. Done. https://chromiumcodereview.appspot.com/9432029/diff/14001/chrome/browser/auto... chrome/browser/autofill/autofill_popup_view.cc:95: selected_line_ < static_cast<int>(autofill_values_.size())); On 2012/03/21 23:35:30, Ilya Sherman wrote: > nit: Please write this as a DCHECK_GE and DCHECK_LT so that the logged failures > will be more detailed in case this expectation ever fails. Done. https://chromiumcodereview.appspot.com/9432029/diff/14001/chrome/browser/ui/g... File chrome/browser/ui/gtk/autofill/autofill_popup_view_gtk.cc (right): https://chromiumcodereview.appspot.com/9432029/diff/14001/chrome/browser/ui/g... chrome/browser/ui/gtk/autofill/autofill_popup_view_gtk.cc:130: SetSelectedLine(line); On 2012/03/21 23:35:30, Ilya Sherman wrote: > Is it actually possible that this line was not already selected? If so, how? I'm not sure, I'm mostly concerned that there could be a race condition where the selection event is received before the movement. I'll replace this with a DCHECK. https://chromiumcodereview.appspot.com/9432029/diff/14001/chrome/browser/ui/g... chrome/browser/ui/gtk/autofill/autofill_popup_view_gtk.cc:234: SetSelectedLine(autofill_values().size() - 1); On 2012/03/21 23:35:30, Ilya Sherman wrote: > Is this intentionally limited only to the suggestions, and not to e.g. the > "Chrome Autofill settings" entry? The suggestion list isn't just the suggestions, it includes the other items (warnings, options, etc), so they are included here. (OnSuggestionsReturned is where they are added to the suggestion vector) https://chromiumcodereview.appspot.com/9432029/diff/14001/chrome/browser/ui/g... chrome/browser/ui/gtk/autofill/autofill_popup_view_gtk.cc:241: if (selected_line() == kNoSelection) { On 2012/03/21 23:35:30, Ilya Sherman wrote: > Should this be "!="? Done. https://chromiumcodereview.appspot.com/9432029/diff/14001/chrome/browser/ui/g... chrome/browser/ui/gtk/autofill/autofill_popup_view_gtk.cc:242: RemoveLine(selected_line()); On 2012/03/21 23:35:30, Ilya Sherman wrote: > How about instead "return RemoveSelectedLine()", and have the > RemoveSelectedLine() method figure out if the line can be removed? Done. https://chromiumcodereview.appspot.com/9432029/diff/14001/chrome/browser/ui/g... chrome/browser/ui/gtk/autofill/autofill_popup_view_gtk.cc:244: } On 2012/03/21 23:35:30, Ilya Sherman wrote: > This currently incorrectly falls through if the if-stmt condition is not > satisfied. Done. https://chromiumcodereview.appspot.com/9432029/diff/14001/chrome/browser/ui/g... chrome/browser/ui/gtk/autofill/autofill_popup_view_gtk.cc:248: AcceptSelectedLine(); On 2012/03/21 23:35:30, Ilya Sherman wrote: > Likewise, how about instead "return AcceptSelectedLine()", and have the > AcceptSelectedLine() method figure out if the line can be removed? Done. https://chromiumcodereview.appspot.com/9432029/diff/14001/chrome/browser/ui/g... File chrome/browser/ui/gtk/autofill/autofill_popup_view_gtk.h (right): https://chromiumcodereview.appspot.com/9432029/diff/14001/chrome/browser/ui/g... chrome/browser/ui/gtk/autofill/autofill_popup_view_gtk.h:54: // KeyboardListener implementations. On 2012/03/21 23:35:30, Ilya Sherman wrote: > nit: "implementations" -> "implementation" Done.
https://chromiumcodereview.appspot.com/9432029/diff/7001/chrome/browser/autof... File chrome/browser/autofill/autofill_popup_view.cc (right): https://chromiumcodereview.appspot.com/9432029/diff/7001/chrome/browser/autof... chrome/browser/autofill/autofill_popup_view.cc:79: new_selected_line = -1; On 2012/03/21 23:35:30, Ilya Sherman wrote: > On 2012/03/21 15:32:28, csharp wrote: > > On 2012/03/15 20:10:56, Ilya Sherman wrote: > > > nit: Why -1 and not 0? > > > > Because when you go off either edge you have no selection before starting at > the > > other side. > > Are you matching a system UI convention, or the existing WebKit popup, or > something else? I would not expect to have no selection prior to starting at > the other side. This is the current WebKit Autofill method.
https://chromiumcodereview.appspot.com/9432029/diff/7001/chrome/browser/autof... File chrome/browser/autofill/autofill_popup_view.cc (right): https://chromiumcodereview.appspot.com/9432029/diff/7001/chrome/browser/autof... chrome/browser/autofill/autofill_popup_view.cc:79: new_selected_line = -1; On 2012/03/23 15:15:19, csharp wrote: > On 2012/03/21 23:35:30, Ilya Sherman wrote: > > On 2012/03/21 15:32:28, csharp wrote: > > > On 2012/03/15 20:10:56, Ilya Sherman wrote: > > > > nit: Why -1 and not 0? > > > > > > Because when you go off either edge you have no selection before starting at > > the > > > other side. > > > > Are you matching a system UI convention, or the existing WebKit popup, or > > something else? I would not expect to have no selection prior to starting at > > the other side. > > This is the current WebKit Autofill method. Ok. Unless this matches a system convention, or makes sense for a different reason, I think we shouldn't have this "wrap to no selection" case. Feel free to add reasoning explaining why this is desirable, though, if you have something in mind -- I'm open to being convinced otherwise :) https://chromiumcodereview.appspot.com/9432029/diff/24001/chrome/browser/ui/g... File chrome/browser/ui/gtk/autofill/autofill_popup_view_gtk.cc (right): https://chromiumcodereview.appspot.com/9432029/diff/24001/chrome/browser/ui/g... chrome/browser/ui/gtk/autofill/autofill_popup_view_gtk.cc:241: if (selected_line() != kNoSelection) { nit: I was suggesting that this check be part of RemoveSelectedLine(), so that we don't need to re-implemented it in each platform's keyboard handler code. https://chromiumcodereview.appspot.com/9432029/diff/24001/chrome/browser/ui/g... chrome/browser/ui/gtk/autofill/autofill_popup_view_gtk.cc:247: if (selected_line() != kNoSelection) { nit: Ditto
https://chromiumcodereview.appspot.com/9432029/diff/24001/chrome/browser/ui/g... File chrome/browser/ui/gtk/autofill/autofill_popup_view_gtk.cc (right): https://chromiumcodereview.appspot.com/9432029/diff/24001/chrome/browser/ui/g... chrome/browser/ui/gtk/autofill/autofill_popup_view_gtk.cc:240: case GDK_KP_Delete: Also, it looks like this is still checking for just Delete rather than Shift-Delete -- am I misreading the code?
https://chromiumcodereview.appspot.com/9432029/diff/24001/chrome/browser/ui/g... File chrome/browser/ui/gtk/autofill/autofill_popup_view_gtk.cc (right): https://chromiumcodereview.appspot.com/9432029/diff/24001/chrome/browser/ui/g... chrome/browser/ui/gtk/autofill/autofill_popup_view_gtk.cc:240: case GDK_KP_Delete: On 2012/03/24 00:53:21, Ilya Sherman wrote: > Also, it looks like this is still checking for just Delete rather than > Shift-Delete -- am I misreading the code? Sorry, I'd missed your comment on how normal Autocomplete should also work that way. Fixed. https://chromiumcodereview.appspot.com/9432029/diff/24001/chrome/browser/ui/g... chrome/browser/ui/gtk/autofill/autofill_popup_view_gtk.cc:241: if (selected_line() != kNoSelection) { On 2012/03/23 22:31:13, Ilya Sherman wrote: > nit: I was suggesting that this check be part of RemoveSelectedLine(), so that > we don't need to re-implemented it in each platform's keyboard handler code. Done. https://chromiumcodereview.appspot.com/9432029/diff/24001/chrome/browser/ui/g... chrome/browser/ui/gtk/autofill/autofill_popup_view_gtk.cc:247: if (selected_line() != kNoSelection) { On 2012/03/23 22:31:13, Ilya Sherman wrote: > nit: Ditto Done.
https://chromiumcodereview.appspot.com/9432029/diff/32001/chrome/browser/ui/g... File chrome/browser/ui/gtk/autofill/autofill_popup_view_gtk.cc (right): https://chromiumcodereview.appspot.com/9432029/diff/32001/chrome/browser/ui/g... chrome/browser/ui/gtk/autofill/autofill_popup_view_gtk.cc:244: break; nit: You can just write this all (lines 2241--244) as "return RemoveSelectedLine()" https://chromiumcodereview.appspot.com/9432029/diff/32001/chrome/browser/ui/g... chrome/browser/ui/gtk/autofill/autofill_popup_view_gtk.cc:247: if (event->state == GDK_SHIFT_MASK && AcceptSelectedLine()) { Hmm, Shift should apply to the "Delete" keypress, not to the "Return" one, right? https://chromiumcodereview.appspot.com/9432029/diff/32001/chrome/browser/ui/g... chrome/browser/ui/gtk/autofill/autofill_popup_view_gtk.cc:250: break; nit: Ditto.
https://chromiumcodereview.appspot.com/9432029/diff/32001/chrome/browser/ui/g... File chrome/browser/ui/gtk/autofill/autofill_popup_view_gtk.cc (right): https://chromiumcodereview.appspot.com/9432029/diff/32001/chrome/browser/ui/g... chrome/browser/ui/gtk/autofill/autofill_popup_view_gtk.cc:244: break; On 2012/03/26 23:07:34, Ilya Sherman wrote: > nit: You can just write this all (lines 2241--244) as "return > RemoveSelectedLine()" Done. https://chromiumcodereview.appspot.com/9432029/diff/32001/chrome/browser/ui/g... chrome/browser/ui/gtk/autofill/autofill_popup_view_gtk.cc:247: if (event->state == GDK_SHIFT_MASK && AcceptSelectedLine()) { On 2012/03/26 23:07:34, Ilya Sherman wrote: > Hmm, Shift should apply to the "Delete" keypress, not to the "Return" one, > right? Done. https://chromiumcodereview.appspot.com/9432029/diff/32001/chrome/browser/ui/g... chrome/browser/ui/gtk/autofill/autofill_popup_view_gtk.cc:250: break; On 2012/03/26 23:07:34, Ilya Sherman wrote: > nit: Ditto. Done.
https://chromiumcodereview.appspot.com/9432029/diff/38001/chrome/browser/ui/g... File chrome/browser/ui/gtk/autofill/autofill_popup_view_gtk.cc (right): https://chromiumcodereview.appspot.com/9432029/diff/38001/chrome/browser/ui/g... chrome/browser/ui/gtk/autofill/autofill_popup_view_gtk.cc:242: return RemoveSelectedLine(); This should be "return (event->state == GDK_SHIFT_MASK) && RemoveSelectedLine();" -- otherwise you incorrectly fall through to the cases below...
https://chromiumcodereview.appspot.com/9432029/diff/38001/chrome/browser/ui/g... File chrome/browser/ui/gtk/autofill/autofill_popup_view_gtk.cc (right): https://chromiumcodereview.appspot.com/9432029/diff/38001/chrome/browser/ui/g... chrome/browser/ui/gtk/autofill/autofill_popup_view_gtk.cc:242: return RemoveSelectedLine(); On 2012/03/27 17:52:40, Ilya Sherman wrote: > This should be "return (event->state == GDK_SHIFT_MASK) && > RemoveSelectedLine();" -- otherwise you incorrectly fall through to the cases > below... D'oh. Done.
LGTM with a few final nits https://chromiumcodereview.appspot.com/9432029/diff/36006/chrome/browser/auto... File chrome/browser/autofill/autofill_popup_unittest.cc (right): https://chromiumcodereview.appspot.com/9432029/diff/36006/chrome/browser/auto... chrome/browser/autofill/autofill_popup_unittest.cc:5: #include "chrome/browser/autofill/autofill_popup_view.h" nit: This should be alpha-sorted with the other #includes -- it only needs to be promoted to the top for the corresponding implementation file. (We're not quite consistent about this in the codebase, but this is what the style guide recommends.) https://chromiumcodereview.appspot.com/9432029/diff/36006/chrome/browser/auto... chrome/browser/autofill/autofill_popup_unittest.cc:79: EXPECT_EQ(-1, autofill_popup_view_->selected_line()); nit: This seems to be testing an implementation detail. Should the public API perhaps just be that the selected line is less than 0 if there is no selection? https://chromiumcodereview.appspot.com/9432029/diff/36006/chrome/browser/auto... chrome/browser/autofill/autofill_popup_unittest.cc:81: static_cast<int>(autofill_popup_view_->autofill_values().size())); nit: Why is this expectation needed for the test? https://chromiumcodereview.appspot.com/9432029/diff/36006/chrome/browser/auto... chrome/browser/autofill/autofill_popup_unittest.cc:85: EXPECT_EQ(static_cast<int>(autofill_popup_view_->autofill_values().size() -1), nit: "-1" -> "- 1" https://chromiumcodereview.appspot.com/9432029/diff/36006/chrome/browser/auto... File chrome/browser/autofill/autofill_popup_view.h (right): https://chromiumcodereview.appspot.com/9432029/diff/36006/chrome/browser/auto... chrome/browser/autofill/autofill_popup_view.h:91: const int kNoSelection = -1; nit: Please move this into the implementation file. https://chromiumcodereview.appspot.com/9432029/diff/36006/chrome/browser/ui/g... File chrome/browser/ui/gtk/autofill/autofill_popup_view_gtk.h (right): https://chromiumcodereview.appspot.com/9432029/diff/36006/chrome/browser/ui/g... chrome/browser/ui/gtk/autofill/autofill_popup_view_gtk.h:60: // Get width of popup needs by values. nit: "needs" -> "needed"
https://chromiumcodereview.appspot.com/9432029/diff/36006/chrome/browser/auto... File chrome/browser/autofill/autofill_popup_unittest.cc (right): https://chromiumcodereview.appspot.com/9432029/diff/36006/chrome/browser/auto... chrome/browser/autofill/autofill_popup_unittest.cc:5: #include "chrome/browser/autofill/autofill_popup_view.h" On 2012/03/27 18:19:53, Ilya Sherman wrote: > nit: This should be alpha-sorted with the other #includes -- it only needs to be > promoted to the top for the corresponding implementation file. (We're not quite > consistent about this in the codebase, but this is what the style guide > recommends.) Done. https://chromiumcodereview.appspot.com/9432029/diff/36006/chrome/browser/auto... chrome/browser/autofill/autofill_popup_unittest.cc:79: EXPECT_EQ(-1, autofill_popup_view_->selected_line()); On 2012/03/27 18:19:53, Ilya Sherman wrote: > nit: This seems to be testing an implementation detail. Should the public API > perhaps just be that the selected line is less than 0 if there is no selection? Done. https://chromiumcodereview.appspot.com/9432029/diff/36006/chrome/browser/auto... chrome/browser/autofill/autofill_popup_unittest.cc:81: static_cast<int>(autofill_popup_view_->autofill_values().size())); On 2012/03/27 18:19:53, Ilya Sherman wrote: > nit: Why is this expectation needed for the test? I want to make sure there are at least 2 element to ensure that the first and last index aren't the same. Added a comment to explain that. https://chromiumcodereview.appspot.com/9432029/diff/36006/chrome/browser/auto... chrome/browser/autofill/autofill_popup_unittest.cc:85: EXPECT_EQ(static_cast<int>(autofill_popup_view_->autofill_values().size() -1), On 2012/03/27 18:19:53, Ilya Sherman wrote: > nit: "-1" -> "- 1" Done. https://chromiumcodereview.appspot.com/9432029/diff/36006/chrome/browser/auto... File chrome/browser/autofill/autofill_popup_view.h (right): https://chromiumcodereview.appspot.com/9432029/diff/36006/chrome/browser/auto... chrome/browser/autofill/autofill_popup_view.h:91: const int kNoSelection = -1; On 2012/03/27 18:19:53, Ilya Sherman wrote: > nit: Please move this into the implementation file. Done. https://chromiumcodereview.appspot.com/9432029/diff/36006/chrome/browser/ui/g... File chrome/browser/ui/gtk/autofill/autofill_popup_view_gtk.h (right): https://chromiumcodereview.appspot.com/9432029/diff/36006/chrome/browser/ui/g... chrome/browser/ui/gtk/autofill/autofill_popup_view_gtk.h:60: // Get width of popup needs by values. On 2012/03/27 18:19:53, Ilya Sherman wrote: > nit: "needs" -> "needed" Done.
Still LGTM https://chromiumcodereview.appspot.com/9432029/diff/34003/chrome/browser/auto... File chrome/browser/autofill/autofill_popup_view.h (right): https://chromiumcodereview.appspot.com/9432029/diff/34003/chrome/browser/auto... chrome/browser/autofill/autofill_popup_view.h:91: const int kNoSelection; nit: There's no need to have the constant declared in the header -- please move the whole declaration into the implementation file.
lgtm
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/csharp@chromium.org/9432029/42001
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/csharp@chromium.org/9432029/44004
Try job failure for 9432029-44004 (retry) on win_rel for step "browser_tests". It's a second try, previously, step "browser_tests" failed. http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/csharp@chromium.org/9432029/44004
Change committed as 129553 |
