Observe visibility of password inputs, for HTTP-bad phase 1

Observe visibility of password inputs, for HTTP-bad phase 1

This CL is a step along the way to putting a warning in the omnibox when
an insecure HTTP page displays a visible password field. When a password
input field becomes visible, a Mojo IPC is sent to the browser process
and handled by the ContentPasswordManagerDriver. At the moment
ContentPasswordManagerDriver does nothing with this message, but in a
follow-up CL, it will notify the WebContents which will cause a warning
to appear in the omnibox when an HTTP page displays a visible password field.

Design doc: https://docs.google.com/document/d/1xno6g6OnA7strcyzE-o_drevW8L0Mb6ZBEkjsiwa6x0/edit#

BUG=647560
Committed: https://crrev.com/b3f84073ec1cb1222b32e7dc8d796d231a25efec
Cr-Commit-Position: refs/heads/master@{#424809}

[estark, 2016-09-27 23:02:16]

The CQ bit was checked by estark@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-09-27 23:02:36]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[estark, 2016-09-27 23:05:20]

estark@chromium.org changed reviewers:
+ esprehn@chromium.org, vabr@chromium.org

[estark, 2016-09-27 23:05:21]

esprehn, can you please review the Blink parts of this?
vabr, can you please review the components/autofill and
components/password_manager parts? Thanks!

[commit-bot: I haz the power, 2016-09-27 23:20:46]

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

[commit-bot: I haz the power, 2016-09-27 23:20:47]

Dry run: Try jobs failed on following builders:
  linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[estark, 2016-09-27 23:28:13]

The CQ bit was checked by estark@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-09-27 23:28:42]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[esprehn, 2016-09-27 23:30:20]

https://codereview.chromium.org/2378503002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/web/ChromeClientImpl.cpp (right):

https://codereview.chromium.org/2378503002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/web/ChromeClientImpl.cpp:1077:
webframe->autofillClient()->passwordFieldBecameVisible();
Please use mojo directly instead, I don't want to plumb out more callbacks.

[esprehn, 2016-09-27 23:32:36]

https://codereview.chromium.org/2378503002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/html/HTMLInputElement.cpp (right):

https://codereview.chromium.org/2378503002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/html/HTMLInputElement.cpp:1902:
m_passwordVisibilityObserver = new ElementVisibilityObserver(this,
WTF::bind(&HTMLInputElement::onVisibilityChangedForPasswordInput,
wrapWeakPersistent(this)));
Lets make this a member of the PasswordManagerType itself instead. I don't think
the input needs to know anything about this

[estark, 2016-09-27 23:35:42]

https://codereview.chromium.org/2378503002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/web/ChromeClientImpl.cpp (right):

https://codereview.chromium.org/2378503002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/web/ChromeClientImpl.cpp:1077:
webframe->autofillClient()->passwordFieldBecameVisible();
On 2016/09/27 23:30:19, esprehn wrote:
> Please use mojo directly instead, I don't want to plumb out more callbacks.

Can you point to an example of what you mean by "use mojo directly"? I'm a mojo
noob and am not sure what that means.

[estark, 2016-09-27 23:39:42]

https://codereview.chromium.org/2378503002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/web/ChromeClientImpl.cpp (right):

https://codereview.chromium.org/2378503002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/web/ChromeClientImpl.cpp:1077:
webframe->autofillClient()->passwordFieldBecameVisible();
On 2016/09/27 23:35:42, estark wrote:
> On 2016/09/27 23:30:19, esprehn wrote:
> > Please use mojo directly instead, I don't want to plumb out more callbacks.
> 
> Can you point to an example of what you mean by "use mojo directly"? I'm a
mojo
> noob and am not sure what that means.

Oh, actually, I just found and am reading
https://dev.chromium.org/developers/design-documents/mojo/calling-mojo-from-b...
and have a feeling that will answer my question. :)

[commit-bot: I haz the power, 2016-09-27 23:54:50]

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

[commit-bot: I haz the power, 2016-09-27 23:54:51]

Dry run: Try jobs failed on following builders:
  android_arm64_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_arm6...)

[estark, 2016-09-28 04:20:10]

The CQ bit was checked by estark@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-09-28 04:20:24]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-09-28 04:33:34]

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

