[Password Generation] Trigger confirmation bubble when a password is saved

[Password Generation] Trigger confirmation bubble when a password is saved

BUG=318977
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=284169

[Garrett Casto, 2014-07-16 06:29:01]

https://codereview.chromium.org/399573002/diff/20001/chrome/browser/ui/passwo...
File chrome/browser/ui/passwords/manage_passwords_ui_controller.cc (left):

https://codereview.chromium.org/399573002/diff/20001/chrome/browser/ui/passwo...
chrome/browser/ui/passwords/manage_passwords_ui_controller.cc:213:
icon->SetState(password_manager::ui::PENDING_PASSWORD_STATE);
This code make it seems like the views only expect to see
PENDING_PASSWORD_STATE, but that doesn't seem to be the case since
IsPendingState() is used to check for this state. I'm fine adding something to
convert states here as necessary, but I wasn't sure if it was actually
necessary. It is a little confusing that state seems to be used to both
communicate information within the controller, which the views shouldn't care
about, and to communicate useful information with the view.

https://codereview.chromium.org/399573002/diff/20001/chrome/browser/ui/passwo...
File chrome/browser/ui/passwords/manage_passwords_ui_controller.h (right):

https://codereview.chromium.org/399573002/diff/20001/chrome/browser/ui/passwo...
chrome/browser/ui/passwords/manage_passwords_ui_controller.h:43: void
ShowSaveConfirmation(
I don't like the fact that this name is prescriptive while the others are
descriptive.  It could be changed to something like
OnGeneratedPasswordSubmitted(), but I was hoping for a name that makes sense for
both generated and auto saved passwords. Suggestions welcome.

[Mike West, 2014-07-16 07:55:03]

Did UI approve this? It seems a bit odd to reuse the manage passwords bubble; at
least I would have expected an additional string to be injected somewhere, or a
different title for the bubble or something.

As it is, the bubble's going to open after submission but it's not clear to me
that the user is going to understand what's going on, and why the bubble opened.

https://codereview.chromium.org/399573002/diff/20001/chrome/browser/ui/passwo...
File chrome/browser/ui/passwords/manage_passwords_bubble_model.cc (left):

https://codereview.chromium.org/399573002/diff/20001/chrome/browser/ui/passwo...
chrome/browser/ui/passwords/manage_passwords_bubble_model.cc:62:
DCHECK(password_manager::ui::IsPendingState(state_));
Did you intentionally drop the DCHECK?

https://codereview.chromium.org/399573002/diff/20001/chrome/browser/ui/passwo...
File chrome/browser/ui/passwords/manage_passwords_bubble_model.cc (right):

https://codereview.chromium.org/399573002/diff/20001/chrome/browser/ui/passwo...
chrome/browser/ui/passwords/manage_passwords_bubble_model.cc:18: namespace
metrics_util = password_manager::metrics_util;
Nit: Next time I'd suggest doing this kind of cleanup in a separate CL; it makes
the diff more difficult to read than it ought to be. :)

https://codereview.chromium.org/399573002/diff/20001/chrome/browser/ui/passwo...
File chrome/browser/ui/passwords/manage_passwords_ui_controller.cc (left):

https://codereview.chromium.org/399573002/diff/20001/chrome/browser/ui/passwo...
chrome/browser/ui/passwords/manage_passwords_ui_controller.cc:213:
icon->SetState(password_manager::ui::PENDING_PASSWORD_STATE);
On 2014/07/16 06:29:00, Garrett Casto wrote:
> This code make it seems like the views only expect to see
> PENDING_PASSWORD_STATE, but that doesn't seem to be the case since
> IsPendingState() is used to check for this state.

Not doing the transition here means that the icon and controller's status_
properties get out of sync once the bubble is opened. That's unfortunate.

> I'm fine adding something to
> convert states here as necessary, but I wasn't sure if it was actually
> necessary. 

It surprises me that the metrics tests pass, as I was pretty sure we needed this
transition. Hence the comment. Hrm.

> It is a little confusing that state seems to be used to both
> communicate information within the controller, which the views shouldn't care
> about, and to communicate useful information with the view.

Indeed. I thought that was worth it, as it means that we're able to use a single
enum for all the state in all the pieces of the bubble, thereby representing the
state of the entire system. I agree that the transitions make it potentially
confusing, and probably violates some view/controller assumptions.

