Record Autofill form events specially for nonsecure pages

Record Autofill form events specially for nonsecure pages

The Form-Not-Secure experiment shows special "Payment not secure" warnings for
credit card forms on HTTP pages. This CL records credit card autofill form
events in a special histogram broken out for nonsecure pages, to measure the
impact of these "Payment not secure" warnings on form interactions.

To do so, the FormEventLogger needs to know whether the main frame URL is
secure, at a point at which it doesn't have access to a form origin URL. iOS is
the only platform on which IsContextSecure used the form_origin parameter, and
there was an existing TODO to fix that (https://crbug.com/505388). So this CL
removes the form_origin parameter from IsContextSecure, fixes the iOS TODO, and
uses the resulting IsContextSecure method to record nonsecure form metrics.

BUG=677291, 687823, 505388
Review-Url: https://codereview.chromium.org/2672623005
Cr-Commit-Position: refs/heads/master@{#449402}
Committed: https://chromium.googlesource.com/chromium/src/+/57464d6c82bea1c759874da3101135e1ae1494eb

[estark, 2017-02-03 02:25:34]

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

[commit-bot: I haz the power, 2017-02-03 02:26:21]

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

[estark, 2017-02-03 02:28:51]

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

[commit-bot: I haz the power, 2017-02-03 02:29:51]

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, 2017-02-03 02:36:57]

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

[commit-bot: I haz the power, 2017-02-03 02:36:58]

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

[estark, 2017-02-03 06:48:24]

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

[commit-bot: I haz the power, 2017-02-03 06:48:36]

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, 2017-02-03 06:50:21]

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

[commit-bot: I haz the power, 2017-02-03 06:50:22]

Dry run: Try jobs failed on following builders:
  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...)
  ios-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xco...)

[estark, 2017-02-03 07:26:22]

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

[commit-bot: I haz the power, 2017-02-03 07:26:30]

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, 2017-02-03 08:29:50]

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

[commit-bot: I haz the power, 2017-02-03 08:29:51]

Dry run: This issue passed the CQ dry run.

[estark, 2017-02-03 16:57:40]

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

[estark, 2017-02-03 16:57:41]

sebsg, can you please take a look? Thanks.

[sebsg, 2017-02-03 19:28:34]

Nice work!

https://codereview.chromium.org/2672623005/diff/60001/components/autofill/cor...
File components/autofill/core/browser/autofill_manager.cc (right):

https://codereview.chromium.org/2672623005/diff/60001/components/autofill/cor...
components/autofill/core/browser/autofill_manager.cc:1485:
credit_card_form_event_logger_->set_is_main_url_secure(
Can you move this with the other credit_card_form_event_logger calls at lines
1465-1468?

https://codereview.chromium.org/2672623005/diff/60001/components/autofill/cor...
File components/autofill/core/browser/autofill_metrics_unittest.cc (right):

https://codereview.chromium.org/2672623005/diff/60001/components/autofill/cor...
components/autofill/core/browser/autofill_metrics_unittest.cc:4273:
AutofillMetrics::FORM_EVENT_NO_SUGGESTION_SUBMITTED_ONCE, 1);
Can you also make sure that the normal histograms are logged correctly?

https://codereview.chromium.org/2672623005/diff/60001/components/autofill/cor...
components/autofill/core/browser/autofill_metrics_unittest.cc:4326:
AutofillMetrics::FORM_EVENT_NO_SUGGESTION_SUBMITTED_ONCE, 1);
Can you add a check to make sure that nothing was logged for your new histogram?
Like 
histograms.ExpectBucketCount("Autofill.FormEvents.CreditCard.OnNonsecurePage",
AutofillMetrics::FORM_EVENT_NO_SUGGESTION_SUBMITTED_ONCE, 0);

[estark, 2017-02-04 02:15:19]

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

[commit-bot: I haz the power, 2017-02-04 02:15:40]

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

[estark, 2017-02-04 02:16:12]

