Deprecate CredentialRequestOptions.unmediated in favor mediation enum

Deprecate CredentialRequestOptions.unmediated in favor mediation enum

This change introduces an `CredentialMediationRequirement` enum argument to
`CredentialsContainer::get()` that is intended to replace the currently existing
boolean flag `unmediated`. The enum will have three states ("silent",
"optional", "required") with "silent" and "optional" directly mapping to the
existing boolean states, "required" is a novel option.

Intent to Ship:
https://groups.google.com/a/chromium.org/d/msg/blink-dev/veCKBk1NguM/ctyHrdVWAwAJ

BUG=721399
Review-Url: https://codereview.chromium.org/2864493003
Cr-Commit-Position: refs/heads/master@{#473532}
Committed: https://chromium.googlesource.com/chromium/src/+/46feffcdbeca2da9245a3c48ff6d81d08f26d5d8

[jdoerrie, 2017-05-05 14:40:53]

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

[commit-bot: I haz the power, 2017-05-05 14:41:06]

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-05-05 14:51:02]

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

[commit-bot: I haz the power, 2017-05-05 14:51:03]

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...)

[jdoerrie, 2017-05-08 10:04:41]

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

[commit-bot: I haz the power, 2017-05-08 10:05:10]

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-05-08 10:29:39]

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

[commit-bot: I haz the power, 2017-05-08 10:29:40]

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

[jdoerrie, 2017-05-17 16:12:28]

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

[commit-bot: I haz the power, 2017-05-17 16:13:23]

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-05-17 16:41:18]

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

[commit-bot: I haz the power, 2017-05-17 16:41:19]

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...)

[jdoerrie, 2017-05-18 06:27:56]

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

[commit-bot: I haz the power, 2017-05-18 06:28:28]

Dry run: CQ is trying da patch.

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

[jdoerrie, 2017-05-18 06:29:54]

jdoerrie@chromium.org changed reviewers:
+ mkwst@chromium.org, vasilii@chromium.org

[jdoerrie, 2017-05-18 06:29:54]

vasilii@chromium.org: Please review changes in //components/password_manager

mkwst@chromium.org: Please review changes in the remaining files

[jdoerrie, 2017-05-18 06:41:13]

Description was changed from

==========
Deprecate CredentialRequestOptions.unmediated in favor mediation enum

BUG=
==========

to

==========
Deprecate CredentialRequestOptions.unmediated in favor mediation enum

This change introduces an `CredentialMediationRequirement` enum argument to
`CredentialsContainer::get()` that is intended to replace the currently existing
boolean flag `unmediated`. The enum will have three states ("silent",
"optional", "required") with "silent" and "optional" directly mapping to the
existing boolean states, "required" is a novel option.


BUG=721399
==========

[jdoerrie, 2017-05-18 06:42:05]

Description was changed from

==========
Deprecate CredentialRequestOptions.unmediated in favor mediation enum

This change introduces an `CredentialMediationRequirement` enum argument to
`CredentialsContainer::get()` that is intended to replace the currently existing
boolean flag `unmediated`. The enum will have three states ("silent",
"optional", "required") with "silent" and "optional" directly mapping to the
existing boolean states, "required" is a novel option.


BUG=721399
==========

to

==========
Deprecate CredentialRequestOptions.unmediated in favor mediation enum

This change introduces an `CredentialMediationRequirement` enum argument to
`CredentialsContainer::get()` that is intended to replace the currently existing
boolean flag `unmediated`. The enum will have three states ("silent",
"optional", "required") with "silent" and "optional" directly mapping to the
existing boolean states, "required" is a novel option.

Intent to Ship:
https://groups.google.com/a/chromium.org/d/msg/blink-dev/veCKBk1NguM/ctyHrdVW...

BUG=721399
==========

[commit-bot: I haz the power, 2017-05-18 07:10:49]

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

[commit-bot: I haz the power, 2017-05-18 07:10:50]

Dry run: Try jobs failed on following builders:
  win_clang on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_clang/builds/...)

[jdoerrie, 2017-05-18 08:07:51]

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

[commit-bot: I haz the power, 2017-05-18 08:08:19]

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-05-18 08:54:23]

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

[commit-bot: I haz the power, 2017-05-18 08:54:24]

Dry run: Try jobs failed on following builders:
  win_chromium_compile_dbg_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_comp...)

[vasilii, 2017-05-18 12:34:04]

https://codereview.chromium.org/2864493003/diff/80001/components/password_man...
File components/password_manager/content/browser/credential_manager_impl.cc
(right):

https://codereview.chromium.org/2864493003/diff/80001/components/password_man...
components/password_manager/content/browser/credential_manager_impl.cc:165: }();
Ideally it should go away.

