Add a learn more link to password generation bubble to point to a help page.

Add a learn more link to password generation bubble to point to a help page.


BUG=120798
TEST=not tested


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

[Garrett Casto, 2012-04-26 21:45:15]

You also need to fill out the bug and test sections of the cl. The bug number
for this issue is crbug.com/120798. For test, you should just say not tested if
you aren't running any tests (since there are none for this).

http://codereview.chromium.org/10226009/diff/1/chrome/browser/ui/gtk/password...
File chrome/browser/ui/gtk/password_generation_bubble_gtk.cc (right):

http://codereview.chromium.org/10226009/diff/1/chrome/browser/ui/gtk/password...
chrome/browser/ui/gtk/password_generation_bubble_gtk.cc:8: #include
"chrome/browser/platform_util.h"
Why did you add this?

http://codereview.chromium.org/10226009/diff/1/chrome/browser/ui/gtk/password...
chrome/browser/ui/gtk/password_generation_bubble_gtk.cc:45:
g_signal_connect(learn_more_link, "clicked",
nit: I would put all the callback registrations together.

http://codereview.chromium.org/10226009/diff/1/chrome/browser/ui/gtk/password...
chrome/browser/ui/gtk/password_generation_bubble_gtk.cc:95: Browser* browser =
Browser::Create(profile_);
Don't open a new window, open a new tab in the current window and switch to that
tab. Popup windows are annoying.

This might change with the above change, but you need to destroy the bubble,
either explicitly or by it losing focus. Currently if the new window happens to
overlap the bubble, you can see the bubble on top of the new window until you
click on something.

[zysxqn, 2012-04-26 23:46:24]

Please have another look. Thanks!

http://codereview.chromium.org/10226009/diff/1/chrome/browser/ui/gtk/password...
File chrome/browser/ui/gtk/password_generation_bubble_gtk.cc (right):

http://codereview.chromium.org/10226009/diff/1/chrome/browser/ui/gtk/password...
chrome/browser/ui/gtk/password_generation_bubble_gtk.cc:8: #include
"chrome/browser/platform_util.h"
On 2012/04/26 21:45:15, Garrett Casto wrote:
> Why did you add this?

Forgot to remove this after I decided not to use some functions in it. Removed.

http://codereview.chromium.org/10226009/diff/1/chrome/browser/ui/gtk/password...
chrome/browser/ui/gtk/password_generation_bubble_gtk.cc:45:
g_signal_connect(learn_more_link, "clicked",
On 2012/04/26 21:45:15, Garrett Casto wrote:
> nit: I would put all the callback registrations together.

Done.

http://codereview.chromium.org/10226009/diff/1/chrome/browser/ui/gtk/password...
chrome/browser/ui/gtk/password_generation_bubble_gtk.cc:95: Browser* browser =
Browser::Create(profile_);
On 2012/04/26 21:45:15, Garrett Casto wrote:
> Don't open a new window, open a new tab in the current window and switch to
that
> tab. Popup windows are annoying.
> 
> This might change with the above change, but you need to destroy the bubble,
> either explicitly or by it losing focus. Currently if the new window happens
to
> overlap the bubble, you can see the bubble on top of the new window until you
> click on something.

Changed to open a new tab in the same window. 

My question is whether we should close the bubble. I think a common use case
would be: user click password field -> bubble shows -> user click learn more ->
user decided to use auto password generation and close the new tab -> user
returns to the sign up page and expect to click "try it" but the bubble has gone
now. The user need to move the mouse elsewhere and back to the password field to
trigger "focus change" callback.

One solution is not to close the bubble, but like you said it looks a little
weird to see the bubble while reading the help page. 

Any ideas?

[Garrett Casto, 2012-04-27 06:25:25]

On 2012/04/26 23:46:24, zysxqn wrote:
> Please have another look. Thanks!
> 
>
http://codereview.chromium.org/10226009/diff/1/chrome/browser/ui/gtk/password...
> File chrome/browser/ui/gtk/password_generation_bubble_gtk.cc (right):
> 
>
http://codereview.chromium.org/10226009/diff/1/chrome/browser/ui/gtk/password...
> chrome/browser/ui/gtk/password_generation_bubble_gtk.cc:8: #include
> "chrome/browser/platform_util.h"
> On 2012/04/26 21:45:15, Garrett Casto wrote:
> > Why did you add this?
> 
> Forgot to remove this after I decided not to use some functions in it.
Removed.
> 
>
http://codereview.chromium.org/10226009/diff/1/chrome/browser/ui/gtk/password...
> chrome/browser/ui/gtk/password_generation_bubble_gtk.cc:45:
> g_signal_connect(learn_more_link, "clicked",
> On 2012/04/26 21:45:15, Garrett Casto wrote:
> > nit: I would put all the callback registrations together.
> 
> Done.
> 
>
http://codereview.chromium.org/10226009/diff/1/chrome/browser/ui/gtk/password...
> chrome/browser/ui/gtk/password_generation_bubble_gtk.cc:95: Browser* browser =
> Browser::Create(profile_);
> On 2012/04/26 21:45:15, Garrett Casto wrote:
> > Don't open a new window, open a new tab in the current window and switch to
> that
> > tab. Popup windows are annoying.
> > 
> > This might change with the above change, but you need to destroy the bubble,
> > either explicitly or by it losing focus. Currently if the new window happens
> to
> > overlap the bubble, you can see the bubble on top of the new window until
you
> > click on something.
> 
> Changed to open a new tab in the same window. 
> 
> My question is whether we should close the bubble. I think a common use case
> would be: user click password field -> bubble shows -> user click learn more
->
> user decided to use auto password generation and close the new tab -> user
> returns to the sign up page and expect to click "try it" but the bubble has
gone
> now. The user need to move the mouse elsewhere and back to the password field
to
> trigger "focus change" callback.
> 
> One solution is not to close the bubble, but like you said it looks a little
> weird to see the bubble while reading the help page. 
> 
> Any ideas?

This is part of a larger problem with the current UI. The bubbles that currently
exist in Chrome are meant to be transient. I think for our bubbles we want it to
be more like content, hide like it, move like it, etc. The find bar is the only
UI in chrome that I know of that does this, so I've meant to look into how it
accomplishes this to see how hard it would be to add.

[Garrett Casto, 2012-04-30 20:00:32]

LGTM

You need to get this approved by someone from brower/ui/gtk/OWNERS. I used
erg@chromium.org last time.

[zysxqn, 2012-05-01 17:34:31]

Elliot, could you take a look at of this CL and approve the changes? Thanks!

Yue

[Elliot Glaysher, 2012-05-01 17:37:08]

lgtm

[commit-bot: I haz the power, 2012-05-01 18:36:49]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/zysxqn@google.com/10226009/6001

[commit-bot: I haz the power, 2012-05-01 20:34:12]

Change committed as 134766