estark@chromium.org changed reviewers:
+ estade@chromium.org, jdonnelly@chromium.org, sgurun@chromium.org

[estark, 2017-02-04 02:16:13]

+estade for chrome/browser/ui/autofill
+jdonnelly for ios
+sgurun for android_webview
Thanks!

https://codereview.chromium.org/2672623005/diff/60001/components/autofill/cor...
File components/autofill/core/browser/autofill_manager.cc (right):

https://codereview.chromium.org/2672623005/diff/60001/components/autofill/cor...
components/autofill/core/browser/autofill_manager.cc:1485:
credit_card_form_event_logger_->set_is_main_url_secure(
On 2017/02/03 19:28:34, sebsg wrote:
> Can you move this with the other credit_card_form_event_logger calls at lines
> 1465-1468? 

Done.

https://codereview.chromium.org/2672623005/diff/60001/components/autofill/cor...
File components/autofill/core/browser/autofill_metrics_unittest.cc (right):

https://codereview.chromium.org/2672623005/diff/60001/components/autofill/cor...
components/autofill/core/browser/autofill_metrics_unittest.cc:4273:
AutofillMetrics::FORM_EVENT_NO_SUGGESTION_SUBMITTED_ONCE, 1);
On 2017/02/03 19:28:34, sebsg wrote:
> Can you also make sure that the normal histograms are logged correctly?

Done.

https://codereview.chromium.org/2672623005/diff/60001/components/autofill/cor...
components/autofill/core/browser/autofill_metrics_unittest.cc:4326:
AutofillMetrics::FORM_EVENT_NO_SUGGESTION_SUBMITTED_ONCE, 1);
On 2017/02/03 19:28:34, sebsg wrote:
> Can you add a check to make sure that nothing was logged for your new
histogram?
> Like 
> histograms.ExpectBucketCount("Autofill.FormEvents.CreditCard.OnNonsecurePage",
> AutofillMetrics::FORM_EVENT_NO_SUGGESTION_SUBMITTED_ONCE, 0);

Done.

[Mathieu, 2017-02-04 02:27:34]

Description was changed from

==========
Record Autofill form events specially for nonsecure pages

The Form-Not-Secure experiment shows special "Payment not secure" warnings for
credit card forms on HTTP pages. This CL records credit card autofill form
events in a special histogram broken out for nonsecure pages, to measure the
impact of these "Payment not secure" warnings on form interactions.

To do so, a bit of plumbing was needed, because the FormEventLogger needs to
know whether the main frame URL is secure. So I introduced a new
AutofillClient::IsMainUrlSecure() method to provide that.