If you have a suggestion for improvement here, I'm happy to review it. :)

https://codereview.chromium.org/399573002/diff/20001/chrome/browser/ui/passwo...
File chrome/browser/ui/passwords/manage_passwords_ui_controller.h (right):

https://codereview.chromium.org/399573002/diff/20001/chrome/browser/ui/passwo...
chrome/browser/ui/passwords/manage_passwords_ui_controller.h:43: void
ShowSaveConfirmation(
On 2014/07/16 06:29:00, Garrett Casto wrote:
> I don't like the fact that this name is prescriptive while the others are
> descriptive.  It could be changed to something like
> OnGeneratedPasswordSubmitted(), but I was hoping for a name that makes sense
for
> both generated and auto saved passwords. Suggestions welcome.

"OnAutomaticPasswordSave"? Best I can come up with off the top of my head. :)

https://codereview.chromium.org/399573002/diff/20001/chrome/browser/ui/passwo...
chrome/browser/ui/passwords/manage_passwords_ui_controller.h:113: // physically
const, but the contents of the maps should be treated as const.
Would it be worth creating an `autofill::ConstPasswordFormMap` or something
similar in a future CL?

https://codereview.chromium.org/399573002/diff/20001/components/password_mana...
File components/password_manager/core/browser/password_manager.cc (right):

https://codereview.chromium.org/399573002/diff/20001/components/password_mana...
components/password_manager/core/browser/password_manager.cc:464:
client_->ShowPasswordSaveConfirmation(provisional_save_manager_.Pass());
'Pass()' or 'release()'? If 'Pass()' is what we mean here, please change #457 as
well.

[Garrett Casto, 2014-07-17 06:42:46]

Per the question about mocks, turns out I'm a bit of an idiot. You can see them
at 
https://drive.google.com/a/google.com/folderview?id=0B-Vmdj5n-YddVWhXdEpNUUd0....
In an earlier iteration the confirmation dialog looked pretty similar to the
manage passwords dialog, so when I was flipping through these earlier I made the
mistake of thinking they were the same. Updated the CL to have a real
confirmation bubble and make the flow match the mocks (icon will pop manage
bubble after confirmation is dismissed).

https://codereview.chromium.org/399573002/diff/20001/chrome/browser/ui/passwo...
File chrome/browser/ui/passwords/manage_passwords_bubble_model.cc (left):

https://codereview.chromium.org/399573002/diff/20001/chrome/browser/ui/passwo...
chrome/browser/ui/passwords/manage_passwords_bubble_model.cc:62:
DCHECK(password_manager::ui::IsPendingState(state_));
On 2014/07/16 07:55:03, Mike West wrote:
> Did you intentionally drop the DCHECK?

Yes, you can end up with CONFIRMATION_STATE here, which isn't a pending state.

https://codereview.chromium.org/399573002/diff/20001/chrome/browser/ui/passwo...
File chrome/browser/ui/passwords/manage_passwords_bubble_model.cc (right):

https://codereview.chromium.org/399573002/diff/20001/chrome/browser/ui/passwo...
chrome/browser/ui/passwords/manage_passwords_bubble_model.cc:18: namespace
metrics_util = password_manager::metrics_util;
On 2014/07/16 07:55:03, Mike West wrote:
> Nit: Next time I'd suggest doing this kind of cleanup in a separate CL; it
makes
> the diff more difficult to read than it ought to be. :)

Yeah, sorry. I did this because I had a line that was over chars unless I
wrapped in mid namespace, and I thought that this was a better solution.

https://codereview.chromium.org/399573002/diff/20001/chrome/browser/ui/passwo...
File chrome/browser/ui/passwords/manage_passwords_ui_controller.cc (left):

https://codereview.chromium.org/399573002/diff/20001/chrome/browser/ui/passwo...
chrome/browser/ui/passwords/manage_passwords_ui_controller.cc:213:
icon->SetState(password_manager::ui::PENDING_PASSWORD_STATE);
On 2014/07/16 07:55:03, Mike West wrote:
> On 2014/07/16 06:29:00, Garrett Casto wrote:
> > This code make it seems like the views only expect to see
> > PENDING_PASSWORD_STATE, but that doesn't seem to be the case since
> > IsPendingState() is used to check for this state.
> 
> Not doing the transition here means that the icon and controller's status_
> properties get out of sync once the bubble is opened. That's unfortunate.
> 