https://codereview.chromium.org/2864493003/diff/80001/components/password_man...
components/password_manager/content/browser/credential_manager_impl.cc:247:
const SendCredentialCallback& send_callback,
It should get |mediation| as a parameter. |mediation_| should go away.

https://codereview.chromium.org/2864493003/diff/80001/components/password_man...
File components/password_manager/content/browser/credential_manager_impl.h
(right):

https://codereview.chromium.org/2864493003/diff/80001/components/password_man...
components/password_manager/content/browser/credential_manager_impl.h:56: void
Get(mojom::CredentialMediationRequirement mediation,
Can we use only the enum declared in credential_manager_types?
See components/autofill/content/common/autofill_types.typemap

https://codereview.chromium.org/2864493003/diff/80001/components/password_man...
File
components/password_manager/content/browser/credential_manager_impl_unittest.cc
(right):

https://codereview.chromium.org/2864493003/diff/80001/components/password_man...
components/password_manager/content/browser/credential_manager_impl_unittest.cc:296:
mojom::CredentialMediationRequirement mediation,
No "mojo::"

https://codereview.chromium.org/2864493003/diff/80001/components/password_man...
components/password_manager/content/browser/credential_manager_impl_unittest.cc:841:
ExpectCredentialType(mojom::CredentialMediationRequirement::SILENT, true,
We have CredentialManagerOnRequestCredentialWithZeroClickOnlyEmptyPasswordStore
for this.

The test in the file are split into account chooser part and zero-click part.
Here you want to test CredentialMediationRequirement::OPTIONAL only. For
CredentialMediationRequirement::REQUIRED I'd create a separate section. You can
avoid code duplication by making the test body a function taking
CredentialMediationRequirement parameter.

https://codereview.chromium.org/2864493003/diff/80001/components/password_man...
components/password_manager/content/browser/credential_manager_impl_unittest.cc:857:
ExpectCredentialType(mojom::CredentialMediationRequirement::REQUIRED, true,
I'd avoid calling Get() twice in the same test unless it actually reproduces a
bug.
Get() and other methods have side effects on the password store. Thus, writing
one test or two are not the same.

https://codereview.chromium.org/2864493003/diff/80001/components/password_man...
components/password_manager/content/browser/credential_manager_impl_unittest.cc:1013:
ExpectZeroClickSignInFailure(mojom::CredentialMediationRequirement::REQUIRED,
Here and below, we shouldn't mix CredentialMediationRequirement::REQUIRED and
CredentialMediationRequirement::SILENT tests.

https://codereview.chromium.org/2864493003/diff/80001/components/password_man...
components/password_manager/content/browser/credential_manager_impl_unittest.cc:1633:
}
There should be a new test for CredentialMediationRequirement::REQUIRED:
- Setup everything that auto sign-in can happen.
- Call Get(REQUIRED)
- Check that autosign in didn't happen but the account chooser was shown.

https://codereview.chromium.org/2864493003/diff/80001/components/password_man...
File components/password_manager/core/browser/credential_manager_logger.cc
(right):

https://codereview.chromium.org/2864493003/diff/80001/components/password_man...
components/password_manager/core/browser/credential_manager_logger.cc:31: s +=
"silent";
What about breaks?

https://codereview.chromium.org/2864493003/diff/80001/components/password_man...
File
components/password_manager/core/browser/credential_manager_pending_request_task.cc
(right):

https://codereview.chromium.org/2864493003/diff/80001/components/password_man...
components/password_manager/core/browser/credential_manager_pending_request_task.cc:195:
local_results.size() == 1u && delegate_->IsZeroClickAllowed() &&
I think the class should not ask the delegate unnecessary questions in case
mediation is required.

https://codereview.chromium.org/2864493003/diff/80001/components/password_man...
File components/password_manager/core/browser/password_manager_metrics_util.h
(right):

https://codereview.chromium.org/2864493003/diff/80001/components/password_man...
components/password_manager/core/browser/password_manager_metrics_util.h:246: //
Log the result of navigator.credentials.get. |status| specifies the
Not |status| anymore.

https://codereview.chromium.org/2864493003/diff/80001/third_party/WebKit/Sour...
File
third_party/WebKit/Source/modules/credentialmanager/CredentialsContainer.cpp
(right):

https://codereview.chromium.org/2864493003/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/credentialmanager/CredentialsContainer.cpp:187:
// Show a warning that unmediated will be ignored?
Should be an exception I think.

https://codereview.chromium.org/2864493003/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/credentialmanager/CredentialsContainer.cpp:199:
} else {
What about an exception in case of random trash string?

https://codereview.chromium.org/2864493003/diff/80001/third_party/WebKit/publ...
File third_party/WebKit/public/platform/WebCredentialMediationRequirement.h
(right):

https://codereview.chromium.org/2864493003/diff/80001/third_party/WebKit/publ...
third_party/WebKit/public/platform/WebCredentialMediationRequirement.h:10: enum
WebCredentialMediationRequirement {
What about an enum class so you can drop the long prefix below?

https://codereview.chromium.org/2864493003/diff/80001/tools/metrics/histogram...
File tools/metrics/histograms/enums.xml (right):

https://codereview.chromium.org/2864493003/diff/80001/tools/metrics/histogram...
tools/metrics/histograms/enums.xml:14987: +     
label="CredentialManagerCredentialRequestOptionsUnmediated"/>
Is it ever used for counting?

[jdoerrie, 2017-05-18 14:30:35]

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

[commit-bot: I haz the power, 2017-05-18 14:31:00]

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-05-18 14:43:30]

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

[commit-bot: I haz the power, 2017-05-18 14:43:31]

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...)

[jdoerrie, 2017-05-18 14:56:27]

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

[commit-bot: I haz the power, 2017-05-18 14:56:55]

Dry run: CQ is trying da patch.

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

[jdoerrie, 2017-05-18 14:57:25]

https://codereview.chromium.org/2864493003/diff/80001/components/password_man...
File components/password_manager/content/browser/credential_manager_impl.cc
(right):

https://codereview.chromium.org/2864493003/diff/80001/components/password_man...
components/password_manager/content/browser/credential_manager_impl.cc:165: }();
On 2017/05/18 12:34:03, vasilii(OOO til 18.05) wrote:
> Ideally it should go away.

Done.

https://codereview.chromium.org/2864493003/diff/80001/components/password_man...
components/password_manager/content/browser/credential_manager_impl.cc:247:
const SendCredentialCallback& send_callback,
On 2017/05/18 12:34:03, vasilii(OOO til 18.05) wrote:
> It should get |mediation| as a parameter. |mediation_| should go away.

Done.

https://codereview.chromium.org/2864493003/diff/80001/components/password_man...
File components/password_manager/content/browser/credential_manager_impl.h
(right):

https://codereview.chromium.org/2864493003/diff/80001/components/password_man...
components/password_manager/content/browser/credential_manager_impl.h:56: void
Get(mojom::CredentialMediationRequirement mediation,
On 2017/05/18 12:34:03, vasilii(OOO til 18.05) wrote:
> Can we use only the enum declared in credential_manager_types?
> See components/autofill/content/common/autofill_types.typemap 

Done, this required entries in

components/password_manager/content/common/credential_manager.typemap

and
components/password_manager/content/common/credential_manager_struct_traits.{h,cc}

https://codereview.chromium.org/2864493003/diff/80001/components/password_man...
File
components/password_manager/content/browser/credential_manager_impl_unittest.cc
(right):

https://codereview.chromium.org/2864493003/diff/80001/components/password_man...
components/password_manager/content/browser/credential_manager_impl_unittest.cc:296:
mojom::CredentialMediationRequirement mediation,
On 2017/05/18 12:34:03, vasilii(OOO til 18.05) wrote:
> No "mojo::"

Done.

https://codereview.chromium.org/2864493003/diff/80001/components/password_man...
File components/password_manager/core/browser/credential_manager_logger.cc
(right):

https://codereview.chromium.org/2864493003/diff/80001/components/password_man...
components/password_manager/core/browser/credential_manager_logger.cc:31: s +=
"silent";
On 2017/05/18 12:34:03, vasilii(OOO til 18.05) wrote:
> What about breaks?

Done.

https://codereview.chromium.org/2864493003/diff/80001/components/password_man...
File
components/password_manager/core/browser/credential_manager_pending_request_task.cc
(right):

https://codereview.chromium.org/2864493003/diff/80001/components/password_man...
components/password_manager/core/browser/credential_manager_pending_request_task.cc:195:
local_results.size() == 1u && delegate_->IsZeroClickAllowed() &&
On 2017/05/18 12:34:03, vasilii(OOO til 18.05) wrote:
> I think the class should not ask the delegate unnecessary questions in case
> mediation is required.

Done.

https://codereview.chromium.org/2864493003/diff/80001/components/password_man...
File components/password_manager/core/browser/password_manager_metrics_util.h
(right):

https://codereview.chromium.org/2864493003/diff/80001/components/password_man...
components/password_manager/core/browser/password_manager_metrics_util.h:246: //
Log the result of navigator.credentials.get. |status| specifies the
On 2017/05/18 12:34:03, vasilii(OOO til 18.05) wrote:
> Not |status| anymore.

Acknowledged, I'll drop it. '|mediation| specifies the "mediation" parameter of
the API method.' would be pretty redundant.

https://codereview.chromium.org/2864493003/diff/80001/third_party/WebKit/Sour...
File
third_party/WebKit/Source/modules/credentialmanager/CredentialsContainer.cpp
(right):

https://codereview.chromium.org/2864493003/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/credentialmanager/CredentialsContainer.cpp:187:
// Show a warning that unmediated will be ignored?
On 2017/05/18 12:34:03, vasilii(OOO til 18.05) wrote:
> Should be an exception I think.

Done.

https://codereview.chromium.org/2864493003/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/credentialmanager/CredentialsContainer.cpp:199:
} else {
On 2017/05/18 12:34:03, vasilii(OOO til 18.05) wrote:
> What about an exception in case of random trash string?

The generated code already includes a call to IsValidEnum:
https://codesearch.chromium.org/chromium/src/third_party/WebKit/Source/bindin...

This is sufficient for user facing exceptions:
$ navigator.credentials.get({mediation: "foobar"})

Uncaught (in promise) TypeError: Failed to execute 'get' on
'CredentialsContainer': The provided value 'foobar' is not a valid enum value of
type CredentialMediationRequirement.

I added a DCHECK to guard against programmer errors, however.

https://codereview.chromium.org/2864493003/diff/80001/third_party/WebKit/publ...
File third_party/WebKit/public/platform/WebCredentialMediationRequirement.h
(right):

https://codereview.chromium.org/2864493003/diff/80001/third_party/WebKit/publ...
third_party/WebKit/public/platform/WebCredentialMediationRequirement.h:10: enum
WebCredentialMediationRequirement {
On 2017/05/18 12:34:03, vasilii(OOO til 18.05) wrote:
> What about an enum class so you can drop the long prefix below?

Done.

https://codereview.chromium.org/2864493003/diff/80001/tools/metrics/histogram...
File tools/metrics/histograms/enums.xml (right):

https://codereview.chromium.org/2864493003/diff/80001/tools/metrics/histogram...
tools/metrics/histograms/enums.xml:14987: +     
label="CredentialManagerCredentialRequestOptionsUnmediated"/>
On 2017/05/18 12:34:04, vasilii(OOO til 18.05) wrote:
> Is it ever used for counting?

They way I understand it this file has a bijection with UseCounter.h. There we
need the entry to show the deprecation warning. Maybe this is used internally to
count how often the deprecated method is called?

[commit-bot: I haz the power, 2017-05-18 15:38:58]

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

[commit-bot: I haz the power, 2017-05-18 15:39:00]

Dry run: Try jobs failed on following builders:
  win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)

[vasilii, 2017-05-18 16:16:13]

https://codereview.chromium.org/2864493003/diff/80001/components/password_man...
File
components/password_manager/content/browser/credential_manager_impl_unittest.cc
(right):

https://codereview.chromium.org/2864493003/diff/80001/components/password_man...
components/password_manager/content/browser/credential_manager_impl_unittest.cc:1633:
}
On 2017/05/18 12:34:03, vasilii wrote:
> There should be a new test for CredentialMediationRequirement::REQUIRED:
> - Setup everything that auto sign-in can happen.
> - Call Get(REQUIRED)
> - Check that autosign in didn't happen but the account chooser was shown.

Not addressed.

https://codereview.chromium.org/2864493003/diff/80001/tools/metrics/histogram...
File tools/metrics/histograms/enums.xml (right):

https://codereview.chromium.org/2864493003/diff/80001/tools/metrics/histogram...
tools/metrics/histograms/enums.xml:14987: +     
label="CredentialManagerCredentialRequestOptionsUnmediated"/>
On 2017/05/18 14:57:25, jdoerrie wrote:
> On 2017/05/18 12:34:04, vasilii(OOO til 18.05) wrote:
> > Is it ever used for counting?
> 
> They way I understand it this file has a bijection with UseCounter.h. There we
> need the entry to show the deprecation warning. Maybe this is used internally
to
> count how often the deprecated method is called?

Acknowledged.

https://codereview.chromium.org/2864493003/diff/120001/third_party/WebKit/Sou...
File
third_party/WebKit/Source/modules/credentialmanager/CredentialRequestOptions.idl
(right):

https://codereview.chromium.org/2864493003/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/credentialmanager/CredentialRequestOptions.idl:7:
[RuntimeEnabled=CredentialRequestOptionsMediation]
What is the motivation for this?

https://codereview.chromium.org/2864493003/diff/120001/third_party/WebKit/Sou...
File
third_party/WebKit/Source/modules/credentialmanager/CredentialsContainer.cpp
(right):

https://codereview.chromium.org/2864493003/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/credentialmanager/CredentialsContainer.cpp:172:
// Set the default mediation option if none is provided.
I think it does more. Copying one to another.

https://codereview.chromium.org/2864493003/diff/120001/third_party/WebKit/Sou...
File third_party/WebKit/Source/platform/RuntimeEnabledFeatures.json5 (right):

https://codereview.chromium.org/2864493003/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/RuntimeEnabledFeatures.json5:190: name:
"CredentialRequestOptionsMediation",
Why is it needed?

[vasilii, 2017-05-18 17:00:27]

OPTIONAL is defined in minwindef.h(87)

[jdoerrie, 2017-05-19 08:59:30]

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

[commit-bot: I haz the power, 2017-05-19 08:59:49]

Dry run: CQ is trying da patch.

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

[jdoerrie, 2017-05-19 09:03:16]

jdoerrie@chromium.org changed reviewers:
+ isherman@chromium.org

[jdoerrie, 2017-05-19 09:03:18]

Gentle ping at mkwst@chromium.org to review //third_party/WebKit and security
relevant files:
components/password_manager/content/common/credential_manager.mojom
components/password_manager/content/common/credential_manager.typemap
components/password_manager/content/common/credential_manager_struct_traits.h
components/password_manager/content/common/credential_manager_struct_traits.cc

isherman@chromium.org:
Please review tools/metrics/histograms/histograms.xml

https://codereview.chromium.org/2864493003/diff/80001/components/password_man...
File
components/password_manager/content/browser/credential_manager_impl_unittest.cc
(right):

https://codereview.chromium.org/2864493003/diff/80001/components/password_man...
components/password_manager/content/browser/credential_manager_impl_unittest.cc:1633:
}
On 2017/05/18 16:16:12, vasilii wrote:
> On 2017/05/18 12:34:03, vasilii wrote:
> > There should be a new test for CredentialMediationRequirement::REQUIRED:
> > - Setup everything that auto sign-in can happen.
> > - Call Get(REQUIRED)
> > - Check that autosign in didn't happen but the account chooser was shown.
> 
> Not addressed.

