Use shadow DOM API to show icon for password generation.

Use shadow DOM API to show icon for password generation.

The key icon in this CL is rough draft and will be improved before launch.

BUG=114092
TEST=Ran PasswordGenerationManagerTest

[Garrett Casto, 2012-05-31 22:35:25]

+tony for webkit/ changes

This change seems to fail on the trybots because it can't find the new image. Is
this just an issue because they aren't doing 'gclient runhooks', or is there
something else going on here? This compiles fine on my local machine.

[tony, 2012-05-31 22:40:40]

webkit LGTM.

The try bots don't know how to handle binary files in patches. You have 2
options:
1) Land manually.
2) Commit the png file first (you can TBR it), then send the rest of your patch
to the try bots.

On 2012/05/31 22:35:25, Garrett Casto wrote:
> +tony for webkit/ changes
> 
> This change seems to fail on the trybots because it can't find the new image.
Is
> this just an issue because they aren't doing 'gclient runhooks', or is there
> something else going on here? This compiles fine on my local machine.

[Ilya Sherman, 2012-06-01 00:34:30]

LGTM

Out of curiosity, what happens if the password field already has an icon in the
space where this decoration would normally go?  Does one overlap the other?

http://codereview.chromium.org/10449099/diff/1/chrome/renderer/autofill/passw...
File chrome/renderer/autofill/password_generation_manager.cc (right):

http://codereview.chromium.org/10449099/diff/1/chrome/renderer/autofill/passw...
chrome/renderer/autofill/password_generation_manager.cc:18: #include
"third_party/WebKit/Source/WebKit/chromium/public/platform/WebVector.h"
nit: All of the platform/ #inlcudes should precede the Web* ones.

http://codereview.chromium.org/10449099/diff/1/chrome/renderer/autofill/passw...
chrome/renderer/autofill/password_generation_manager.cc:37: if
(!ShouldAnalyzeDocument(frame->document()))
nit: Is there any chance that the frame is null?

http://codereview.chromium.org/10449099/diff/1/chrome/renderer/autofill/passw...
chrome/renderer/autofill/password_generation_manager.cc:89: }
Am I understanding correctly that this will add a decoration element to *all*
password fields on the page; but we rely on the code on lines 65-67 to hide/show
it appropriately?  That seems a little less ideal than just adding the icon to
the field we think is relevant...

http://codereview.chromium.org/10449099/diff/1/chrome/renderer/autofill/passw...
File chrome/renderer/autofill/password_generation_manager_browsertest.cc
(right):

http://codereview.chromium.org/10449099/diff/1/chrome/renderer/autofill/passw...
chrome/renderer/autofill/password_generation_manager_browsertest.cc:105:
generation_manager_.get()).hasNonEmptyBoundingBox());
nit: This condition is somewhat lengthy -- perhaps worth decomposing a wrapper
method for it?

http://codereview.chromium.org/10449099/diff/1/chrome/renderer/autofill/passw...
chrome/renderer/autofill/password_generation_manager_browsertest.cc:118:
generation_manager_.get()).hasNonEmptyBoundingBox());
nit: Hmm, shouldn't there be an EXPECT_TRUE with this same condition somewhere
further along in the test?

[Garrett Casto, 2012-06-01 01:21:27]

http://codereview.chromium.org/10449099/diff/1/chrome/renderer/autofill/passw...
File chrome/renderer/autofill/password_generation_manager.cc (right):

http://codereview.chromium.org/10449099/diff/1/chrome/renderer/autofill/passw...
chrome/renderer/autofill/password_generation_manager.cc:18: #include
"third_party/WebKit/Source/WebKit/chromium/public/platform/WebVector.h"
On 2012/06/01 00:34:31, Ilya Sherman wrote:
> nit: All of the platform/ #inlcudes should precede the Web* ones.

Done.

http://codereview.chromium.org/10449099/diff/1/chrome/renderer/autofill/passw...
chrome/renderer/autofill/password_generation_manager.cc:37: if
(!ShouldAnalyzeDocument(frame->document()))
On 2012/06/01 00:34:31, Ilya Sherman wrote:
> nit: Is there any chance that the frame is null?

It doesn't look like other implementations of this check to see if frame or
document are NULL.

http://codereview.chromium.org/10449099/diff/1/chrome/renderer/autofill/passw...
chrome/renderer/autofill/password_generation_manager.cc:89: }
On 2012/06/01 00:34:31, Ilya Sherman wrote:
> Am I understanding correctly that this will add a decoration element to *all*
> password fields on the page; but we rely on the code on lines 65-67 to
hide/show
> it appropriately?  That seems a little less ideal than just adding the icon to
> the field we think is relevant...