Part of the reason that I was wondering about this is that
ManagePasswordViewIcon specifically looks for PENDING_PASSWORD_AND_BUBBLE_STATE
at the moment. So it seemed like one or the other needed to change. Should we
just force layout in the parent of the icon on every update? That's what is
actually happening currently anyway because of a bug. I'll make that change for
now.

> > I'm fine adding something to
> > convert states here as necessary, but I wasn't sure if it was actually
> > necessary. 
> 
> It surprises me that the metrics tests pass, as I was pretty sure we needed
this
> transition. Hence the comment. Hrm.
> 
> > It is a little confusing that state seems to be used to both
> > communicate information within the controller, which the views shouldn't
care
> > about, and to communicate useful information with the view.
> 
> Indeed. I thought that was worth it, as it means that we're able to use a
single
> enum for all the state in all the pieces of the bubble, thereby representing
the
> state of the entire system. I agree that the transitions make it potentially
> confusing, and probably violates some view/controller assumptions.
> 
> If you have a suggestion for improvement here, I'm happy to review it. :)

I'm fine with the way it is, I just wanted to make sure that I understood the
intent, because given the current setup it seemed like not changing the state
seemed possible, and even preferred in the case of the icon.

https://codereview.chromium.org/399573002/diff/20001/chrome/browser/ui/passwo...
File chrome/browser/ui/passwords/manage_passwords_ui_controller.h (right):

https://codereview.chromium.org/399573002/diff/20001/chrome/browser/ui/passwo...
chrome/browser/ui/passwords/manage_passwords_ui_controller.h:43: void
ShowSaveConfirmation(
On 2014/07/16 07:55:03, Mike West wrote:
> On 2014/07/16 06:29:00, Garrett Casto wrote:
> > I don't like the fact that this name is prescriptive while the others are
> > descriptive.  It could be changed to something like
> > OnGeneratedPasswordSubmitted(), but I was hoping for a name that makes sense
> for
> > both generated and auto saved passwords. Suggestions welcome.
> 
> "OnAutomaticPasswordSave"? Best I can come up with off the top of my head. :)

It'll do. Changed.

https://codereview.chromium.org/399573002/diff/20001/chrome/browser/ui/passwo...
chrome/browser/ui/passwords/manage_passwords_ui_controller.h:113: // physically
const, but the contents of the maps should be treated as const.
On 2014/07/16 07:55:03, Mike West wrote:
> Would it be worth creating an `autofill::ConstPasswordFormMap` or something
> similar in a future CL?

I thought about that, but was wasn't sure how viral the change would be. Turns
out it's not to bad. I feel like there is probably a one line way to convert
PasswordFormMap -> ConstPasswordFormMap, but I'm not sure what it is.

https://codereview.chromium.org/399573002/diff/20001/components/password_mana...
File components/password_manager/core/browser/password_manager.cc (right):

https://codereview.chromium.org/399573002/diff/20001/components/password_mana...
components/password_manager/core/browser/password_manager.cc:464:
client_->ShowPasswordSaveConfirmation(provisional_save_manager_.Pass());
On 2014/07/16 07:55:03, Mike West wrote:
> 'Pass()' or 'release()'? If 'Pass()' is what we mean here, please change #457
as
> well.

Pass(). Being explicit about pointer ownership is good. Updated
PromptUserToSavePassword() to match.

[Mike West, 2014-07-17 13:43:18]

LGTM if you can make the bots happy. Thanks for taking another pass.

(Next time, please do refactorings like '.Pass' in a separate CL. It's totally a
great change, it just complicates the diff. :) )

https://codereview.chromium.org/399573002/diff/40001/chrome/browser/ui/passwo...
File chrome/browser/ui/passwords/manage_passwords_bubble_model.cc (right):

https://codereview.chromium.org/399573002/diff/40001/chrome/browser/ui/passwo...
chrome/browser/ui/passwords/manage_passwords_bubble_model.cc:58:
save_confirmation_text_ = JoinString(pieces, base::string16());
Nit: This could probably be split out to a helper for clarity.

https://codereview.chromium.org/399573002/diff/40001/chrome/browser/ui/passwo...
chrome/browser/ui/passwords/manage_passwords_bubble_model.cc:133:
dismissal_reason_ = metrics_util::CLICKED_OK;
Since we now have "done" and "ok", we should probably rename them to something
more usably descriptive, as it's not going to be clear to me which is which next
time I look at this code. :)