[commit-bot: I haz the power, 2016-09-28 04:33:35]

Dry run: Try jobs failed on following builders:
  linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[estark, 2016-09-28 04:50:22]

The CQ bit was checked by estark@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-09-28 04:50:37]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[estark, 2016-09-28 04:54:09]

Ok, I've taken a stab at esprehn's comments, though I'm not sure that I did it
right (calling mojo directly from Blink).

vabr: I'm curious whether you think this belongs in components/password_manager.
It seems related to the password manager, but also sort of separate. I could
move it to a separate component or perhaps another component like
components/security_state if you prefer.

[commit-bot: I haz the power, 2016-09-28 05:32:47]

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

[commit-bot: I haz the power, 2016-09-28 05:32:48]

Dry run: Try jobs failed on following builders:
  linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[vabr (Chromium), 2016-09-28 08:49:47]

Hi estark@,

The changes in components/password_manager LGTM, assuming you fix the DEPS as
specified below. Placing this into the password_manager component seems
reasonable to me.

(I have not found any components/autofill changes in the CL.)

Cheers,
Vaclav

https://codereview.chromium.org/2378503002/diff/60001/components/password_man...
File components/password_manager/DEPS (right):

https://codereview.chromium.org/2378503002/diff/60001/components/password_man...
components/password_manager/DEPS:15:
"+third_party/WebKit/public/platform/modules/sensitive_input_visibility/sensitive_input_visibility_service.mojom.h",
This dependency cannot be added here, because components/password_manager is
used also on iOS, where WebKit is not used. Instead you probably want to add it
in components/password_manager/content.

[estark, 2016-09-30 21:45:38]

The CQ bit was checked by estark@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-09-30 21:46:07]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-09-30 22:38:14]

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

[commit-bot: I haz the power, 2016-09-30 22:38:15]

Dry run: Try jobs failed on following builders:
  linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[estark, 2016-10-06 02:47:51]

The CQ bit was checked by estark@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-10-06 02:48:16]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[estark, 2016-10-06 02:49:39]

esprehn, could you please take another look?

I seem to be doing something Mojo-y wrong, because I'm running afoul of this
DCHECK:
https://cs.chromium.org/chromium/src/mojo/public/cpp/bindings/lib/binding_sta...

(You'll be able to see that in the test failures on the bots.)

I haven't been able to figure out what I'm doing wrong so please let me know if
anything looks amiss to you.

https://codereview.chromium.org/2378503002/diff/60001/components/password_man...
File components/password_manager/DEPS (right):

https://codereview.chromium.org/2378503002/diff/60001/components/password_man...
components/password_manager/DEPS:15:
"+third_party/WebKit/public/platform/modules/sensitive_input_visibility/sensitive_input_visibility_service.mojom.h",
On 2016/09/28 08:49:47, vabr (Chromium) wrote:
> This dependency cannot be added here, because components/password_manager is
> used also on iOS, where WebKit is not used. Instead you probably want to add
it
> in components/password_manager/content.

Done.

I also ended up splitting this out into a separate PasswordVisibilityService,
because it looks to me (?) like the same implementation isn't meant to be used
for multiple Mojo interfaces.

[commit-bot: I haz the power, 2016-10-06 04:00:01]

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

[commit-bot: I haz the power, 2016-10-06 04:00:02]

Dry run: Try jobs failed on following builders:
  linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[dcheng, 2016-10-06 05:12:35]

dcheng@chromium.org changed reviewers:
+ dcheng@chromium.org

[dcheng, 2016-10-06 05:12:35]

It looks like the Binding in the browser-side impl is getting rebound.

Two possible solutions:
- Use a BindingSet, so the impl can support multiple connections.
- Cache the pointer on the Blink-side so we only connect once per
RenderFrameHost.

BindingSet is the easier solution for now. Down the road, I think we might want
to have better support for a per-frame model (I've seen a fair amount of Blink
code that lazily caches interfaces, and it'd be nice not to have to do that so
much).

[estark, 2016-10-06 13:04:46]

The CQ bit was checked by estark@chromium.org to run a CQ dry run

[estark, 2016-10-06 13:04:47]

On 2016/10/06 05:12:35, dcheng wrote:
> It looks like the Binding in the browser-side impl is getting rebound.
> 
> Two possible solutions:
> - Use a BindingSet, so the impl can support multiple connections.
> - Cache the pointer on the Blink-side so we only connect once per
> RenderFrameHost.
> 
> BindingSet is the easier solution for now. Down the road, I think we might
want
> to have better support for a per-frame model (I've seen a fair amount of Blink
> code that lazily caches interfaces, and it'd be nice not to have to do that so
> much).

Aha! Thanks! That seems to do the trick.

[estark, 2016-10-07 16:37:00]

The CQ bit was checked by estark@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-10-07 16:37:28]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-10-07 16:48:29]

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