So the first iteration of the API was something like this, but this has some
issues. In particular at the time we are attaching this decorator we don't have
enough information to determine if we want it to be visible (the whole form
hasn't been parsed for instance). Also, eventually we are going to have to wait
on the browser to determine if we want to show this icon, as we need to make
sure that the user hasn't opted out of saving passwords for this domain. Given
these constraints, this is what the WebKit guys thought that we should do. I
agree that it's not necessarily the conceptually cleanest way of doing things,
but I'm not sure if there are other acceptable options.

http://codereview.chromium.org/10449099/diff/1/chrome/renderer/autofill/passw...
File chrome/renderer/autofill/password_generation_manager_browsertest.cc
(right):

http://codereview.chromium.org/10449099/diff/1/chrome/renderer/autofill/passw...
chrome/renderer/autofill/password_generation_manager_browsertest.cc:105:
generation_manager_.get()).hasNonEmptyBoundingBox());
On 2012/06/01 00:34:31, Ilya Sherman wrote:
> nit: This condition is somewhat lengthy -- perhaps worth decomposing a wrapper
> method for it?

Done.

http://codereview.chromium.org/10449099/diff/1/chrome/renderer/autofill/passw...
chrome/renderer/autofill/password_generation_manager_browsertest.cc:118:
generation_manager_.get()).hasNonEmptyBoundingBox());
On 2012/06/01 00:34:31, Ilya Sherman wrote:
> nit: Hmm, shouldn't there be an EXPECT_TRUE with this same condition somewhere
> further along in the test?

Yep, fixed.

[Ilya Sherman, 2012-06-01 02:57:15]

(Still LGTM)

http://codereview.chromium.org/10449099/diff/1/chrome/renderer/autofill/passw...
File chrome/renderer/autofill/password_generation_manager.cc (right):

http://codereview.chromium.org/10449099/diff/1/chrome/renderer/autofill/passw...
chrome/renderer/autofill/password_generation_manager.cc:89: }
On 2012/06/01 01:21:28, Garrett Casto wrote:
> On 2012/06/01 00:34:31, Ilya Sherman wrote:
> > Am I understanding correctly that this will add a decoration element to
*all*
> > password fields on the page; but we rely on the code on lines 65-67 to
> hide/show
> > it appropriately?  That seems a little less ideal than just adding the icon
to
> > the field we think is relevant...
> 
> So the first iteration of the API was something like this, but this has some
> issues. In particular at the time we are attaching this decorator we don't
have
> enough information to determine if we want it to be visible (the whole form
> hasn't been parsed for instance). Also, eventually we are going to have to
wait
> on the browser to determine if we want to show this icon, as we need to make
> sure that the user hasn't opted out of saving passwords for this domain. Given
> these constraints, this is what the WebKit guys thought that we should do. I
> agree that it's not necessarily the conceptually cleanest way of doing things,
> but I'm not sure if there are other acceptable options.

Ok.  I still don't fully understand why we need to attach the decorator early
and modulate its visibility later, rather than simply attaching it later... but
as long as you've considered this approach and explicitly decided against it,
I'll trust your judgement.

http://codereview.chromium.org/10449099/diff/12001/chrome/renderer/autofill/p...
File chrome/renderer/autofill/password_generation_manager_browsertest.cc
(right):

http://codereview.chromium.org/10449099/diff/12001/chrome/renderer/autofill/p...
chrome/renderer/autofill/password_generation_manager_browsertest.cc:68: void
SimulateClickOnDecoration(WebKit::WebInputElement& input_element) {
nit: Please pass either by const-reference or as a pointer.

http://codereview.chromium.org/10449099/diff/12001/chrome/renderer/autofill/p...
chrome/renderer/autofill/password_generation_manager_browsertest.cc:74: bool
DecorationIsVisible(WebKit::WebInputElement& input_element) {
nit: Please pass either by const-reference or as a pointer.

[tony, 2012-06-01 16:32:55]

http://codereview.chromium.org/10449099/diff/1/chrome/renderer/autofill/passw...
File chrome/renderer/autofill/password_generation_manager.cc (right):

http://codereview.chromium.org/10449099/diff/1/chrome/renderer/autofill/passw...
chrome/renderer/autofill/password_generation_manager.cc:18: #include
"third_party/WebKit/Source/WebKit/chromium/public/platform/WebVector.h"
On 2012/06/01 00:34:31, Ilya Sherman wrote:
> nit: All of the platform/ #inlcudes should precede the Web* ones.

FWIW, I thought we did 'LANG=C sort' for headers.  That would put 'Web' before
'platform'.

[Ilya Sherman, 2012-06-01 19:56:11]

http://codereview.chromium.org/10449099/diff/1/chrome/renderer/autofill/passw...
File chrome/renderer/autofill/password_generation_manager.cc (right):

http://codereview.chromium.org/10449099/diff/1/chrome/renderer/autofill/passw...
chrome/renderer/autofill/password_generation_manager.cc:18: #include
"third_party/WebKit/Source/WebKit/chromium/public/platform/WebVector.h"
On 2012/06/01 16:32:55, tony wrote:
> On 2012/06/01 00:34:31, Ilya Sherman wrote:
> > nit: All of the platform/ #inlcudes should precede the Web* ones.
> 
> FWIW, I thought we did 'LANG=C sort' for headers.  That would put 'Web' before
> 'platform'.

Oh, you're right, I forgot that case differences matter.  Apologies for the
misdirection, Garrett.