IsMainUrlSecure() is very similar to the existing IsContextSecure() method,
except that it doesn't take a |form_origin| parameter, and it computes the
result based on the top-level URL on all platforms (as opposed to
IsContextSecure which currently incorrectly computes the result based only on
|form_origin|). Fortunately, there was already a TODO to make IsContextSecure
use the main-frame URL on all platforms (https://crbug.com/505388), so this CL
gets us part of the way there. In a follow-up CL, I can remove IsContextSecure
and start using IsMainUrlSecure in its place. (I don't want to do that in this
CL because I may want to merge it to M57 and thus don't want it to contain
functionality changes, just histogram changes.)

BUG=677291,687823,505388
==========

to

==========
Record Autofill form events specially for nonsecure pages

The Form-Not-Secure experiment shows special "Payment not secure" warnings for
credit card forms on HTTP pages. This CL records credit card autofill form
events in a special histogram broken out for nonsecure pages, to measure the
impact of these "Payment not secure" warnings on form interactions.

To do so, a bit of plumbing was needed, because the FormEventLogger needs to
know whether the main frame URL is secure. So I introduced a new
AutofillClient::IsMainUrlSecure() method to provide that.

IsMainUrlSecure() is very similar to the existing IsContextSecure() method,
except that it doesn't take a |form_origin| parameter, and it computes the
result based on the top-level URL on all platforms (as opposed to
IsContextSecure which currently incorrectly computes the result based only on
|form_origin|). Fortunately, there was already a TODO to make IsContextSecure
use the main-frame URL on all platforms (https://crbug.com/505388), so this CL
gets us part of the way there. In a follow-up CL, I can remove IsContextSecure
and start using IsMainUrlSecure in its place. (I don't want to do that in this
CL because I may want to merge it to M57 and thus don't want it to contain
functionality changes, just histogram changes.)

BUG=677291,687823,505388
==========

[Mathieu, 2017-02-04 02:27:34]

mathp@chromium.org changed reviewers:
+ mathp@chromium.org
- estade@chromium.org

[Mathieu, 2017-02-04 02:34:57]

Hi Emily, 

I'm not sure about adding IsMainUrlSecure, mostly equivalent to the existing
IsContextSecure, if your intention is to remove one of them as a follow-up
patch. You could save a ton of work if you just used the implementation of
IsContextSecure for your histogram (improving it for iOS). I don't see risks
(but rather wins) in merging an updated IsContextSecure for iOS, do you?

https://codereview.chromium.org/2672623005/diff/80001/ios/chrome/browser/ui/a...
File ios/chrome/browser/ui/autofill/autofill_client_ios.mm (right):

https://codereview.chromium.org/2672623005/diff/80001/ios/chrome/browser/ui/a...
ios/chrome/browser/ui/autofill/autofill_client_ios.mm:189: // TODO (sigbjorn):
Return if the context is secure, not just
I feel like this TODO could be addressed if we moved the impl that you added in
IsMainUrlSecure to this?

https://codereview.chromium.org/2672623005/diff/80001/ios/chrome/browser/ui/a...
ios/chrome/browser/ui/autofill/autofill_client_ios.mm:194: bool
AutofillClientIOS::IsMainUrlSecure() {
By splitting IsContextSecure and IsMainUrlSecure on iOS, aren't you logging
based on one logic (IsMainUrlSecure) while deciding on the Form HTTP warning
using another logic (IsContextSecure)?

[commit-bot: I haz the power, 2017-02-04 05:33:37]

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

[commit-bot: I haz the power, 2017-02-04 05:33:38]

Dry run: Try jobs failed on following builders:
  linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT,
no build URL)
  linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux
(JOB_TIMED_OUT, no build URL)
  linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, no
build URL)
  linux_chromium_tsan_rel_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT,
no build URL)

[estark, 2017-02-06 22:29:15]

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

[commit-bot: I haz the power, 2017-02-06 22:30:21]

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, 2017-02-06 22:40:37]

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

[commit-bot: I haz the power, 2017-02-06 22:40:39]

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

[estark, 2017-02-06 22:46:27]

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

[commit-bot: I haz the power, 2017-02-06 22:47:03]

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

[estark, 2017-02-06 22:51:02]

https://codereview.chromium.org/2672623005/diff/80001/ios/chrome/browser/ui/a...
File ios/chrome/browser/ui/autofill/autofill_client_ios.mm (right):

https://codereview.chromium.org/2672623005/diff/80001/ios/chrome/browser/ui/a...
ios/chrome/browser/ui/autofill/autofill_client_ios.mm:189: // TODO (sigbjorn):
Return if the context is secure, not just
On 2017/02/04 02:34:57, Mathieu Perreault wrote:
> I feel like this TODO could be addressed if we moved the impl that you added
in
> IsMainUrlSecure to this?

If you don't have any qualms about making a functionality change in this patch,
then that's fine with me. Updated to remove the GURL argument from
IsContextSecure, removed IsMainUrlSecure, and moved the implementation in here.