Done.

https://codereview.chromium.org/2864493003/diff/120001/third_party/WebKit/Sou...
File
third_party/WebKit/Source/modules/credentialmanager/CredentialRequestOptions.idl
(right):

https://codereview.chromium.org/2864493003/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/credentialmanager/CredentialRequestOptions.idl:7:
[RuntimeEnabled=CredentialRequestOptionsMediation]
On 2017/05/18 16:16:12, vasilii wrote:
> What is the motivation for this?

I added this so that we are not blocked by the intent to ship. However, there
has been movement in the thread so this is no longer necessary (assuming jochen@
soon gives the LGTM3).

https://codereview.chromium.org/2864493003/diff/120001/third_party/WebKit/Sou...
File
third_party/WebKit/Source/modules/credentialmanager/CredentialsContainer.cpp
(right):

https://codereview.chromium.org/2864493003/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/credentialmanager/CredentialsContainer.cpp:172:
// Set the default mediation option if none is provided.
On 2017/05/18 16:16:12, vasilii wrote:
> I think it does more. Copying one to another.

Done.

https://codereview.chromium.org/2864493003/diff/120001/third_party/WebKit/Sou...
File third_party/WebKit/Source/platform/RuntimeEnabledFeatures.json5 (right):

https://codereview.chromium.org/2864493003/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/RuntimeEnabledFeatures.json5:190: name:
"CredentialRequestOptionsMediation",
On 2017/05/18 16:16:12, vasilii wrote:
> Why is it needed?