[commit-bot: I haz the power, 2016-10-07 16:48:30]

Dry run: Try jobs failed on following builders:
  chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[estark, 2016-10-07 16:56:07]

Friendly ping to esprehn

[esprehn, 2016-10-07 21:56:38]

https://codereview.chromium.org/2378503002/diff/120001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/html/forms/PasswordInputType.cpp (right):

https://codereview.chromium.org/2378503002/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/html/forms/PasswordInputType.cpp:106:
mojom::blink::SensitiveInputVisibilityServicePtr sensitiveInputServicePtr;
should we be checking isSecureContext() here to avoid sending IPCs for all
secure pages?

https://codereview.chromium.org/2378503002/diff/120001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/html/forms/PasswordInputType.h (right):

https://codereview.chromium.org/2378503002/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/html/forms/PasswordInputType.h:45:
PasswordInputType(HTMLInputElement&);
explicit

https://codereview.chromium.org/2378503002/diff/120001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/html/forms/PasswordInputTypeTest.cpp
(right):

https://codereview.chromium.org/2378503002/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/html/forms/PasswordInputTypeTest.cpp:31:
std::move(handle))));
this fulfills every getInterface call with a SensitiveInputVisibilityService
instance?

[estark, 2016-10-07 23:24:09]

The CQ bit was checked by estark@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-10-07 23:24:40]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[estark, 2016-10-07 23:26:03]

For posterity, bots are red because of crbug.com/654082 and crbug.com/654079 --
thanks, Elliott, for investigating those with me.

https://codereview.chromium.org/2378503002/diff/120001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/html/forms/PasswordInputType.cpp (right):

https://codereview.chromium.org/2378503002/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/html/forms/PasswordInputType.cpp:106:
mojom::blink::SensitiveInputVisibilityServicePtr sensitiveInputServicePtr;
On 2016/10/07 21:56:38, esprehn wrote:
> should we be checking isSecureContext() here to avoid sending IPCs for all
> secure pages?

Good idea, thanks. I think it's sufficient to do this check at line 61 and avoid
starting the observer all-together for a secure context; I don't think there's
any way to go from a secure context to a non-secure context or vice versa.

https://codereview.chromium.org/2378503002/diff/120001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/html/forms/PasswordInputType.h (right):

https://codereview.chromium.org/2378503002/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/html/forms/PasswordInputType.h:45:
PasswordInputType(HTMLInputElement&);
On 2016/10/07 21:56:38, esprehn wrote:
> explicit

Done.

https://codereview.chromium.org/2378503002/diff/120001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/html/forms/PasswordInputTypeTest.cpp
(right):

https://codereview.chromium.org/2378503002/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/html/forms/PasswordInputTypeTest.cpp:31:
std::move(handle))));
On 2016/10/07 21:56:38, esprehn wrote:
> this fulfills every getInterface call with a SensitiveInputVisibilityService
> instance?

I based this on ScreenWakeLockTest.cpp. Is it better to only create the service
once and re-use it? Went ahead and did that.

[commit-bot: I haz the power, 2016-10-07 23:35:23]

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

[commit-bot: I haz the power, 2016-10-07 23:35:25]

Dry run: Try jobs failed on following builders:
  chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[estark, 2016-10-11 04:19:37]

The CQ bit was checked by estark@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-10-11 04:19:58]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-10-11 04:29:13]

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

[commit-bot: I haz the power, 2016-10-11 04:29:14]

Dry run: Try jobs failed on following builders:
  chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[estark, 2016-10-11 05:17:50]

The CQ bit was checked by estark@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-10-11 05:18:03]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-10-11 05:27:03]

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

