Upgrade Seperator in New Autofill UI

Upgrade Seperator in New Autofill UI

Modify the new Autofill UI to use WebAutofillClient::MenuItemIDSeparator as the sperator index, which will make it easier to have multiple separators in the future.

BUG=125001
TEST=Unit tests pass


Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=138860

[csharp, 2012-05-22 20:31:04]

I'll add erg@ to review the gtk part once I get the lgtm from you.

[Ilya Sherman, 2012-05-22 20:44:03]

https://chromiumcodereview.appspot.com/10408070/diff/1/chrome/browser/autofil...
File chrome/browser/autofill/autofill_popup_view.h (right):

https://chromiumcodereview.appspot.com/10408070/diff/1/chrome/browser/autofil...
chrome/browser/autofill/autofill_popup_view.h:102: int RowToIndex(size_t row);
This seems much more complex than just storing the separators as items in the
list we're indexing into.  Did you try that approach and decide against it?

[csharp, 2012-05-23 13:56:08]

Code rewritten to hide the row size logic in the implementation specific code.

[Ilya Sherman, 2012-05-23 18:58:08]

LGTM with nits.  Thanks.

https://chromiumcodereview.appspot.com/10408070/diff/4001/chrome/browser/ui/g...
File chrome/browser/ui/gtk/autofill/autofill_popup_view_gtk.cc (right):

https://chromiumcodereview.appspot.com/10408070/diff/4001/chrome/browser/ui/g...
chrome/browser/ui/gtk/autofill/autofill_popup_view_gtk.cc:33: // The bertical
height of a separator in pixels.
nit: "bertical" -> "vertical"

https://chromiumcodereview.appspot.com/10408070/diff/4001/chrome/browser/ui/g...
chrome/browser/ui/gtk/autofill/autofill_popup_view_gtk.cc:180: switch
(autofill_unique_ids()[i]) {
nit: Since there are only two cases, I think this might be clearer as an
if-else.

https://chromiumcodereview.appspot.com/10408070/diff/4001/chrome/browser/ui/g...
chrome/browser/ui/gtk/autofill/autofill_popup_view_gtk.cc:388: return
autofill_unique_ids().size() - 1;
Hmm, I think we might want to return "-1" in this case instead.  I know this
isn't how the WebKit popup currently works, but if your mouse is off the popup,
no row should be selected (this has been a long-standing bug with the WebKit
popup).  It's fine if you want to tackle this bug more generally in a separate
CL, since you'll also need to look at the x-coordinate of the mouse position for
this to work correctly.

https://chromiumcodereview.appspot.com/10408070/diff/4001/chrome/browser/ui/g...
File chrome/browser/ui/gtk/autofill/autofill_popup_view_gtk.h (right):

https://chromiumcodereview.appspot.com/10408070/diff/4001/chrome/browser/ui/g...
chrome/browser/ui/gtk/autofill/autofill_popup_view_gtk.h:66: void
DrawSeparator(cairo_t* cr, const gfx::Rect& separator_rect);
nit: Is there a more meaningful name we can give to the first parameter?  The
meaning of "cr" is completely unclear to me.

[csharp, 2012-05-24 13:40:10]

erg@, can you take a quick look at the gtk files. Thanks

https://chromiumcodereview.appspot.com/10408070/diff/4001/chrome/browser/ui/g...
File chrome/browser/ui/gtk/autofill/autofill_popup_view_gtk.cc (right):

https://chromiumcodereview.appspot.com/10408070/diff/4001/chrome/browser/ui/g...
chrome/browser/ui/gtk/autofill/autofill_popup_view_gtk.cc:33: // The bertical
height of a separator in pixels.
On 2012/05/23 18:58:08, Ilya Sherman wrote:
> nit: "bertical" -> "vertical"

Done.

https://chromiumcodereview.appspot.com/10408070/diff/4001/chrome/browser/ui/g...
chrome/browser/ui/gtk/autofill/autofill_popup_view_gtk.cc:180: switch
(autofill_unique_ids()[i]) {
On 2012/05/23 18:58:08, Ilya Sherman wrote:
> nit: Since there are only two cases, I think this might be clearer as an
> if-else.

Done.

https://chromiumcodereview.appspot.com/10408070/diff/4001/chrome/browser/ui/g...
chrome/browser/ui/gtk/autofill/autofill_popup_view_gtk.cc:388: return
autofill_unique_ids().size() - 1;
On 2012/05/23 18:58:08, Ilya Sherman wrote:
> Hmm, I think we might want to return "-1" in this case instead.  I know this
> isn't how the WebKit popup currently works, but if your mouse is off the
popup,
> no row should be selected (this has been a long-standing bug with the WebKit
> popup).  It's fine if you want to tackle this bug more generally in a separate
> CL, since you'll also need to look at the x-coordinate of the mouse position
for
> this to work correctly.

I'm leaving this as is for now, but I've added a TODO and filed a bug to fix the
behaviour.

https://chromiumcodereview.appspot.com/10408070/diff/4001/chrome/browser/ui/g...
File chrome/browser/ui/gtk/autofill/autofill_popup_view_gtk.h (right):

https://chromiumcodereview.appspot.com/10408070/diff/4001/chrome/browser/ui/g...
chrome/browser/ui/gtk/autofill/autofill_popup_view_gtk.h:66: void
DrawSeparator(cairo_t* cr, const gfx::Rect& separator_rect);
On 2012/05/23 18:58:08, Ilya Sherman wrote:
> nit: Is there a more meaningful name we can give to the first parameter?  The
> meaning of "cr" is completely unclear to me.

Done.

[Elliot Glaysher, 2012-05-24 17:02:12]

quick gtk glance lgtm

[commit-bot: I haz the power, 2012-05-24 17:30:59]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/csharp@chromium.org/10408070/10001

[commit-bot: I haz the power, 2012-05-24 19:29:35]

Change committed as 138860