See above.

[Mike West, 2017-05-19 09:42:36]

I think the approach in //third_party/WebKit is fine, but I'd like to see the
conflict resolution change.

https://codereview.chromium.org/2864493003/diff/140001/third_party/WebKit/Sou...
File
third_party/WebKit/Source/modules/credentialmanager/CredentialsContainer.cpp
(right):

https://codereview.chromium.org/2864493003/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/credentialmanager/CredentialsContainer.cpp:177:
options.mediation() = "optional";
Nit: Assigning to `mediation()` doesn't seem like it's going to work.

https://codereview.chromium.org/2864493003/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/credentialmanager/CredentialsContainer.cpp:183:
"At most one of 'unmediated' and 'mediation' can be specified."));
I don't think this is what we agreed on. I thought we agreed that if `mediation`
is present, then we'd _ignore_ the `unmediated` boolean, which allows developers
to change their code from `{ ..., unmediated: true }` to `{ ..., unmediated:
true, mediation: "silent" }` today, and then forget about it for the rest of
their natural lives.

The current code will force them back to their editors in a few months, which
doesn't seem necessary or valuable.

[jdoerrie, 2017-05-19 09:59:51]

https://codereview.chromium.org/2864493003/diff/140001/third_party/WebKit/Sou...
File
third_party/WebKit/Source/modules/credentialmanager/CredentialsContainer.cpp
(right):