[commit-bot: I haz the power, 2016-10-11 05:27:04]

Dry run: Try jobs failed on following builders:
  chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[estark, 2016-10-11 15:45:19]

As discussed off-thread, I changed this to just use createLayoutObject() 
instead of an IntersectionObserver. That more closely matches the UX we want.
PTAL, thanks!

[vabr (Chromium), 2016-10-11 16:52:45]

components/password_manager LGTM with the comments below.

I explicitly acknowledge that the new code is not tested: the service does not
have the non-trivial code added in this CL, so there is nothing to test. The
factory would ideally be tested, but there are two other instances of the same
duplicated code in Chromium already, and they are virtually untested as well.
Let's join the two pieces of technical debt (deduplication + adding tests) and
mention them in the crbug issue, if you end up creating one in response to my
comment below.

Thanks!
Vaclav

https://codereview.chromium.org/2378503002/diff/180001/components/password_ma...
File
components/password_manager/content/browser/password_visibility_service_factory.cc
(right):

https://codereview.chromium.org/2378503002/diff/180001/components/password_ma...
components/password_manager/content/browser/password_visibility_service_factory.cc:31:
base::WrapUnique(new PasswordVisibilityServiceFactory(web_contents));
Please use
base::MakeUnique<PasswordVisibilityServiceFactory>(web_contents);
instead.
(See the discussion around
https://groups.google.com/a/chromium.org/d/msg/chromium-dev/iQgMedVA8-k/RCcpL...
for more context.)

https://codereview.chromium.org/2378503002/diff/180001/components/password_ma...
components/password_manager/content/browser/password_visibility_service_factory.cc:71:
// totally
nit: Please re-indent the comment to use all the horizontal space available.

https://codereview.chromium.org/2378503002/diff/180001/components/password_ma...
File
components/password_manager/content/browser/password_visibility_service_factory.h
(right):

https://codereview.chromium.org/2378503002/diff/180001/components/password_ma...
components/password_manager/content/browser/password_visibility_service_factory.h:1:
// Copyright 2016 The Chromium Authors. All rights reserved.
Seeing the amount of code duplicated amongst this factory and
content_password_manager_driver_factory.* and
components/autofill/content/browser/content_autofill_driver_factory.*, I wish we
had a well-tested base class providing all the "retrieve service for a RFH"
logic.

Because this is a pre-existing problem (the two mentioned factories already
duplicate the code), I won't block this CL on it. But if you want, you can file
a bug about the code duplication and insert a TODO(crbug.com/XXX) pointing to it
here. If you do, feel free to Cc me and add any other candidates for sharing
this code you might know of. Thanks!

https://codereview.chromium.org/2378503002/diff/180001/components/password_ma...
components/password_manager/content/browser/password_visibility_service_factory.h:48:
PasswordVisibilityServiceFactory(content::WebContents* web_contents);
Should this constructor be "explicit"?

[esprehn, 2016-10-11 22:15:07]

https://codereview.chromium.org/2378503002/diff/180001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/html/forms/PasswordInputType.cpp (right):

https://codereview.chromium.org/2378503002/diff/180001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/html/forms/PasswordInputType.cpp:92: if
(!element().document().frame() || element().document().isSecureContext()) {
We can't get here if there's no frame, you don't need to null check the frame()

https://codereview.chromium.org/2378503002/diff/180001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/html/forms/PasswordInputType.cpp:100:
mojom::blink::SensitiveInputVisibilityServicePtr sensitiveInputServicePtr;
you need to null check layoutObject, it'll be null if the password input is not
displayed.

This function should be:

LayoutObject* layoutObject = 
    TextFieldInputType::createLayoutObject(style);
Document& document = element().document();
if (document.isSecureContext() || !layoutObject)
  return layoutObject;

// send ipc here.
return layoutObject;

https://codereview.chromium.org/2378503002/diff/180001/third_party/WebKit/pub...
File
third_party/WebKit/public/platform/modules/sensitive_input_visibility/sensitive_input_visibility_service.mojom
(right):

https://codereview.chromium.org/2378503002/diff/180001/third_party/WebKit/pub...
third_party/WebKit/public/platform/modules/sensitive_input_visibility/sensitive_input_visibility_service.mojom:9:
PasswordFieldVisible();
Since we only do this for insecure contexts, I do wonder if this should have the
word Insecure somewhere. This isn't just sensitive stuff being displayed.

[estark, 2016-10-11 22:41:17]

The CQ bit was checked by estark@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-10-11 22:42:01]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[estark, 2016-10-11 22:42:41]