Not in this CL, though, it's already big and refactory enough.

https://codereview.chromium.org/399573002/diff/40001/chrome/browser/ui/passwo...
chrome/browser/ui/passwords/manage_passwords_bubble_model.cc:143:
dismissal_reason_ = metrics_util::CLICKED_MANAGE;
Do we want to treat these as the same? If we're splitting "done" and "ok", we
should split these too. I'm equally happy to just treat them both as manage, and
both the "please go away, bubble." buttons as the same as well. No idea if the
granularity is useful.

https://codereview.chromium.org/399573002/diff/40001/chrome/browser/ui/passwo...
File chrome/browser/ui/passwords/manage_passwords_bubble_model_unittest.cc
(right):

https://codereview.chromium.org/399573002/diff/40001/chrome/browser/ui/passwo...
chrome/browser/ui/passwords/manage_passwords_bubble_model_unittest.cc:56:
autofill::ConstPasswordFormMap map;
Yay.

https://codereview.chromium.org/399573002/diff/40001/chrome/browser/ui/passwo...
File chrome/browser/ui/passwords/manage_passwords_ui_controller.cc (right):

https://codereview.chromium.org/399573002/diff/40001/chrome/browser/ui/passwo...
chrome/browser/ui/passwords/manage_passwords_ui_controller.cc:156: //
TODO(gcasto): Change this to navigate to account central once passwords
I'd either change the name of this method to
"NavgiateToAutoPasswordGenerationLearnMoreURL" or move this comment out to the
.h file to avoid confusion.

https://codereview.chromium.org/399573002/diff/40001/chrome/browser/ui/passwo...
File chrome/browser/ui/passwords/manage_passwords_ui_controller.h (right):

https://codereview.chromium.org/399573002/diff/40001/chrome/browser/ui/passwo...
chrome/browser/ui/passwords/manage_passwords_ui_controller.h:118: //
ManagePasswordsUIControllerMock.
You can kill this bit of the comment, right?

https://codereview.chromium.org/399573002/diff/40001/chrome/browser/ui/views/...
File chrome/browser/ui/views/passwords/manage_passwords_icon_view.cc (left):

https://codereview.chromium.org/399573002/diff/40001/chrome/browser/ui/views/...
chrome/browser/ui/views/passwords/manage_passwords_icon_view.cc:56: if
(password_manager::ui::PENDING_PASSWORD_AND_BUBBLE_STATE) {
This is probably a good change, but what was the bug here that you mentioned? :)

[Garrett Casto, 2014-07-17 20:54:55]

https://codereview.chromium.org/399573002/diff/40001/chrome/browser/ui/passwo...
File chrome/browser/ui/passwords/manage_passwords_bubble_model.cc (right):

https://codereview.chromium.org/399573002/diff/40001/chrome/browser/ui/passwo...
chrome/browser/ui/passwords/manage_passwords_bubble_model.cc:58:
save_confirmation_text_ = JoinString(pieces, base::string16());
On 2014/07/17 13:43:17, Mike West wrote:
> Nit: This could probably be split out to a helper for clarity.

Done.

https://codereview.chromium.org/399573002/diff/40001/chrome/browser/ui/passwo...
chrome/browser/ui/passwords/manage_passwords_bubble_model.cc:133:
dismissal_reason_ = metrics_util::CLICKED_OK;
On 2014/07/17 13:43:17, Mike West wrote:
> Since we now have "done" and "ok", we should probably rename them to something
> more usably descriptive, as it's not going to be clear to me which is which
next
> time I look at this code. :)
> 
> Not in this CL, though, it's already big and refactory enough.

I'll add a TODO to merge these. I don't know if the granularity is worth keeping
track of.

https://codereview.chromium.org/399573002/diff/40001/chrome/browser/ui/passwo...
chrome/browser/ui/passwords/manage_passwords_bubble_model.cc:143:
dismissal_reason_ = metrics_util::CLICKED_MANAGE;
On 2014/07/17 13:43:17, Mike West wrote:
> Do we want to treat these as the same? If we're splitting "done" and "ok", we
> should split these too. I'm equally happy to just treat them both as manage,
and
> both the "please go away, bubble." buttons as the same as well. No idea if the
> granularity is useful.