https://codereview.chromium.org/2672623005/diff/80001/ios/chrome/browser/ui/a...
ios/chrome/browser/ui/autofill/autofill_client_ios.mm:194: bool
AutofillClientIOS::IsMainUrlSecure() {
On 2017/02/04 02:34:57, Mathieu Perreault wrote:
> By splitting IsContextSecure and IsMainUrlSecure on iOS, aren't you logging
> based on one logic (IsMainUrlSecure) while deciding on the Form HTTP warning
> using another logic (IsContextSecure)?

Yes, but Form-Not-Secure warnings aren't enabled for iOS so it doesn't really
matter -- we won't be looking at these metrics for iOS. (I suppose, since they
don't matter for iOS, I could have just used IsContextSecure as-is with the
faulty implementation on iOS, but I thought it would be a recipe for disaster to
have a metric that's recorded under very different conditions on different
platforms.)

Anyway, I've done as you suggested and gotten rid of IsMainUrlSecure, so this
isn't a problem anymore.

[commit-bot: I haz the power, 2017-02-06 22:55:25]

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

[commit-bot: I haz the power, 2017-02-06 22:55:27]

Dry run: Try jobs failed on following builders:
  ios-simulator on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
  ios-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xco...)

[estark, 2017-02-06 23:01:17]

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

[commit-bot: I haz the power, 2017-02-06 23:02:02]

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, 2017-02-07 01:37:40]

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

[commit-bot: I haz the power, 2017-02-07 01:37:41]

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

[sgurun-gerrit only, 2017-02-07 16:14:59]

aw lgtm

[Mathieu, 2017-02-07 17:27:14]

On 2017/02/07 16:14:59, sgurun wrote:
> aw lgtm

LGTM, but the description is outdated. Thanks!

[Justin Donnelly, 2017-02-07 17:42:44]

jdonnelly@chromium.org changed reviewers:
+ eugenebut@chromium.org

[Justin Donnelly, 2017-02-07 17:42:45]

+eugenebut (please double-check that usage of NavigationManager in
autofill_client_ios.mm is correct)

ios lgtm but please wait for eugenebut as I'm not that familiar with the
behavior of NavigationManager.

https://codereview.chromium.org/2672623005/diff/160001/ios/chrome/browser/ui/...
File ios/chrome/browser/ui/autofill/autofill_client_ios.mm (right):

https://codereview.chromium.org/2672623005/diff/160001/ios/chrome/browser/ui/...
ios/chrome/browser/ui/autofill/autofill_client_ios.mm:188: bool
AutofillClientIOS::IsContextSecure() {
Can you add a comment to this function explaining that it differs from the other
platforms? (Doesn't check RAN_INSECURE_CONTENT because the iOS web view doesn't
expose this information.)

[Eugene But (OOO till 7-30), 2017-02-07 18:47:09]

https://codereview.chromium.org/2672623005/diff/160001/ios/chrome/browser/ui/...
File ios/chrome/browser/ui/autofill/autofill_client_ios.mm (right):

https://codereview.chromium.org/2672623005/diff/160001/ios/chrome/browser/ui/...
ios/chrome/browser/ui/autofill/autofill_client_ios.mm:30: #include
"ios/web/public/navigation_item.h"
s/include/import

https://google.github.io/styleguide/objcguide.xml#_import_and__include

https://codereview.chromium.org/2672623005/diff/160001/ios/chrome/browser/ui/...
ios/chrome/browser/ui/autofill/autofill_client_ios.mm:31: #include
"ios/web/public/navigation_manager.h"
ditto

https://codereview.chromium.org/2672623005/diff/160001/ios/chrome/browser/ui/...
ios/chrome/browser/ui/autofill/autofill_client_ios.mm:33: #include
"ios/web/public/web_state/web_state.h"
ditto

https://codereview.chromium.org/2672623005/diff/160001/ios/chrome/browser/ui/...
ios/chrome/browser/ui/autofill/autofill_client_ios.mm:190: web::NavigationItem*
navItem = manager->GetVisibleItem();
Do you want to use LastCommittedItem instead? 

LastCommittedItem is actual navigation item of the page. VisibleItem can be
transient or even pending item if it was not initiated by renderer.

https://codereview.chromium.org/2672623005/diff/160001/ios/chrome/browser/ui/...
ios/chrome/browser/ui/autofill/autofill_client_ios.mm:190: web::NavigationItem*
navItem = manager->GetVisibleItem();
s/navItem/nav_item

https://codereview.chromium.org/2672623005/diff/160001/ios/chrome/browser/ui/...
ios/chrome/browser/ui/autofill/autofill_client_ios.mm:197:
net::IsCertStatusMinorError(ssl.cert_status));
Do you want to check for |SECURITY_STYLE_AUTHENTICATED| instead of checking for
scheme and cert?