Patchset #11 (id:200001) has been deleted

[commit-bot: I haz the power, 2016-10-11 22:45:21]

Dry run: Try jobs failed on following builders:
  android_arm64_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_arm6...)
  android_clang_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_clan...)
  chromeos_amd64-generic_chromium_compile_only_ng on
master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_amd64-...)
  chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[commit-bot: I haz the power, 2016-10-11 22:55:10]

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

[commit-bot: I haz the power, 2016-10-11 22:55:13]

Dry run: Try jobs failed on following builders:
  chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[estark, 2016-10-11 23:39:27]

The CQ bit was checked by estark@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-10-11 23:39:46]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[estark, 2016-10-11 23:45:14]

https://codereview.chromium.org/2378503002/diff/180001/components/password_ma...
File
components/password_manager/content/browser/password_visibility_service_factory.cc
(right):

https://codereview.chromium.org/2378503002/diff/180001/components/password_ma...
components/password_manager/content/browser/password_visibility_service_factory.cc:31:
base::WrapUnique(new PasswordVisibilityServiceFactory(web_contents));
On 2016/10/11 16:52:45, vabr (Chromium) wrote:
> Please use
> base::MakeUnique<PasswordVisibilityServiceFactory>(web_contents);
> instead.
> (See the discussion around
>
https://groups.google.com/a/chromium.org/d/msg/chromium-dev/iQgMedVA8-k/RCcpL...
> for more context.)

No longer applicable, but MakeUnique doesn't work here because the constructor
is private (see
https://groups.google.com/a/chromium.org/d/msg/chromium-dev/iQgMedVA8-k/MWAZy...
in that thread).

https://codereview.chromium.org/2378503002/diff/180001/components/password_ma...
components/password_manager/content/browser/password_visibility_service_factory.cc:71:
// totally
On 2016/10/11 16:52:45, vabr (Chromium) wrote:
> nit: Please re-indent the comment to use all the horizontal space available.

no longer applicable

https://codereview.chromium.org/2378503002/diff/180001/components/password_ma...
File
components/password_manager/content/browser/password_visibility_service_factory.h
(right):

https://codereview.chromium.org/2378503002/diff/180001/components/password_ma...
components/password_manager/content/browser/password_visibility_service_factory.h:1:
// Copyright 2016 The Chromium Authors. All rights reserved.
On 2016/10/11 16:52:45, vabr (Chromium) wrote:
> Seeing the amount of code duplicated amongst this factory and
> content_password_manager_driver_factory.* and
> components/autofill/content/browser/content_autofill_driver_factory.*, I wish
we
> had a well-tested base class providing all the "retrieve service for a RFH"
> logic.
> 
> Because this is a pre-existing problem (the two mentioned factories already
> duplicate the code), I won't block this CL on it. But if you want, you can
file
> a bug about the code duplication and insert a TODO(crbug.com/XXX) pointing to
it
> here. If you do, feel free to Cc me and add any other candidates for sharing
> this code you might know of. Thanks!