https://codereview.chromium.org/2864493003/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/credentialmanager/CredentialsContainer.cpp:177:
options.mediation() = "optional";
On 2017/05/19 09:42:35, Mike West wrote:
> Nit: Assigning to `mediation()` doesn't seem like it's going to work.

Yeah, this needs to be `setMediation`. It might be worthwhile to change the code
generation script to return const refs so this wouldn't compile.

https://codereview.chromium.org/2864493003/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/credentialmanager/CredentialsContainer.cpp:183:
"At most one of 'unmediated' and 'mediation' can be specified."));
On 2017/05/19 09:42:35, Mike West wrote:
> I don't think this is what we agreed on. I thought we agreed that if
`mediation`
> is present, then we'd _ignore_ the `unmediated` boolean, which allows
developers
> to change their code from `{ ..., unmediated: true }` to `{ ..., unmediated:
> true, mediation: "silent" }` today, and then forget about it for the rest of
> their natural lives.
> 
> The current code will force them back to their editors in a few months, which
> doesn't seem necessary or valuable.

Agreed, fair point. Should we just silently ignore unmediated in this case or
issue a warning when unmediated and mediation don't match (e.g. unmediated=true
but mediation="required")?

[commit-bot: I haz the power, 2017-05-19 10:20:53]

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

