Credential Manager: Create a local or federated credential to hand to Blink.

Credential Manager: Create a local or federated credential to hand to Blink.

BUG=400674
Committed: https://crrev.com/d55ad56342ec5928ee315351f542f147a01beb89
Cr-Commit-Position: refs/heads/master@{#297805}

[Mike West, 2014-10-01 14:38:46]

mkwst@chromium.org changed reviewers:
+ vabr@chromium.org

[Mike West, 2014-10-01 14:38:47]

One more on the pile. WDYT?

I need to land a blink-side change before I can test the difference between
WebLocalCredentials and WebFederatedCredentials when they're both passed as
plain ol' WebCredential objects.

-mike

[Mike West, 2014-10-01 14:39:34]

On 2014/10/01 14:38:47, Mike West wrote:
> One more on the pile. WDYT?
> 
> I need to land a blink-side change before I can test the difference between
> WebLocalCredentials and WebFederatedCredentials when they're both passed as
> plain ol' WebCredential objects.

Though such a test is pointless at the moment, as federated credentials are a
big TODO on the Chromium side. :) We only generate local credentials.

[vabr (Chromium), 2014-10-01 16:05:23]

LGTM with comments.
Cheers,
Vaclav

https://codereview.chromium.org/621503005/diff/1/components/password_manager/...
File components/password_manager/content/renderer/credential_manager_client.cc
(right):