[estark, 2017-02-07 22:39:40]

Description was changed from

==========
Record Autofill form events specially for nonsecure pages

The Form-Not-Secure experiment shows special "Payment not secure" warnings for
credit card forms on HTTP pages. This CL records credit card autofill form
events in a special histogram broken out for nonsecure pages, to measure the
impact of these "Payment not secure" warnings on form interactions.

To do so, a bit of plumbing was needed, because the FormEventLogger needs to
know whether the main frame URL is secure. So I introduced a new
AutofillClient::IsMainUrlSecure() method to provide that.

IsMainUrlSecure() is very similar to the existing IsContextSecure() method,
except that it doesn't take a |form_origin| parameter, and it computes the
result based on the top-level URL on all platforms (as opposed to
IsContextSecure which currently incorrectly computes the result based only on
|form_origin|). Fortunately, there was already a TODO to make IsContextSecure
use the main-frame URL on all platforms (https://crbug.com/505388), so this CL
gets us part of the way there. In a follow-up CL, I can remove IsContextSecure
and start using IsMainUrlSecure in its place. (I don't want to do that in this
CL because I may want to merge it to M57 and thus don't want it to contain
functionality changes, just histogram changes.)

BUG=677291,687823,505388
==========

to

==========
Record Autofill form events specially for nonsecure pages

The Form-Not-Secure experiment shows special "Payment not secure" warnings for
credit card forms on HTTP pages. This CL records credit card autofill form
events in a special histogram broken out for nonsecure pages, to measure the
impact of these "Payment not secure" warnings on form interactions.

To do so, the FormEventLogger needs to know whether the main frame URL is
secure, at a point at which it doesn't have access to a form origin URL. iOS is
the only platform on which IsContextSecure used the form_origin parameter, and
there was an existing TODO to fix that (https://crbug.com/505388). So this CL
removes the form_origin parameter from IsContextSecure, fixes the iOS TODO, and
uses the resulting IsContextSecure method to record nonsecure form metrics.

BUG=677291,687823,505388
==========

[estark, 2017-02-07 22:39:49]

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

[estark, 2017-02-07 22:40:34]

https://codereview.chromium.org/2672623005/diff/160001/ios/chrome/browser/ui/...
File ios/chrome/browser/ui/autofill/autofill_client_ios.mm (right):

https://codereview.chromium.org/2672623005/diff/160001/ios/chrome/browser/ui/...
ios/chrome/browser/ui/autofill/autofill_client_ios.mm:30: #include
"ios/web/public/navigation_item.h"
On 2017/02/07 18:47:08, Eugene But wrote:
> s/include/import
> 
> https://google.github.io/styleguide/objcguide.xml#_import_and__include

Done.

https://codereview.chromium.org/2672623005/diff/160001/ios/chrome/browser/ui/...
ios/chrome/browser/ui/autofill/autofill_client_ios.mm:31: #include
"ios/web/public/navigation_manager.h"
On 2017/02/07 18:47:08, Eugene But wrote:
> ditto

Done.

https://codereview.chromium.org/2672623005/diff/160001/ios/chrome/browser/ui/...
ios/chrome/browser/ui/autofill/autofill_client_ios.mm:33: #include
"ios/web/public/web_state/web_state.h"
On 2017/02/07 18:47:08, Eugene But wrote:
> ditto

Done.

https://codereview.chromium.org/2672623005/diff/160001/ios/chrome/browser/ui/...
ios/chrome/browser/ui/autofill/autofill_client_ios.mm:188: bool
AutofillClientIOS::IsContextSecure() {
On 2017/02/07 17:42:45, Justin Donnelly wrote:
> Can you add a comment to this function explaining that it differs from the
other
> platforms? (Doesn't check RAN_INSECURE_CONTENT because the iOS web view
doesn't
> expose this information.)

Done.

https://codereview.chromium.org/2672623005/diff/160001/ios/chrome/browser/ui/...
ios/chrome/browser/ui/autofill/autofill_client_ios.mm:190: web::NavigationItem*
navItem = manager->GetVisibleItem();
On 2017/02/07 18:47:08, Eugene But wrote:
> s/navItem/nav_item

Done.

https://codereview.chromium.org/2672623005/diff/160001/ios/chrome/browser/ui/...
ios/chrome/browser/ui/autofill/autofill_client_ios.mm:190: web::NavigationItem*
navItem = manager->GetVisibleItem();
On 2017/02/07 18:47:08, Eugene But wrote:
> Do you want to use LastCommittedItem instead? 
> 
> LastCommittedItem is actual navigation item of the page. VisibleItem can be
> transient or even pending item if it was not initiated by renderer.

Done.

https://codereview.chromium.org/2672623005/diff/160001/ios/chrome/browser/ui/...
ios/chrome/browser/ui/autofill/autofill_client_ios.mm:197:
net::IsCertStatusMinorError(ssl.cert_status));
On 2017/02/07 18:47:08, Eugene But wrote:
> Do you want to check for |SECURITY_STYLE_AUTHENTICATED| instead of checking
for
> scheme and cert?

|security_style| doesn't exist in //content anymore, I wonder if ios should make
a similar change.

I have a slight preference for leaving this as-is to match the other platforms
as much as possible, but happy to change it if you feel strongly.

[commit-bot: I haz the power, 2017-02-07 22:41:06]

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

[estark, 2017-02-07 22:42:25]

estark@chromium.org changed reviewers:
+ dvadym@chromium.org, jwd@chromium.org

[estark, 2017-02-07 22:42:26]

+dvadym for the small change to components/password_manager, +jwd for
histograms.xml

[Eugene But (OOO till 7-30), 2017-02-07 23:15:02]

Thanks! ios lgtm

[commit-bot: I haz the power, 2017-02-08 00:05:50]

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

[commit-bot: I haz the power, 2017-02-08 00:05:51]

Dry run: This issue passed the CQ dry run.

[dvadym, 2017-02-08 09:25:05]

components/password_manager/* LGTM

[sebsg, 2017-02-09 14:28:08]

lgtm thanks!

[jwd, 2017-02-09 16:05:35]

lgtm

[estark, 2017-02-09 18:07:50]

The CQ bit was checked by estark@chromium.org

[estark, 2017-02-09 18:07:51]

The patchset sent to the CQ was uploaded after l-g-t-m from mathp@chromium.org,
sgurun@chromium.org, jdonnelly@chromium.org
Link to the patchset: https://codereview.chromium.org/2672623005/#ps180001
(title: "jdonnelly, eugenebut comments")

[commit-bot: I haz the power, 2017-02-09 18:08:35]

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, 2017-02-09 18:11:29]

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

[commit-bot: I haz the power, 2017-02-09 18:11:31]

Try jobs failed on following builders:
  ios-device on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...)
  ios-device-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-xcode-...)
  ios-simulator on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
  ios-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xco...)
  win_chromium_compile_dbg_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_comp...)

[estark, 2017-02-09 18:17:40]

Patchset #11 (id:200001) has been deleted

[estark, 2017-02-09 18:19:12]

The CQ bit was checked by estark@chromium.org

[estark, 2017-02-09 18:19:12]

The patchset sent to the CQ was uploaded after l-g-t-m from sgurun@chromium.org,
sebsg@chromium.org, mathp@chromium.org, eugenebut@chromium.org,
dvadym@chromium.org, jwd@chromium.org, jdonnelly@chromium.org
Link to the patchset: https://codereview.chromium.org/2672623005/#ps220001
(title: "rebase")

[commit-bot: I haz the power, 2017-02-09 18:19:58]

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, 2017-02-09 18:29:33]

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

[commit-bot: I haz the power, 2017-02-09 18:29:35]

Try jobs failed on following builders:
  ios-device on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...)
  ios-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xco...)

[estark, 2017-02-09 18:32:34]

The CQ bit was checked by estark@chromium.org

[estark, 2017-02-09 18:32:34]

The patchset sent to the CQ was uploaded after l-g-t-m from sgurun@chromium.org,
sebsg@chromium.org, mathp@chromium.org, eugenebut@chromium.org,
dvadym@chromium.org, jwd@chromium.org, jdonnelly@chromium.org
Link to the patchset: https://codereview.chromium.org/2672623005/#ps240001
(title: "rebase fixup")

[commit-bot: I haz the power, 2017-02-09 18:33:29]

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, 2017-02-09 19:13:05]

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

[commit-bot: I haz the power, 2017-02-09 19:13:07]

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

[estark, 2017-02-09 19:35:01]

The CQ bit was checked by estark@chromium.org

[estark, 2017-02-09 19:35:01]

The patchset sent to the CQ was uploaded after l-g-t-m from sgurun@chromium.org,
sebsg@chromium.org, mathp@chromium.org, eugenebut@chromium.org,
dvadym@chromium.org, jwd@chromium.org, jdonnelly@chromium.org
Link to the patchset: https://codereview.chromium.org/2672623005/#ps260001
(title: "fix test added in rebase")

[commit-bot: I haz the power, 2017-02-09 19:35:43]

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, 2017-02-09 20:50:07]

CQ is committing da patch.

Bot data: {"patchset_id": 260001, "attempt_start_ts": 1486668901095790,
"parent_rev": "145325203534c94394bcb6966851486b8fcd29e8", "commit_rev":
"57464d6c82bea1c759874da3101135e1ae1494eb"}

[commit-bot: I haz the power, 2017-02-09 20:50:43]

Description was changed from

==========
Record Autofill form events specially for nonsecure pages

The Form-Not-Secure experiment shows special "Payment not secure" warnings for
credit card forms on HTTP pages. This CL records credit card autofill form
events in a special histogram broken out for nonsecure pages, to measure the
impact of these "Payment not secure" warnings on form interactions.

To do so, the FormEventLogger needs to know whether the main frame URL is
secure, at a point at which it doesn't have access to a form origin URL. iOS is
the only platform on which IsContextSecure used the form_origin parameter, and
there was an existing TODO to fix that (https://crbug.com/505388). So this CL
removes the form_origin parameter from IsContextSecure, fixes the iOS TODO, and
uses the resulting IsContextSecure method to record nonsecure form metrics.

BUG=677291,687823,505388
==========

to

==========
Record Autofill form events specially for nonsecure pages

The Form-Not-Secure experiment shows special "Payment not secure" warnings for
credit card forms on HTTP pages. This CL records credit card autofill form
events in a special histogram broken out for nonsecure pages, to measure the
impact of these "Payment not secure" warnings on form interactions.

To do so, the FormEventLogger needs to know whether the main frame URL is
secure, at a point at which it doesn't have access to a form origin URL. iOS is
the only platform on which IsContextSecure used the form_origin parameter, and
there was an existing TODO to fix that (https://crbug.com/505388). So this CL
removes the form_origin parameter from IsContextSecure, fixes the iOS TODO, and
uses the resulting IsContextSecure method to record nonsecure form metrics.

BUG=677291,687823,505388

Review-Url: https://codereview.chromium.org/2672623005
Cr-Commit-Position: refs/heads/master@{#449402}
Committed:
https://chromium.googlesource.com/chromium/src/+/57464d6c82bea1c759874da31011...
==========

[commit-bot: I haz the power, 2017-02-09 20:50:45]

Committed patchset #13 (id:260001) as
https://chromium.googlesource.com/chromium/src/+/57464d6c82bea1c759874da31011...