[commit-bot: I haz the power, 2017-05-19 10:20:54]

Dry run: Try jobs failed on following builders:
  win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)

[Mike West, 2017-05-19 11:09:51]

On 2017/05/19 at 09:59:51, jdoerrie wrote:
>
https://codereview.chromium.org/2864493003/diff/140001/third_party/WebKit/Sou...
> File
third_party/WebKit/Source/modules/credentialmanager/CredentialsContainer.cpp
(right):
> 
>
https://codereview.chromium.org/2864493003/diff/140001/third_party/WebKit/Sou...
>
third_party/WebKit/Source/modules/credentialmanager/CredentialsContainer.cpp:177:
options.mediation() = "optional";
> On 2017/05/19 09:42:35, Mike West wrote:
> > Nit: Assigning to `mediation()` doesn't seem like it's going to work.
> 
> Yeah, this needs to be `setMediation`. It might be worthwhile to change the
code generation script to return const refs so this wouldn't compile.

File a bug? :)

https://codereview.chromium.org/2864493003/diff/140001/third_party/WebKit/Sou...
>
third_party/WebKit/Source/modules/credentialmanager/CredentialsContainer.cpp:183:
"At most one of 'unmediated' and 'mediation' can be specified."));
> On 2017/05/19 09:42:35, Mike West wrote:
> > I don't think this is what we agreed on. I thought we agreed that if
`mediation`
> > is present, then we'd _ignore_ the `unmediated` boolean, which allows
developers
> > to change their code from `{ ..., unmediated: true }` to `{ ..., unmediated:
> > true, mediation: "silent" }` today, and then forget about it for the rest of
> > their natural lives.
> > 
> > The current code will force them back to their editors in a few months,
which
> > doesn't seem necessary or valuable.
> 
> Agreed, fair point. Should we just silently ignore unmediated in this case or
issue a warning when unmediated and mediation don't match (e.g. unmediated=true
but mediation="required")?

Sending a warning seems reasonable, but I don't think it's essential. I'm happy
to stamp it if you want to add something, but I wouldn't spend too much time on
it. :)

[jdoerrie, 2017-05-19 11:50:55]

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

[commit-bot: I haz the power, 2017-05-19 11:51:26]

Dry run: CQ is trying da patch.

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

[jdoerrie, 2017-05-19 11:53:26]

On 2017/05/19 11:09:51, Mike West wrote:
> On 2017/05/19 at 09:59:51, jdoerrie wrote:
> >
>
https://codereview.chromium.org/2864493003/diff/140001/third_party/WebKit/Sou...
> > File
> third_party/WebKit/Source/modules/credentialmanager/CredentialsContainer.cpp
> (right):
> > 
> >
>
https://codereview.chromium.org/2864493003/diff/140001/third_party/WebKit/Sou...
> >
>
third_party/WebKit/Source/modules/credentialmanager/CredentialsContainer.cpp:177:
> options.mediation() = "optional";
> > On 2017/05/19 09:42:35, Mike West wrote:
> > > Nit: Assigning to `mediation()` doesn't seem like it's going to work.
> > 
> > Yeah, this needs to be `setMediation`. It might be worthwhile to change the
> code generation script to return const refs so this wouldn't compile.
> 
> File a bug? :)

Sent a Fix :) http://crrev.com/2897603003

