Adding skeleton code for showing Bubble, asking user to submit URL when Password Manager fails

Adding skeleton code for showing Bubble, asking user to submit URL when Password Manager fails

With this patch work on "Allow to collect URL?" bubble starts. View of bubble is implemented. But
bubble itself never will be shown to a user

BUG=435080
R=vabr@chromium.org

Committed: https://crrev.com/09dd734520dc40ad87a45bc7a85d95c48a608904
Cr-Commit-Position: refs/heads/master@{#307036}

[melandory, 2014-12-03 10:11:18]

Patchset #1 (id:1) has been deleted

[melandory, 2014-12-03 12:29:47]

Patchset #1 (id:20001) has been deleted

[melandory, 2014-12-03 12:55:13]

Patchset #1 (id:40001) has been deleted

[vabr (Chromium), 2014-12-03 14:08:07]

(+sabineb FYI, because I mentioned you in the comments)

Hi Tanja,

I have left a couple of comments for patch set 1. The diff between p.s. 1 and 2
(BlueButton -> LabelButton) looks OK.

One big comment in addition to those below: please add tests. You will find that
Mike created quite extensive unit tests for the bubble, checking the state and
corresponding bubble changes.

Cheers,
Vaclav

https://codereview.chromium.org/765073004/diff/60001/chrome/app/generated_res...
File chrome/app/generated_resources.grd (right):

https://codereview.chromium.org/765073004/diff/60001/chrome/app/generated_res...
chrome/app/generated_resources.grd:7556: +        <message
name="IDS_PASSWORD_MANAGER_SEND_URL_BUTTON" desc="Send URL text for 'Allow to
collect URL?' bubble in password manager">
nit: It is not clear what a "Send URL text" is. What about:
"The label of a button on an 'Allow to collect URL?' bubble; clicking this
button causes a collected URL to be sent to Google."

https://codereview.chromium.org/765073004/diff/60001/chrome/app/generated_res...
chrome/app/generated_resources.grd:13272: +      <message
name="IDS_MANAGE_PASSWORDS_ASK_TO_SUBMIT_URL_TITLE" desc="The title text that is
used in the manage passwords bubble when the password manager failed to detect
the form and user is asked if he wants to report the URL">
I'd remove "and user is asked if he wants to report the URL" from the title
description, leave it for the text description (the title does not speak about
reporting).
Also, "the form" has no previous context, so perhaps "a password form"?

https://codereview.chromium.org/765073004/diff/60001/chrome/app/generated_res...
chrome/app/generated_resources.grd:13273: +      We detected that Chrome
password manager failed to handle this URL.
"We" is ambiguous. What about:
"Chrome password manager failed to detect a password form on this page."

Might be also a good idea to check with sabineb@, if it is better to say just
"Chrome" or "Chrome password manager" (and which capitalisation to use).

https://codereview.chromium.org/765073004/diff/60001/chrome/app/generated_res...
chrome/app/generated_resources.grd:13275: +      <message
name="IDS_MANAGE_PASSWORDS_ASK_TO_SUBMIT_URL_TEXT" desc="The text that is used
in the manage passwords bubble when the password manager failed to detect the
form and user is asked if he wants to report the URL">
To clarify the focus (on reporting, not on disclosing the failure, which
happened in the title), I suggest changing the end to:
"... when the user is asked to report the URL of a page where the password
manager failed to detect a password form."

https://codereview.chromium.org/765073004/diff/60001/chrome/app/generated_res...
chrome/app/generated_resources.grd:13276: +      Do you want to send this URL to
Google to improve Chrome?
This lacks the concrete reason for sending the URL (although it is hinted by the
title). What about:
"Do you want to send this URL to Google to improve password detection in
Chrome?"

https://codereview.chromium.org/765073004/diff/60001/chrome/browser/ui/passwo...
File chrome/browser/ui/passwords/manage_passwords_bubble_model.h (right):

https://codereview.chromium.org/765073004/diff/60001/chrome/browser/ui/passwo...
chrome/browser/ui/passwords/manage_passwords_bubble_model.h:116: GURL origin_;
Please clarify (in a comment), origin of what this is.

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

https://codereview.chromium.org/765073004/diff/60001/chrome/browser/ui/passwo...
chrome/browser/ui/passwords/manage_passwords_ui_controller.cc:96: origin_ =
GURL("http://" + host);
Why do you assume that the scheme is HTTP? It could be HTTPS. It might be a
better idea to pass the whole URL in the argument.

https://codereview.chromium.org/765073004/diff/60001/chrome/browser/ui/passwo...
chrome/browser/ui/passwords/manage_passwords_ui_controller.cc:260: // This
allows "Allow to collect URL?" bubble to leave 1 page longer.
typo: leave -> live (?)
Also, "1 page longer" sounds strange (page is not a time unit). Maybe "outlive
the coming navigation"?

https://codereview.chromium.org/765073004/diff/60001/chrome/browser/ui/passwo...
chrome/browser/ui/passwords/manage_passwords_ui_controller.cc:262: //
TODO(melandory): Substitude this with proper solution using
typo: Substitude -> Substitute
Also: proper solution -> a proper solution

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

https://codereview.chromium.org/765073004/diff/60001/chrome/browser/ui/passwo...
chrome/browser/ui/passwords/manage_passwords_ui_controller.h:36: // of
corresponding experiment. Triggers the UI to ask user if he want to
grammar nit: "he wants" or "they want"
(Note that using they+plural even if the user is singular is a valid way to
avoid specifying the user's gender.)

https://codereview.chromium.org/765073004/diff/60001/chrome/browser/ui/passwo...
chrome/browser/ui/passwords/manage_passwords_ui_controller.h:37: // report the
|host| and maybe report |host| based on user response.
The absence of explicit scopes (parentheses) in the natural language makes this
sentence a bit hard to parse. A comma placed before "and" might help. Or perhaps
rephrasing the whole sentence, e.g., as: "Triggers the UI to allow the user to
report the |host|."

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

https://codereview.chromium.org/765073004/diff/60001/chrome/browser/ui/views/...
chrome/browser/ui/views/passwords/manage_passwords_bubble_view.cc:39: 
nit: As a general rule, formatting changes not related to the rest of the CL
should be kept out of the CL. The goal is to keep the CL as small and focussed
as possible. Please revert removing of the 4 blank lines in this file.

(Whether 1 or 2 blank lines are better on this place is not completely clear
(the style is not strict, and whether 1 or 2 help readability is subjective). I
would avoid the need to discuss this together with your main change by just not
changing it in this CL. :))

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

https://codereview.chromium.org/765073004/diff/60001/chrome/browser/ui/views/...
chrome/browser/ui/views/passwords/manage_passwords_bubble_view.cc:321: // Better
to omit url if it's incorrect or empty.
But what is the purpose of asking the user, if they want to report the origin,
if we cannot show it? The bubble should not show up, unless there is an origin
to show to the user.

https://codereview.chromium.org/765073004/diff/60001/chrome/browser/ui/views/...
chrome/browser/ui/views/passwords/manage_passwords_bubble_view.cc:344:
parent_->set_initially_focused_view(send_button_);
Please make sure that the choice of the default was agreed upon by sabineb@.
Sending the URL is a privacy sensitive action, so we need to be careful with the
default choices.

https://codereview.chromium.org/765073004/diff/60001/components/password_mana...
File components/password_manager/core/common/password_manager_ui.h (right):

https://codereview.chromium.org/765073004/diff/60001/components/password_mana...
components/password_manager/core/common/password_manager_ui.h:48:
ASK_SUBMIT_URL_AND_BUBBLE_STATE,
The name is unclear ("submit URL and bubble"?).
What about ASK_USER_TO_REPORT_URL_STATE?

https://codereview.chromium.org/765073004/diff/60001/components/password_mana...
components/password_manager/core/common/password_manager_ui.h:51:
ASK_SUBMIT_URL_STATE
I don't think the comment and the state name explain sufficiently the purpose.

Is the meaning "ask the user to report the URL on next navigation?"

If so, please be explicit in the state name (and comment). Something like
ASK_USER_TO_REPORT_URL_AFTER_NEXT_LOAD_STATE is long, but probably helpful.

[melandory, 2014-12-04 10:27:48]

Patchset #4 (id:120001) has been deleted

[melandory, 2014-12-04 10:31:59]

Patchset #3 (id:100001) has been deleted

[melandory, 2014-12-04 10:33:47]

Hi Vaclav, 

thanks for reviewing. I discussed with @sabineb strings and default focused
button.


-- 
Cheers,
Tanja

https://codereview.chromium.org/765073004/diff/60001/chrome/app/generated_res...
File chrome/app/generated_resources.grd (right):

https://codereview.chromium.org/765073004/diff/60001/chrome/app/generated_res...
chrome/app/generated_resources.grd:7556: +        <message
name="IDS_PASSWORD_MANAGER_SEND_URL_BUTTON" desc="Send URL text for 'Allow to
collect URL?' bubble in password manager">
On 2014/12/03 14:08:06, vabr (Chromium) wrote:
> nit: It is not clear what a "Send URL text" is. What about:
> "The label of a button on an 'Allow to collect URL?' bubble; clicking this
> button causes a collected URL to be sent to Google."

Done.

https://codereview.chromium.org/765073004/diff/60001/chrome/app/generated_res...
chrome/app/generated_resources.grd:13272: +      <message
name="IDS_MANAGE_PASSWORDS_ASK_TO_SUBMIT_URL_TITLE" desc="The title text that is
used in the manage passwords bubble when the password manager failed to detect
the form and user is asked if he wants to report the URL">
On 2014/12/03 14:08:06, vabr (Chromium) wrote:
> I'd remove "and user is asked if he wants to report the URL" from the title
> description, leave it for the text description (the title does not speak about
> reporting).
> Also, "the form" has no previous context, so perhaps "a password form"?

Done.

https://codereview.chromium.org/765073004/diff/60001/chrome/app/generated_res...
chrome/app/generated_resources.grd:13273: +      We detected that Chrome
password manager failed to handle this URL.
On 2014/12/03 14:08:06, vabr (Chromium) wrote:
> "We" is ambiguous. What about:
> "Chrome password manager failed to detect a password form on this page."
> 
> Might be also a good idea to check with sabineb@, if it is better to say just
> "Chrome" or "Chrome password manager" (and which capitalisation to use).

Done. sabineb@ said that we should use "Chrome password manager"

https://codereview.chromium.org/765073004/diff/60001/chrome/app/generated_res...
chrome/app/generated_resources.grd:13275: +      <message
name="IDS_MANAGE_PASSWORDS_ASK_TO_SUBMIT_URL_TEXT" desc="The text that is used
in the manage passwords bubble when the password manager failed to detect the
form and user is asked if he wants to report the URL">
On 2014/12/03 14:08:06, vabr (Chromium) wrote:
> To clarify the focus (on reporting, not on disclosing the failure, which
> happened in the title), I suggest changing the end to:
> "... when the user is asked to report the URL of a page where the password
> manager failed to detect a password form."

Done.

https://codereview.chromium.org/765073004/diff/60001/chrome/app/generated_res...
chrome/app/generated_resources.grd:13276: +      Do you want to send this URL to
Google to improve Chrome?
On 2014/12/03 14:08:06, vabr (Chromium) wrote:
> This lacks the concrete reason for sending the URL (although it is hinted by
the
> title). What about:
> "Do you want to send this URL to Google to improve password detection in
> Chrome?"

IMHO, "password detection" doesn't sound user-friendly, I'll ask sabineb@ on her
opinion.

Update: sabineb@ propose to use "Do you want to send this URL to Google to
improve Chrome password manager."

https://codereview.chromium.org/765073004/diff/60001/chrome/browser/ui/passwo...
File chrome/browser/ui/passwords/manage_passwords_bubble_model.h (right):

https://codereview.chromium.org/765073004/diff/60001/chrome/browser/ui/passwo...
chrome/browser/ui/passwords/manage_passwords_bubble_model.h:116: GURL origin_;
On 2014/12/03 14:08:06, vabr (Chromium) wrote:
> Please clarify (in a comment), origin of what this is.

Done.

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

https://codereview.chromium.org/765073004/diff/60001/chrome/browser/ui/passwo...
chrome/browser/ui/passwords/manage_passwords_ui_controller.cc:96: origin_ =
GURL("http://" + host);
On 2014/12/03 14:08:06, vabr (Chromium) wrote:
> Why do you assume that the scheme is HTTP? It could be HTTPS. It might be a
> better idea to pass the whole URL in the argument.

I've added "http" in order to form proper GURL and later I do not care is it
http or https because I show only host to user, which doesn't contain any
schema.

I'll pass proper GURL from the beginning, so we can later use more data, like
full URL instead of host only.

Done.

https://codereview.chromium.org/765073004/diff/60001/chrome/browser/ui/passwo...
chrome/browser/ui/passwords/manage_passwords_ui_controller.cc:260: // This
allows "Allow to collect URL?" bubble to leave 1 page longer.
On 2014/12/03 14:08:06, vabr (Chromium) wrote:
> typo: leave -> live (?)
> Also, "1 page longer" sounds strange (page is not a time unit). Maybe "outlive
> the coming navigation"?

Done.

https://codereview.chromium.org/765073004/diff/60001/chrome/browser/ui/passwo...
chrome/browser/ui/passwords/manage_passwords_ui_controller.cc:262: //
TODO(melandory): Substitude this with proper solution using
On 2014/12/03 14:08:06, vabr (Chromium) wrote:
> typo: Substitude -> Substitute
> Also: proper solution -> a proper solution

Done. Also "setlocal spell spelllang=en_us" IS now in my .vimrc, sorry.

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

https://codereview.chromium.org/765073004/diff/60001/chrome/browser/ui/passwo...
chrome/browser/ui/passwords/manage_passwords_ui_controller.h:36: // of
corresponding experiment. Triggers the UI to ask user if he want to
On 2014/12/03 14:08:06, vabr (Chromium) wrote:
> grammar nit: "he wants" or "they want"
> (Note that using they+plural even if the user is singular is a valid way to
> avoid specifying the user's gender.)

Done.

https://codereview.chromium.org/765073004/diff/60001/chrome/browser/ui/passwo...
chrome/browser/ui/passwords/manage_passwords_ui_controller.h:37: // report the
|host| and maybe report |host| based on user response.
On 2014/12/03 14:08:06, vabr (Chromium) wrote:
> The absence of explicit scopes (parentheses) in the natural language makes
this
> sentence a bit hard to parse. A comma placed before "and" might help. Or
perhaps
> rephrasing the whole sentence, e.g., as: "Triggers the UI to allow the user to
> report the |host|."

Done.

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

https://codereview.chromium.org/765073004/diff/60001/chrome/browser/ui/views/...
chrome/browser/ui/views/passwords/manage_passwords_bubble_view.cc:39: 
On 2014/12/03 14:08:06, vabr (Chromium) wrote:
> nit: As a general rule, formatting changes not related to the rest of the CL
> should be kept out of the CL. The goal is to keep the CL as small and focussed
> as possible. Please revert removing of the 4 blank lines in this file.
> 
> (Whether 1 or 2 blank lines are better on this place is not completely clear
> (the style is not strict, and whether 1 or 2 help readability is subjective).
I
> would avoid the need to discuss this together with your main change by just
not
> changing it in this CL. :))

Done.

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

https://codereview.chromium.org/765073004/diff/60001/chrome/browser/ui/views/...
chrome/browser/ui/views/passwords/manage_passwords_bubble_view.cc:321: // Better
to omit url if it's incorrect or empty.
On 2014/12/03 14:08:07, vabr (Chromium) wrote:
> But what is the purpose of asking the user, if they want to report the origin,
> if we cannot show it? The bubble should not show up, unless there is an origin
> to show to the user.

Moved this check to  PasswordManager::RecordFailure when condition for should we
show bubble or not is checked.

Done.

https://codereview.chromium.org/765073004/diff/60001/chrome/browser/ui/views/...
chrome/browser/ui/views/passwords/manage_passwords_bubble_view.cc:344:
parent_->set_initially_focused_view(send_button_);
On 2014/12/03 14:08:06, vabr (Chromium) wrote:
> Please make sure that the choice of the default was agreed upon by sabineb@.
> Sending the URL is a privacy sensitive action, so we need to be careful with
the
> default choices.

sabineb@ said that nope as default is better.
Done.

https://codereview.chromium.org/765073004/diff/60001/components/password_mana...
File components/password_manager/core/common/password_manager_ui.h (right):

https://codereview.chromium.org/765073004/diff/60001/components/password_mana...
components/password_manager/core/common/password_manager_ui.h:48:
ASK_SUBMIT_URL_AND_BUBBLE_STATE,
On 2014/12/03 14:08:07, vabr (Chromium) wrote:
> The name is unclear ("submit URL and bubble"?).
> What about ASK_USER_TO_REPORT_URL_STATE?

Done.

https://codereview.chromium.org/765073004/diff/60001/components/password_mana...
components/password_manager/core/common/password_manager_ui.h:51:
ASK_SUBMIT_URL_STATE
On 2014/12/03 14:08:07, vabr (Chromium) wrote:
> I don't think the comment and the state name explain sufficiently the purpose.
> 
> Is the meaning "ask the user to report the URL on next navigation?"
> 
> If so, please be explicit in the state name (and comment). Something like
> ASK_USER_TO_REPORT_URL_AFTER_NEXT_LOAD_STATE is long, but probably helpful.

Done. Also you after you comment I realize that with current solution of how I
prolong bubble life three states is better than two.

[vabr (Chromium), 2014-12-04 13:25:33]

Thanks, Tanja.

I still need to clarify the meaning of the states, so I'll come and talk to you.
I believe we also need more test coverage for the states (that would also help
understanding the meaning of the states).

Cheers,
Vaclav

https://codereview.chromium.org/765073004/diff/110018/chrome/browser/password...
File chrome/browser/password_manager/chrome_password_manager_client_unittest.cc
(right):

https://codereview.chromium.org/765073004/diff/110018/chrome/browser/password...
chrome/browser/password_manager/chrome_password_manager_client_unittest.cc:236:
// TODO(melandory) Since "Ask user to submit URL" functionality is currently
The TODO comment here seems like a copy&paste mistake. Here we should always
EXPECT_FALSE, no?

https://codereview.chromium.org/765073004/diff/110018/chrome/browser/ui/passw...
File chrome/browser/ui/passwords/manage_passwords_bubble_model.h (right):

https://codereview.chromium.org/765073004/diff/110018/chrome/browser/ui/passw...
chrome/browser/ui/passwords/manage_passwords_bubble_model.h:43: // Call by the
view code when user agreed for URL collection.
grammar nit: agreed for -> agreed to

https://codereview.chromium.org/765073004/diff/110018/chrome/browser/ui/passw...
chrome/browser/ui/passwords/manage_passwords_bubble_model.h:122: // URL of a
page from where bubble was triggered.
grammar nits:
a page -> the page
bubble -> this bubble
(Feel free to override my suggestions with the blessing of any native speaker,
though.)

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

https://codereview.chromium.org/765073004/diff/110018/chrome/browser/ui/views...
chrome/browser/ui/views/passwords/manage_passwords_bubble_view.cc:774: 
nit: There are still 3 removed blank lines in this file (original locations are
lines 774, 777, 921). Please undo those changes.

https://codereview.chromium.org/765073004/diff/110018/components/password_man...
File components/password_manager/core/common/password_manager_ui.h (right):

https://codereview.chromium.org/765073004/diff/110018/components/password_man...
components/password_manager/core/common/password_manager_ui.h:46: // Password
manager failed to detect the form, but we don't need to pop up a
I'm rather confused about the names and meanings of the three new states. I'll
come over to you and ask.

https://codereview.chromium.org/765073004/diff/110018/components/password_man...
components/password_manager/core/common/password_manager_ui.h:48:
ASK_USER_TO_REPORT_URL_STATE,
I find confusing that this is about asking the user, but at the same time we
don't need to pop up a bubble. It looks like a relic from the previous naming
scheme.

Is it at all possible to display this bubble manually, by clicking on the key
icon?

If yes, then we should just rename this and change the code comment on lines
46-47 to something explaining that this is the state just before the bubble is
opened automatically for the sake of asking the user about reporting the URL.

If not, then we should simply merge this state with the next one.

https://codereview.chromium.org/765073004/diff/110018/components/password_man...
components/password_manager/core/common/password_manager_ui.h:52:
ASK_USER_TO_REPORT_URL_AFTER_NEXT_LOAD_STATE,
Neither this state, nor the one above seems to be mentioned in any of the tests.
Could we increase the coverage?

https://codereview.chromium.org/765073004/diff/110018/components/password_man...
components/password_manager/core/common/password_manager_ui.h:54: // Now we need
to ask users if thew want to report the URL by showing
typo: thew -> they

[melandory, 2014-12-05 12:25:04]

Patchset #4 (id:150001) has been deleted

[melandory, 2014-12-05 12:56:13]

Patchset #4 (id:170001) has been deleted

[melandory, 2014-12-05 12:56:53]

Hi Vaclav,

I've added unit tests and changes some variables names and comments accordingly.

Thanks.

https://codereview.chromium.org/765073004/diff/110018/chrome/browser/password...
File chrome/browser/password_manager/chrome_password_manager_client_unittest.cc
(right):

https://codereview.chromium.org/765073004/diff/110018/chrome/browser/password...
chrome/browser/password_manager/chrome_password_manager_client_unittest.cc:236:
// TODO(melandory) Since "Ask user to submit URL" functionality is currently
On 2014/12/04 13:25:32, vabr (Chromium) wrote:
> The TODO comment here seems like a copy&paste mistake. Here we should always
> EXPECT_FALSE, no?

Done.

https://codereview.chromium.org/765073004/diff/110018/chrome/browser/ui/passw...
File chrome/browser/ui/passwords/manage_passwords_bubble_model.h (right):

https://codereview.chromium.org/765073004/diff/110018/chrome/browser/ui/passw...
chrome/browser/ui/passwords/manage_passwords_bubble_model.h:43: // Call by the
view code when user agreed for URL collection.
On 2014/12/04 13:25:33, vabr (Chromium) wrote:
> grammar nit: agreed for -> agreed to

Done.

https://codereview.chromium.org/765073004/diff/110018/chrome/browser/ui/passw...
chrome/browser/ui/passwords/manage_passwords_bubble_model.h:122: // URL of a
page from where bubble was triggered.
On 2014/12/04 13:25:33, vabr (Chromium) wrote:
> grammar nits:
> a page -> the page
> bubble -> this bubble
> (Feel free to override my suggestions with the blessing of any native speaker,
> though.)

Done.

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

https://codereview.chromium.org/765073004/diff/110018/chrome/browser/ui/views...
chrome/browser/ui/views/passwords/manage_passwords_bubble_view.cc:774: 
On 2014/12/04 13:25:33, vabr (Chromium) wrote:
> nit: There are still 3 removed blank lines in this file (original locations
are
> lines 774, 777, 921). Please undo those changes.

Done.

https://codereview.chromium.org/765073004/diff/110018/components/password_man...
File components/password_manager/core/common/password_manager_ui.h (right):

https://codereview.chromium.org/765073004/diff/110018/components/password_man...
components/password_manager/core/common/password_manager_ui.h:46: // Password
manager failed to detect the form, but we don't need to pop up a
On 2014/12/04 13:25:33, vabr (Chromium) wrote:
> I'm rather confused about the names and meanings of the three new states. I'll
> come over to you and ask.

Done.

https://codereview.chromium.org/765073004/diff/110018/components/password_man...
components/password_manager/core/common/password_manager_ui.h:48:
ASK_USER_TO_REPORT_URL_STATE,
On 2014/12/04 13:25:33, vabr (Chromium) wrote:
> I find confusing that this is about asking the user, but at the same time we
> don't need to pop up a bubble. It looks like a relic from the previous naming
> scheme.
> 
> Is it at all possible to display this bubble manually, by clicking on the key
> icon?
> 
> If yes, then we should just rename this and change the code comment on lines
> 46-47 to something explaining that this is the state just before the bubble is
> opened automatically for the sake of asking the user about reporting the URL.
> 
> If not, then we should simply merge this state with the next one.

Done.

https://codereview.chromium.org/765073004/diff/110018/components/password_man...
components/password_manager/core/common/password_manager_ui.h:52:
ASK_USER_TO_REPORT_URL_AFTER_NEXT_LOAD_STATE,
On 2014/12/04 13:25:33, vabr (Chromium) wrote:
> Neither this state, nor the one above seems to be mentioned in any of the
tests.
> Could we increase the coverage?

Done.

https://codereview.chromium.org/765073004/diff/110018/components/password_man...
components/password_manager/core/common/password_manager_ui.h:54: // Now we need
to ask users if thew want to report the URL by showing
On 2014/12/04 13:25:33, vabr (Chromium) wrote:
> typo: thew -> they

Done.

[vabr (Chromium), 2014-12-05 13:43:22]

Thanks!

The tests and the CL look good to me, but I have one last confusion about the
state names (see below).

Happy to talk through this, if you find it quicker.

Cheers,
Vaclav

https://codereview.chromium.org/765073004/diff/190001/chrome/browser/ui/passw...
File chrome/browser/ui/passwords/manage_passwords_bubble_model_unittest.cc
(right):

https://codereview.chromium.org/765073004/diff/190001/chrome/browser/ui/passw...
chrome/browser/ui/passwords/manage_passwords_bubble_model_unittest.cc:48: //
"Ask to collect URL?" doesn't appear before navigation is implemented
nit: Full-stop missing at the end.

https://codereview.chromium.org/765073004/diff/190001/chrome/browser/ui/passw...
File chrome/browser/ui/passwords/manage_passwords_ui_controller.cc (right):

https://codereview.chromium.org/765073004/diff/190001/chrome/browser/ui/passw...
chrome/browser/ui/passwords/manage_passwords_ui_controller.cc:262: if (state_ ==
password_manager::ui::ASK_USER_REPORT_URL_AFTER_LOAD_STATE) {
Apparently, ASK_USER_REPORT_URL_AFTER_LOAD_STATE is the state _before_ the page
load. We should rename it (see my comment in password_manager_ui.h).

https://codereview.chromium.org/765073004/diff/190001/components/password_man...
File components/password_manager/core/common/password_manager_ui.h (right):

https://codereview.chromium.org/765073004/diff/190001/components/password_man...
components/password_manager/core/common/password_manager_ui.h:47:
ASK_USER_REPORT_URL_BEFORE_LOAD_STATE,
My suggestion might be a silly one, but I suggest rotating the state names
forward by one step. As they are now, the names do not seem to me to match the
meanings. My suggestion would be:

ASK_USER_REPORT_URL_AND_PENDING_BUBBLE_STATE -- the very first state. The bubble
is not shown yet, but should be (hence "pending bubble").

ASK_USER_REPORT_URL_BEFORE_LOAD_STATE -- the state when the bubble has already
been displayed, but the page navigation ("load") did not occur yet.

ASK_USER_REPORT_URL_AFTER_LOAD_STATE -- the state after the navigation ("load"),
when the bubble is either still displayed, or can be displayed by the user
clicking on the key icon.

[vabr (Chromium), 2014-12-05 13:53:42]

Thanks for coming over, I'm sending the suggestions we settled upon.

Cheers,
Vaclav

https://codereview.chromium.org/765073004/diff/190001/components/password_man...
File components/password_manager/core/common/password_manager_ui.h (right):

https://codereview.chromium.org/765073004/diff/190001/components/password_man...
components/password_manager/core/common/password_manager_ui.h:47:
ASK_USER_REPORT_URL_BEFORE_LOAD_STATE,
ASK_USER_REPORT_URL_STATE

https://codereview.chromium.org/765073004/diff/190001/components/password_man...
components/password_manager/core/common/password_manager_ui.h:51:
ASK_USER_REPORT_URL_AFTER_LOAD_STATE,
ASK_USER_REPORT_URL_BUBBLE_SHOWN_BEFORE_TRANSITION_STATE

https://codereview.chromium.org/765073004/diff/190001/components/password_man...
components/password_manager/core/common/password_manager_ui.h:54:
ASK_USER_TO_REPORT_URL_AND_PENDING_BUBBLE_STATE,
ASK_USER_REPORT_URL_BUBBLE_SHOWN_STATE

[vabr (Chromium), 2014-12-05 14:11:32]

I forgot to say: LGTM (with the state name changes and previous comments from
today incorporated).

Cheers,
Vaclav

[melandory, 2014-12-05 14:22:56]

https://codereview.chromium.org/765073004/diff/190001/chrome/browser/ui/passw...
File chrome/browser/ui/passwords/manage_passwords_bubble_model_unittest.cc
(right):

https://codereview.chromium.org/765073004/diff/190001/chrome/browser/ui/passw...
chrome/browser/ui/passwords/manage_passwords_bubble_model_unittest.cc:48: //
"Ask to collect URL?" doesn't appear before navigation is implemented
On 2014/12/05 13:43:22, vabr (Chromium) wrote:
> nit: Full-stop missing at the end.

Done.

https://codereview.chromium.org/765073004/diff/190001/chrome/browser/ui/passw...
File chrome/browser/ui/passwords/manage_passwords_ui_controller.cc (right):

https://codereview.chromium.org/765073004/diff/190001/chrome/browser/ui/passw...
chrome/browser/ui/passwords/manage_passwords_ui_controller.cc:262: if (state_ ==
password_manager::ui::ASK_USER_REPORT_URL_AFTER_LOAD_STATE) {
On 2014/12/05 13:43:22, vabr (Chromium) wrote:
> Apparently, ASK_USER_REPORT_URL_AFTER_LOAD_STATE is the state _before_ the
page
> load. We should rename it (see my comment in password_manager_ui.h).

Done.

[melandory, 2014-12-05 15:21:23]

The CQ bit was checked by melandory@chromium.org

[commit-bot: I haz the power, 2014-12-05 15:21:41]

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

[commit-bot: I haz the power, 2014-12-05 17:12:31]

Committed patchset #6 (id:230001)

[commit-bot: I haz the power, 2014-12-05 17:13:19]

Patchset 6 (id:??) landed as
https://crrev.com/09dd734520dc40ad87a45bc7a85d95c48a608904
Cr-Commit-Position: refs/heads/master@{#307036}