I did file a bug for this (https://crbug.com/654902), but then I realized that I
was confused when I split this out into a separate service+factory in patch set
5. Instead of having this whole separate PasswordVisibilityService, we can just
a have a BindingSet member of ContentPasswordManagerDriver to bind to requests
for the new interface. This means I don't have to duplicate all this code. So
patch set 11 is much more like patch set 4 (which you previously reviewed) where
it handles these requests on ContentPasswordManagerDriver instead of introducing
a new service implementation + factory for it.

https://codereview.chromium.org/2378503002/diff/180001/components/password_ma...
components/password_manager/content/browser/password_visibility_service_factory.h:48:
PasswordVisibilityServiceFactory(content::WebContents* web_contents);
On 2016/10/11 16:52:45, vabr (Chromium) wrote:
> Should this constructor be "explicit"?

no longer applicable

https://codereview.chromium.org/2378503002/diff/180001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/html/forms/PasswordInputType.cpp (right):

https://codereview.chromium.org/2378503002/diff/180001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/html/forms/PasswordInputType.cpp:92: if
(!element().document().frame() || element().document().isSecureContext()) {
On 2016/10/11 22:15:07, esprehn wrote:
> We can't get here if there's no frame, you don't need to null check the
frame()

Done.

https://codereview.chromium.org/2378503002/diff/180001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/html/forms/PasswordInputType.cpp:100:
mojom::blink::SensitiveInputVisibilityServicePtr sensitiveInputServicePtr;
On 2016/10/11 22:15:07, esprehn wrote:
> you need to null check layoutObject, it'll be null if the password input is
not
> displayed.
> 
> This function should be:
> 
> LayoutObject* layoutObject = 
>     TextFieldInputType::createLayoutObject(style);
> Document& document = element().document();
> if (document.isSecureContext() || !layoutObject)
>   return layoutObject;
> 
> // send ipc here.
> return layoutObject;

Done.

https://codereview.chromium.org/2378503002/diff/180001/third_party/WebKit/pub...
File
third_party/WebKit/public/platform/modules/sensitive_input_visibility/sensitive_input_visibility_service.mojom
(right):

https://codereview.chromium.org/2378503002/diff/180001/third_party/WebKit/pub...
third_party/WebKit/public/platform/modules/sensitive_input_visibility/sensitive_input_visibility_service.mojom:9:
PasswordFieldVisible();
On 2016/10/11 22:15:07, esprehn wrote:
> Since we only do this for insecure contexts, I do wonder if this should have
the
> word Insecure somewhere. This isn't just sensitive stuff being displayed.

Done.

[esprehn, 2016-10-11 23:49:13]

lgtm, I think we usually would say "InsecureContext" instead of
"NonsecureContext" like you did here though.

[commit-bot: I haz the power, 2016-10-11 23:49:55]

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

[commit-bot: I haz the power, 2016-10-11 23:49:56]

Dry run: Try jobs failed on following builders:
  chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[estark, 2016-10-11 23:50:42]

On 2016/10/11 23:49:13, esprehn wrote:
> lgtm, I think we usually would say "InsecureContext" instead of
> "NonsecureContext" like you did here though.

We are woefully inconsistent about that throughout Chrome, but I do see more
instances of insecure than nonsecure in Blink so I'll go with that.

[estark, 2016-10-11 23:55:10]

The CQ bit was checked by estark@chromium.org to run a CQ dry run

[estark, 2016-10-11 23:55:27]

estark@chromium.org changed reviewers:
+ meacer@chromium.org

[estark, 2016-10-11 23:55:28]

+meacer for IPC review of
third_party/WebKit/public/platform/modules/sensitive_input_visibility/sensitive_input_visibility_service.mojom,
please

[commit-bot: I haz the power, 2016-10-11 23:55:36]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[estark, 2016-10-11 23:56:13]

Description was changed from

==========
Observe visibility of password inputs, for HTTP-bad phase 1

This CL is a step along the way to putting a warning in the omnibox when
an insecure HTTP page displays a visible password field. It adds an
ElementVisibilityObserver to password input fields. When a password
input field becomes visible, a Mojo IPC is sent to the browser process
and handled by the ContentPasswordManagerDriver. At the moment
ContentPasswordManagerDriver does nothing with this message, but in a
follow-up CL (https://codereview.chromium.org/2362523003/), it will
notify the WebContents which will cause a warning to appear in the
omnibox when an HTTP page displays a visible password field.

Design doc:
https://docs.google.com/document/d/1xno6g6OnA7strcyzE-o_drevW8L0Mb6ZBEkjsiwa6...

BUG=647560
==========

to

==========
Observe visibility of password inputs, for HTTP-bad phase 1

This CL is a step along the way to putting a warning in the omnibox when
an insecure HTTP page displays a visible password field. When a password
input field becomes visible, a Mojo IPC is sent to the browser process
and handled by the ContentPasswordManagerDriver. At the moment
ContentPasswordManagerDriver does nothing with this message, but in a
follow-up CL, it will notify the WebContents which will cause a warning
to appear in the omnibox when an HTTP page displays a visible password field.

Design doc:
https://docs.google.com/document/d/1xno6g6OnA7strcyzE-o_drevW8L0Mb6ZBEkjsiwa6...

BUG=647560
==========

[meacer, 2016-10-12 00:00:30]

On 2016/10/11 23:55:28, estark wrote:
> +meacer for IPC review of
>
third_party/WebKit/public/platform/modules/sensitive_input_visibility/sensitive_input_visibility_service.mojom,
> please

mojom LGTM.

[commit-bot: I haz the power, 2016-10-12 00:02:31]

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

[commit-bot: I haz the power, 2016-10-12 00:02:32]

Dry run: Try jobs failed on following builders:
  chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
  ios-device on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...)
  ios-simulator on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)

[vabr (Chromium), 2016-10-12 09:45:22]

(Still LGTM. No comments, just two Acks.)

https://codereview.chromium.org/2378503002/diff/180001/components/password_ma...
File
components/password_manager/content/browser/password_visibility_service_factory.cc
(right):

https://codereview.chromium.org/2378503002/diff/180001/components/password_ma...
components/password_manager/content/browser/password_visibility_service_factory.cc:31:
base::WrapUnique(new PasswordVisibilityServiceFactory(web_contents));
On 2016/10/11 23:45:14, estark wrote:
> On 2016/10/11 16:52:45, vabr (Chromium) wrote:
> > Please use
> > base::MakeUnique<PasswordVisibilityServiceFactory>(web_contents);
> > instead.
> > (See the discussion around
> >
>
https://groups.google.com/a/chromium.org/d/msg/chromium-dev/iQgMedVA8-k/RCcpL...
> > for more context.)
> 
> No longer applicable, but MakeUnique doesn't work here because the constructor
> is private (see
>
https://groups.google.com/a/chromium.org/d/msg/chromium-dev/iQgMedVA8-k/MWAZy...
> in that thread).

Good point, you are correct. Thanks for mentioning it, I keep forgetting about
that use-case.

https://codereview.chromium.org/2378503002/diff/180001/components/password_ma...
File
components/password_manager/content/browser/password_visibility_service_factory.h
(right):

https://codereview.chromium.org/2378503002/diff/180001/components/password_ma...
components/password_manager/content/browser/password_visibility_service_factory.h:1:
// Copyright 2016 The Chromium Authors. All rights reserved.
On 2016/10/11 23:45:14, estark wrote:
> On 2016/10/11 16:52:45, vabr (Chromium) wrote:
> > Seeing the amount of code duplicated amongst this factory and
> > content_password_manager_driver_factory.* and
> > components/autofill/content/browser/content_autofill_driver_factory.*, I
wish
> we
> > had a well-tested base class providing all the "retrieve service for a RFH"
> > logic.
> > 
> > Because this is a pre-existing problem (the two mentioned factories already
> > duplicate the code), I won't block this CL on it. But if you want, you can
> file
> > a bug about the code duplication and insert a TODO(crbug.com/XXX) pointing
to
> it
> > here. If you do, feel free to Cc me and add any other candidates for sharing
> > this code you might know of. Thanks!
> 
> I did file a bug for this (https://crbug.com/654902), but then I realized that
I
> was confused when I split this out into a separate service+factory in patch
set
> 5. Instead of having this whole separate PasswordVisibilityService, we can
just
> a have a BindingSet member of ContentPasswordManagerDriver to bind to requests
> for the new interface. This means I don't have to duplicate all this code. So
> patch set 11 is much more like patch set 4 (which you previously reviewed)
where
> it handles these requests on ContentPasswordManagerDriver instead of
introducing
> a new service implementation + factory for it.

Thanks, the current version looks simpler and therefore better to me!

Also, thanks for the bug. And to be clear -- I did not mean to ask you to fix
the duplication, just to note it in the bugtracker.

[estark, 2016-10-12 15:53:44]

https://codereview.chromium.org/2378503002/diff/180001/components/password_ma...
File
components/password_manager/content/browser/password_visibility_service_factory.h
(right):

https://codereview.chromium.org/2378503002/diff/180001/components/password_ma...
components/password_manager/content/browser/password_visibility_service_factory.h:1:
// Copyright 2016 The Chromium Authors. All rights reserved.
On 2016/10/12 09:45:22, vabr (Chromium) wrote:
> On 2016/10/11 23:45:14, estark wrote:
> > On 2016/10/11 16:52:45, vabr (Chromium) wrote:
> > > Seeing the amount of code duplicated amongst this factory and
> > > content_password_manager_driver_factory.* and
> > > components/autofill/content/browser/content_autofill_driver_factory.*, I
> wish
> > we
> > > had a well-tested base class providing all the "retrieve service for a
RFH"
> > > logic.
> > > 
> > > Because this is a pre-existing problem (the two mentioned factories
already
> > > duplicate the code), I won't block this CL on it. But if you want, you can
> > file
> > > a bug about the code duplication and insert a TODO(crbug.com/XXX) pointing
> to
> > it
> > > here. If you do, feel free to Cc me and add any other candidates for
sharing
> > > this code you might know of. Thanks!
> > 
> > I did file a bug for this (https://crbug.com/654902), but then I realized
that
> I
> > was confused when I split this out into a separate service+factory in patch
> set
> > 5. Instead of having this whole separate PasswordVisibilityService, we can
> just
> > a have a BindingSet member of ContentPasswordManagerDriver to bind to
requests
> > for the new interface. This means I don't have to duplicate all this code.
So
> > patch set 11 is much more like patch set 4 (which you previously reviewed)
> where
> > it handles these requests on ContentPasswordManagerDriver instead of
> introducing
> > a new service implementation + factory for it.
> 
> Thanks, the current version looks simpler and therefore better to me!
> 
> Also, thanks for the bug. And to be clear -- I did not mean to ask you to fix
> the duplication, just to note it in the bugtracker.

Got it, thanks for clarifying (and for the code review!). I felt bad adding on
to the tech debt without trying to help, but now that this CL is simplified, I
feel less guilty and will probably mark it as Available. :)

[estark, 2016-10-12 15:57:19]

The CQ bit was checked by estark@chromium.org

[estark, 2016-10-12 15:57:20]

The patchset sent to the CQ was uploaded after l-g-t-m from
esprehn@chromium.org, vabr@chromium.org, meacer@chromium.org
Link to the patchset: https://codereview.chromium.org/2378503002/#ps260001
(title: "rebase")

[commit-bot: I haz the power, 2016-10-12 15:57:44]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-10-12 19:03:48]

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

[commit-bot: I haz the power, 2016-10-12 19:03:50]

Failed to apply the patch.
On branch working_branch
Your branch is up-to-date with 'origin/refs/pending/heads/master'.
nothing to commit, working tree clean

[commit-bot: I haz the power, 2016-10-12 19:05:15]

Description was changed from

==========
Observe visibility of password inputs, for HTTP-bad phase 1

This CL is a step along the way to putting a warning in the omnibox when
an insecure HTTP page displays a visible password field. When a password
input field becomes visible, a Mojo IPC is sent to the browser process
and handled by the ContentPasswordManagerDriver. At the moment
ContentPasswordManagerDriver does nothing with this message, but in a
follow-up CL, it will notify the WebContents which will cause a warning
to appear in the omnibox when an HTTP page displays a visible password field.

Design doc:
https://docs.google.com/document/d/1xno6g6OnA7strcyzE-o_drevW8L0Mb6ZBEkjsiwa6...

BUG=647560
==========

to

==========
Observe visibility of password inputs, for HTTP-bad phase 1

This CL is a step along the way to putting a warning in the omnibox when
an insecure HTTP page displays a visible password field. When a password
input field becomes visible, a Mojo IPC is sent to the browser process
and handled by the ContentPasswordManagerDriver. At the moment
ContentPasswordManagerDriver does nothing with this message, but in a
follow-up CL, it will notify the WebContents which will cause a warning
to appear in the omnibox when an HTTP page displays a visible password field.

Design doc:
https://docs.google.com/document/d/1xno6g6OnA7strcyzE-o_drevW8L0Mb6ZBEkjsiwa6...

BUG=647560

Committed: https://crrev.com/b3f84073ec1cb1222b32e7dc8d796d231a25efec
Cr-Commit-Position: refs/heads/master@{#424809}
==========

[commit-bot: I haz the power, 2016-10-12 19:05:17]

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