> 
>
https://codereview.chromium.org/2864493003/diff/140001/third_party/WebKit/Sou...
> >
>
third_party/WebKit/Source/modules/credentialmanager/CredentialsContainer.cpp:183:
> "At most one of 'unmediated' and 'mediation' can be specified."));
> > On 2017/05/19 09:42:35, Mike West wrote:
> > > I don't think this is what we agreed on. I thought we agreed that if
> `mediation`
> > > is present, then we'd _ignore_ the `unmediated` boolean, which allows
> developers
> > > to change their code from `{ ..., unmediated: true }` to `{ ...,
unmediated:
> > > true, mediation: "silent" }` today, and then forget about it for the rest
of
> > > their natural lives.
> > > 
> > > The current code will force them back to their editors in a few months,
> which
> > > doesn't seem necessary or valuable.
> > 
> > Agreed, fair point. Should we just silently ignore unmediated in this case
or
> issue a warning when unmediated and mediation don't match (e.g.
unmediated=true
> but mediation="required")?
> 
> Sending a warning seems reasonable, but I don't think it's essential. I'm
happy
> to stamp it if you want to add something, but I wouldn't spend too much time
on
> it. :)

Done.

[vasilii, 2017-05-19 12:05:09]

https://codereview.chromium.org/2864493003/diff/160001/components/password_ma...
File
components/password_manager/content/browser/credential_manager_impl_unittest.cc
(right):

https://codereview.chromium.org/2864493003/diff/160001/components/password_ma...
components/password_manager/content/browser/credential_manager_impl_unittest.cc:1506:
store_->AddLogin(form_);
form_.skip_zero_click = true;

https://codereview.chromium.org/2864493003/diff/160001/components/password_ma...
components/password_manager/content/browser/credential_manager_impl_unittest.cc:1517:
EXPECT_CALL(*client_, NotifyUserAutoSigninPtr()).Times(testing::Exactly(0));
Put expectation before Get().

[jdoerrie, 2017-05-19 14:39:23]

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

[commit-bot: I haz the power, 2017-05-19 14:39:50]

Dry run: CQ is trying da patch.

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

[jdoerrie, 2017-05-19 14:40:21]

https://codereview.chromium.org/2864493003/diff/160001/components/password_ma...
File
components/password_manager/content/browser/credential_manager_impl_unittest.cc
(right):

https://codereview.chromium.org/2864493003/diff/160001/components/password_ma...
components/password_manager/content/browser/credential_manager_impl_unittest.cc:1506:
store_->AddLogin(form_);
On 2017/05/19 12:05:08, vasilii wrote:
> form_.skip_zero_click = true;

Done.

https://codereview.chromium.org/2864493003/diff/160001/components/password_ma...
components/password_manager/content/browser/credential_manager_impl_unittest.cc:1517:
EXPECT_CALL(*client_, NotifyUserAutoSigninPtr()).Times(testing::Exactly(0));
On 2017/05/19 12:05:08, vasilii wrote:
> Put expectation before Get().

Done.

[Mike West, 2017-05-19 15:29:57]

Thank you for sending in a fix for the bindings issue! That's great.

Nits and test requests below. Otherwise good to go.

https://codereview.chromium.org/2864493003/diff/180001/third_party/WebKit/Sou...
File
third_party/WebKit/Source/modules/credentialmanager/CredentialRequestOptions.idl
(right):

https://codereview.chromium.org/2864493003/diff/180001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/credentialmanager/CredentialRequestOptions.idl:25:
// TODO(http://crbug.com/715077): Set the default here when 'unmediated' is
Nit: Whitespace alignment.

https://codereview.chromium.org/2864493003/diff/180001/third_party/WebKit/Sou...
File
third_party/WebKit/Source/modules/credentialmanager/CredentialsContainer.cpp
(right):

https://codereview.chromium.org/2864493003/diff/180001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/credentialmanager/CredentialsContainer.cpp:183:
mediation = options.mediation();
Please add a test for invalid values.

https://codereview.chromium.org/2864493003/diff/180001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/credentialmanager/CredentialsContainer.cpp:190:
(options.unmediated() ? "true" : "false") + "."));
Please add a test for this behavior.

https://codereview.chromium.org/2864493003/diff/180001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/credentialmanager/CredentialsContainer.cpp:218:
UseCounter::Count(context, feature);
Optional nit: I think it's simpler to just call `Count()` in the if blocks
above.

[jdoerrie, 2017-05-19 16:20:20]

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

[commit-bot: I haz the power, 2017-05-19 16:21:14]

Dry run: CQ is trying da patch.

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

[jdoerrie, 2017-05-19 16:22:37]

Thanks, Mike. Sorry for not letting you review the bindings fix in time, my
finger was too quick :/

https://codereview.chromium.org/2864493003/diff/180001/third_party/WebKit/Sou...
File
third_party/WebKit/Source/modules/credentialmanager/CredentialRequestOptions.idl
(right):