I'm actually wondering if it's worth adding granularity here. The normal manage
link and AC manage link get shown in pretty different contexts, and I can
imagine users react pretty differently too them. Added a TODO to look into that
as well.

https://codereview.chromium.org/399573002/diff/40001/chrome/browser/ui/passwo...
File chrome/browser/ui/passwords/manage_passwords_ui_controller.cc (right):

https://codereview.chromium.org/399573002/diff/40001/chrome/browser/ui/passwo...
chrome/browser/ui/passwords/manage_passwords_ui_controller.cc:156: //
TODO(gcasto): Change this to navigate to account central once passwords
On 2014/07/17 13:43:17, Mike West wrote:
> I'd either change the name of this method to
> "NavgiateToAutoPasswordGenerationLearnMoreURL" or move this comment out to the
> .h file to avoid confusion.

Moved. I should probably change this name of the placeholder URL to reference AC
as well. Originally we were going to link the help page instead of linking to
AC, thus the name.

https://codereview.chromium.org/399573002/diff/40001/chrome/browser/ui/passwo...
File chrome/browser/ui/passwords/manage_passwords_ui_controller.h (right):

https://codereview.chromium.org/399573002/diff/40001/chrome/browser/ui/passwo...
chrome/browser/ui/passwords/manage_passwords_ui_controller.h:118: //
ManagePasswordsUIControllerMock.
On 2014/07/17 13:43:17, Mike West wrote:
> You can kill this bit of the comment, right?

Yep, done.

https://codereview.chromium.org/399573002/diff/40001/chrome/browser/ui/views/...
File chrome/browser/ui/views/passwords/manage_passwords_icon_view.cc (left):

https://codereview.chromium.org/399573002/diff/40001/chrome/browser/ui/views/...
chrome/browser/ui/views/passwords/manage_passwords_icon_view.cc:56: if
(password_manager::ui::PENDING_PASSWORD_AND_BUBBLE_STATE) {
On 2014/07/17 13:43:17, Mike West wrote:
> This is probably a good change, but what was the bug here that you mentioned?
:)

Should be if (state() == ....). Should be something that the compiler catches,
but I guess it's not trivial since you want to support the "do {} while(0)" case
for macros.

[Garrett Casto, 2014-07-17 20:55:22]

+Ilya for histograms stuff.

[Ilya Sherman, 2014-07-17 21:32:18]

histograms.xml LGTM

[Garrett Casto, 2014-07-17 23:07:04]

The CQ bit was checked by gcasto@chromium.org

[commit-bot: I haz the power, 2014-07-17 23:10:10]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/gcasto@chromium.org/399573002/80001

[commit-bot: I haz the power, 2014-07-18 04:14:11]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  win8_chromium_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/win8_chromium_rel/bui...)
  win_chromium_compile_dbg on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_compile_...)
  win_chromium_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_rel/buil...)
  win_chromium_x64_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_x64_rel/...)
  win_gpu on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/win_gpu/builds/34541)

[commit-bot: I haz the power, 2014-07-18 04:24:06]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-07-18 04:24:07]

Try jobs failed on following builders:
  win8_chromium_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/win8_chromium_rel/bui...)
  win_chromium_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_rel/buil...)

[Garrett Casto, 2014-07-18 05:20:59]

The CQ bit was checked by gcasto@chromium.org

[commit-bot: I haz the power, 2014-07-18 05:21:33]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/gcasto@chromium.org/399573002/100001

[commit-bot: I haz the power, 2014-07-18 08:11:16]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  win8_chromium_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/win8_chromium_rel/bui...)
  win_chromium_compile_dbg on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_compile_...)
  win_chromium_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_rel/buil...)
  win_chromium_x64_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_x64_rel/...)
  win_gpu on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/win_gpu/builds/34643)

[commit-bot: I haz the power, 2014-07-18 08:16:56]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-07-18 08:16:58]

Try jobs failed on following builders:
  win8_chromium_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/win8_chromium_rel/bui...)

[Garrett Casto, 2014-07-18 16:36:01]

The CQ bit was checked by gcasto@chromium.org

[commit-bot: I haz the power, 2014-07-18 16:36:43]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/gcasto@chromium.org/399573002/120001

[commit-bot: I haz the power, 2014-07-18 18:48:48]

Change committed as 284169