New "Save password" bubble for the Credential Manager API.

New "Save password" bubble for the Credential Manager API.

The new bubble replaces the old one in case the Credential Manager API is activated in chrome://flags.

BUG=400674
Committed: https://crrev.com/b7970f8e6fbdc5a6beacde42ffff72e226082534
Cr-Commit-Position: refs/heads/master@{#321549}

[vasilii, 2015-03-19 12:01:55]

vasilii@chromium.org changed reviewers:
+ mkwst@chromium.org

[vasilii, 2015-03-19 12:01:55]

Hi Mike,

please review the CL.

[Mike West, 2015-03-19 12:07:59]

Android compilation failures look real. Will skim the rest in a minute.

[Mike West, 2015-03-19 12:09:08]

On 2015/03/19 at 12:07:59, Mike West wrote:
> Android compilation failures look real. Will skim the rest in a minute.

Also: screenshots? I like screenshots! :)

Also also: Could you add a little more detail to the CL description so that I
have some idea what the code is supposed to be doing? :)

[vasilii, 2015-03-19 12:27:41]

On 2015/03/19 12:09:08, Mike West wrote:
> On 2015/03/19 at 12:07:59, Mike West wrote:
> > Android compilation failures look real. Will skim the rest in a minute.
> 
> Also: screenshots? I like screenshots! :)
> 
> Also also: Could you add a little more detail to the CL description so that I
> have some idea what the code is supposed to be doing? :)

Done. The screenshot is in the bugtracker.

[Mike West, 2015-03-19 12:50:24]

LGTM % nits.

https://codereview.chromium.org/1001553005/diff/20001/chrome/browser/ui/views...
File chrome/browser/ui/views/passwords/manage_passwords_bubble_view.cc (left):

https://codereview.chromium.org/1001553005/diff/20001/chrome/browser/ui/views...
chrome/browser/ui/views/passwords/manage_passwords_bubble_view.cc:51:
SINGLE_VIEW_COLUMN_SET = 0,
Nit: Why kill the assignments? Don't we need to ensure that these are zero-based
for some reason that I've forgotten?

https://codereview.chromium.org/1001553005/diff/20001/chrome/browser/ui/views...
File chrome/browser/ui/views/passwords/manage_passwords_bubble_view.cc (right):

https://codereview.chromium.org/1001553005/diff/20001/chrome/browser/ui/views...
chrome/browser/ui/views/passwords/manage_passwords_bubble_view.cc:451:
views::GridLayout* layout = new views::GridLayout(this);
Nit: Can you DCHECK that the new UI amazingness is enabled?

https://codereview.chromium.org/1001553005/diff/20001/chrome/browser/ui/views...
chrome/browser/ui/views/passwords/manage_passwords_bubble_view.cc:482: // Extra
padding for visual awesomeness.
I smile every time I see this comment. :)

https://codereview.chromium.org/1001553005/diff/20001/chrome/browser/ui/views...
chrome/browser/ui/views/passwords/manage_passwords_bubble_view.cc:497: }
Nit: Don't need {} here.

https://codereview.chromium.org/1001553005/diff/20001/chrome/browser/ui/views...
File chrome/browser/ui/views/passwords/save_account_more_combobox_model.h
(right):

https://codereview.chromium.org/1001553005/diff/20001/chrome/browser/ui/views...
chrome/browser/ui/views/passwords/save_account_more_combobox_model.h:16:
INDEX_MORE,
Nit: `= 0` isn't _necessary_, but it's nice. :)

Nit: It would be nice to have a comment either here or in the constructor that
the order is important, and shouldn't be changed without changing the other.

[vasilii, 2015-03-19 14:48:58]

https://codereview.chromium.org/1001553005/diff/20001/chrome/browser/ui/views...
File chrome/browser/ui/views/passwords/manage_passwords_bubble_view.cc (left):

https://codereview.chromium.org/1001553005/diff/20001/chrome/browser/ui/views...
chrome/browser/ui/views/passwords/manage_passwords_bubble_view.cc:51:
SINGLE_VIEW_COLUMN_SET = 0,
On 2015/03/19 12:50:24, Mike West wrote:
> Nit: Why kill the assignments? Don't we need to ensure that these are
zero-based
> for some reason that I've forgotten?

There is no reason for them to be zero-based. Note that for every single bubble
we instantiate just a subset of these column sets.

https://codereview.chromium.org/1001553005/diff/20001/chrome/browser/ui/views...
File chrome/browser/ui/views/passwords/manage_passwords_bubble_view.cc (right):

https://codereview.chromium.org/1001553005/diff/20001/chrome/browser/ui/views...
chrome/browser/ui/views/passwords/manage_passwords_bubble_view.cc:451:
views::GridLayout* layout = new views::GridLayout(this);
On 2015/03/19 12:50:23, Mike West wrote:
> Nit: Can you DCHECK that the new UI amazingness is enabled?

Yeah. But it's one more line to fix later, right?

https://codereview.chromium.org/1001553005/diff/20001/chrome/browser/ui/views...
chrome/browser/ui/views/passwords/manage_passwords_bubble_view.cc:482: // Extra
padding for visual awesomeness.
On 2015/03/19 12:50:24, Mike West wrote:
> I smile every time I see this comment. :)

Should I write it somewhere here once more? ;-)

https://codereview.chromium.org/1001553005/diff/20001/chrome/browser/ui/views...
chrome/browser/ui/views/passwords/manage_passwords_bubble_view.cc:497: }
On 2015/03/19 12:50:24, Mike West wrote:
> Nit: Don't need {} here.

Done.

https://codereview.chromium.org/1001553005/diff/20001/chrome/browser/ui/views...
File chrome/browser/ui/views/passwords/save_account_more_combobox_model.h
(right):

https://codereview.chromium.org/1001553005/diff/20001/chrome/browser/ui/views...
chrome/browser/ui/views/passwords/save_account_more_combobox_model.h:16:
INDEX_MORE,
On 2015/03/19 12:50:24, Mike West wrote:
> Nit: `= 0` isn't _necessary_, but it's nice. :)
> 
> Nit: It would be nice to have a comment either here or in the constructor that
> the order is important, and shouldn't be changed without changing the other.

Done.

[vasilii, 2015-03-20 10:21:44]

The CQ bit was checked by vasilii@chromium.org

[vasilii, 2015-03-20 10:21:44]

The patchset sent to the CQ was uploaded after l-g-t-m from mkwst@chromium.org
Link to the patchset: https://codereview.chromium.org/1001553005/#ps40001
(title: "comments")

[commit-bot: I haz the power, 2015-03-20 10:21:54]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1001553005/40001

[commit-bot: I haz the power, 2015-03-20 11:26:45]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2015-03-20 11:27:22]

Patchset 3 (id:??) landed as
https://crrev.com/b7970f8e6fbdc5a6beacde42ffff72e226082534
Cr-Commit-Position: refs/heads/master@{#321549}