https://codereview.chromium.org/621503005/diff/1/components/password_manager/...
components/password_manager/content/renderer/credential_manager_client.cc:86:
credential.reset(new blink::WebFederatedCredential(info.id, info.name,
nit: Is this how clang-format would format the line? If not, could you please
prefer clang-format (git cl format), to make future refactoring easier?
Alternatively, there are deactivation comments
// clang-format off
special_formatting();
// clang-format on
(see b/14285621) for places where the clang-format is really a bad choice.

https://codereview.chromium.org/621503005/diff/1/components/password_manager/...
components/password_manager/content/renderer/credential_manager_client.cc:93:
default:
Please change the "default:" to
case CREDENTIAL_TYPE_UNKNOWN:

This way the compiler will yell at anybody adding a new type without adding
support for it as well.

https://codereview.chromium.org/621503005/diff/1/components/password_manager/...
components/password_manager/content/renderer/credential_manager_client.cc:97:
DCHECK(credential);
nit: This DCHECK seems a bit pointless, because the only way to reach it is by
exhausting memory, when "new" returns NULL.
But if you want to keep it for documentation purposes, I'm not too strongly
against it.

[Mike West, 2014-10-01 18:22:20]

https://codereview.chromium.org/621503005/diff/1/components/password_manager/...
File components/password_manager/content/renderer/credential_manager_client.cc
(right):

https://codereview.chromium.org/621503005/diff/1/components/password_manager/...
components/password_manager/content/renderer/credential_manager_client.cc:86:
credential.reset(new blink::WebFederatedCredential(info.id, info.name,
On 2014/10/01 16:05:23, vabr (Chromium) wrote:
> nit: Is this how clang-format would format the line? If not, could you please
> prefer clang-format (git cl format), to make future refactoring easier?
> Alternatively, there are deactivation comments
> // clang-format off
> special_formatting();
> // clang-format on
> (see b/14285621) for places where the clang-format is really a bad choice.

Re-clang-formatted. Not sure what happened in the first patch.

https://codereview.chromium.org/621503005/diff/1/components/password_manager/...
components/password_manager/content/renderer/credential_manager_client.cc:93:
default:
On 2014/10/01 16:05:22, vabr (Chromium) wrote:
> Please change the "default:" to
> case CREDENTIAL_TYPE_UNKNOWN:
> 
> This way the compiler will yell at anybody adding a new type without adding
> support for it as well.

Done.

https://codereview.chromium.org/621503005/diff/1/components/password_manager/...
components/password_manager/content/renderer/credential_manager_client.cc:97:
DCHECK(credential);
On 2014/10/01 16:05:22, vabr (Chromium) wrote:
> nit: This DCHECK seems a bit pointless, because the only way to reach it is by
> exhausting memory, when "new" returns NULL.
> But if you want to keep it for documentation purposes, I'm not too strongly
> against it.

Done.

[Mike West, 2014-10-01 18:22:24]

The CQ bit was checked by mkwst@chromium.org

[commit-bot: I haz the power, 2014-10-01 18:23:29]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/621503005/20001

[commit-bot: I haz the power, 2014-10-01 19:28:54]

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

[commit-bot: I haz the power, 2014-10-01 19:28:56]

Try jobs failed on following builders:
  linux_chromium_rel_swarming on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[Mike West, 2014-10-01 19:32:51]

The CQ bit was checked by mkwst@chromium.org

[commit-bot: I haz the power, 2014-10-01 19:33:58]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/621503005/40001

[commit-bot: I haz the power, 2014-10-01 20:35:31]

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

[commit-bot: I haz the power, 2014-10-01 20:35:32]

Try jobs failed on following builders:
  linux_chromium_rel_swarming on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  mac_chromium_rel_swarming on tryserver.chromium.mac
(http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
  win_chromium_rel_swarming on tryserver.chromium.win
(http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

[Mike West, 2014-10-02 06:43:33]

The CQ bit was checked by mkwst@chromium.org

[commit-bot: I haz the power, 2014-10-02 06:44:19]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/621503005/60001

[commit-bot: I haz the power, 2014-10-02 06:52:06]

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

[commit-bot: I haz the power, 2014-10-02 06:52:07]

Try jobs failed on following builders:
  linux_chromium_gn_rel on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  win_chromium_rel_swarming on tryserver.chromium.win
(http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

[Mike West, 2014-10-02 06:56:20]

The CQ bit was checked by mkwst@chromium.org

[commit-bot: I haz the power, 2014-10-02 06:57:23]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/621503005/60001

[commit-bot: I haz the power, 2014-10-02 07:03:48]

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

[commit-bot: I haz the power, 2014-10-02 07:03:49]

Try jobs failed on following builders:
  linux_chromium_chromeos_rel_swarming on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_gn_dbg on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_rel_swarming on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[Mike West, 2014-10-02 08:29:09]

The CQ bit was checked by mkwst@chromium.org

[Mike West, 2014-10-02 08:29:41]

The CQ bit was unchecked by mkwst@chromium.org

[Mike West, 2014-10-02 08:30:11]

Patchset #5 (id:80001) has been deleted

[Mike West, 2014-10-02 08:30:23]

The CQ bit was checked by mkwst@chromium.org

[commit-bot: I haz the power, 2014-10-02 08:31:12]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/621503005/60001

[commit-bot: I haz the power, 2014-10-02 08:38:37]

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

[commit-bot: I haz the power, 2014-10-02 08:38:38]

Try jobs failed on following builders:
  android_chromium_gn_compile_dbg on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/android_chromiu...)

[Mike West, 2014-10-02 09:01:19]

The CQ bit was checked by mkwst@chromium.org

[commit-bot: I haz the power, 2014-10-02 09:01:43]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/621503005/60001

[commit-bot: I haz the power, 2014-10-02 09:13:56]

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

[commit-bot: I haz the power, 2014-10-02 09:14:00]

Try jobs failed on following builders:
  linux_chromium_chromeos_rel_swarming on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[Mike West, 2014-10-02 10:52:11]

The CQ bit was checked by mkwst@chromium.org

[commit-bot: I haz the power, 2014-10-02 10:53:09]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/621503005/60001

[commit-bot: I haz the power, 2014-10-02 11:36:06]

Committed patchset #4 (id:60001) as 746fabc32ec69a1a52fa9a9260f11a569beb5caa

[commit-bot: I haz the power, 2014-10-02 11:36:54]

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