https://codereview.chromium.org/2864493003/diff/180001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/credentialmanager/CredentialRequestOptions.idl:25:
// TODO(http://crbug.com/715077): Set the default here when 'unmediated' is
On 2017/05/19 15:29:56, Mike West wrote:
> Nit: Whitespace alignment.

Done.

https://codereview.chromium.org/2864493003/diff/180001/third_party/WebKit/Sou...
File
third_party/WebKit/Source/modules/credentialmanager/CredentialsContainer.cpp
(right):

https://codereview.chromium.org/2864493003/diff/180001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/credentialmanager/CredentialsContainer.cpp:183:
mediation = options.mediation();
On 2017/05/19 15:29:56, Mike West wrote:
> Please add a test for invalid values.

That already exists in credentialscontainer-get-errors.html, no?
Or do you mean a test for something else?

https://codereview.chromium.org/2864493003/diff/180001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/credentialmanager/CredentialsContainer.cpp:190:
(options.unmediated() ? "true" : "false") + "."));
On 2017/05/19 15:29:57, Mike West wrote:
> Please add a test for this behavior.

Done, please see 
credentialscontainer-get-warnings.html and
credentialscontainer-get-warnings-expected.txt

https://codereview.chromium.org/2864493003/diff/180001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/credentialmanager/CredentialsContainer.cpp:218:
UseCounter::Count(context, feature);
On 2017/05/19 15:29:57, Mike West wrote:
> Optional nit: I think it's simpler to just call `Count()` in the if blocks
> above.

Done.

[vasilii, 2017-05-19 16:26:37]

//components/password_manager LGTM

[commit-bot: I haz the power, 2017-05-19 18:21:26]

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

[commit-bot: I haz the power, 2017-05-19 18:21:27]

Dry run: This issue passed the CQ dry run.

[Ilya Sherman, 2017-05-19 21:12:52]

Metrics LGTM, thanks.

[Mike West, 2017-05-20 20:50:12]

LGTM, thanks

[jdoerrie, 2017-05-22 06:49:49]

The CQ bit was checked by jdoerrie@chromium.org

[jdoerrie, 2017-05-22 06:49:50]

The patchset sent to the CQ was uploaded after l-g-t-m from mkwst@chromium.org,
isherman@chromium.org, vasilii@chromium.org
Link to the patchset: https://codereview.chromium.org/2864493003/#ps220001
(title: "Rebase")

[commit-bot: I haz the power, 2017-05-22 06:50:01]

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-05-22 08:21:14]

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

[commit-bot: I haz the power, 2017-05-22 08:21:16]

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...)

[jdoerrie, 2017-05-22 08:50:34]

The CQ bit was checked by jdoerrie@chromium.org

[commit-bot: I haz the power, 2017-05-22 08:50:53]

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-05-22 09:37:45]

CQ is committing da patch.

Bot data: {"patchset_id": 220001, "attempt_start_ts": 1495443034650220,
"parent_rev": "cab2062a0e9c0112df4eb9332c2cc363c32ad838", "commit_rev":
"46feffcdbeca2da9245a3c48ff6d81d08f26d5d8"}

[commit-bot: I haz the power, 2017-05-22 09:38:03]

Description was changed from

==========
Deprecate CredentialRequestOptions.unmediated in favor mediation enum

This change introduces an `CredentialMediationRequirement` enum argument to
`CredentialsContainer::get()` that is intended to replace the currently existing
boolean flag `unmediated`. The enum will have three states ("silent",
"optional", "required") with "silent" and "optional" directly mapping to the
existing boolean states, "required" is a novel option.

Intent to Ship:
https://groups.google.com/a/chromium.org/d/msg/blink-dev/veCKBk1NguM/ctyHrdVW...

BUG=721399
==========

to

==========
Deprecate CredentialRequestOptions.unmediated in favor mediation enum

This change introduces an `CredentialMediationRequirement` enum argument to
`CredentialsContainer::get()` that is intended to replace the currently existing
boolean flag `unmediated`. The enum will have three states ("silent",
"optional", "required") with "silent" and "optional" directly mapping to the
existing boolean states, "required" is a novel option.

Intent to Ship:
https://groups.google.com/a/chromium.org/d/msg/blink-dev/veCKBk1NguM/ctyHrdVW...

BUG=721399

Review-Url: https://codereview.chromium.org/2864493003
Cr-Commit-Position: refs/heads/master@{#473532}
Committed:
https://chromium.googlesource.com/chromium/src/+/46feffcdbeca2da9245a3c48ff6d...
==========

[commit-bot: I haz the power, 2017-05-22 09:38:04]

Committed patchset #12 (id:220001) as
https://chromium.googlesource.com/chromium/src/+/46feffcdbeca2da9245a3c48ff6d...