Credential Manager: Wire up the client.

Credential Manager: Wire up the client.

Now that we've added a mock CredentialManagerClient to content shell, we
can wire up the various CredentialsContainer methods to the client
provided by the embedder.

BUG=400674
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=182052

[Mike West, 2014-09-15 07:08:22]

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

[Mike West, 2014-09-15 07:08:23]

Mind taking a look, tkent? This patch depends upon the mock added in
https://codereview.chromium.org/565943002/, so I won't throw it to the bots
until that lands.

-mike

[Mike West, 2014-09-15 07:13:20]

mkwst@chromium.org changed reviewers:
+ philipj@opera.com, sigbjornf@opera.com
- tkent@chromium.org

[Mike West, 2014-09-15 07:15:48]

Dropping tkent@ for the moment in the hopes that philipj@ or sigbjornf@ can
review this patch before I ask for a platform/ stamp.

WDYT?

[sof, 2014-09-15 07:57:17]

https://codereview.chromium.org/573663002/diff/20001/Source/modules/credentia...
File Source/modules/credentialmanager/CredentialManagerClient.cpp (right):

https://codereview.chromium.org/573663002/diff/20001/Source/modules/credentia...
Source/modules/credentialmanager/CredentialManagerClient.cpp:35: return
from(toDocument(scriptState->executionContext())->page());
If the document has become detached, page() will be 0 -- add a null check?

https://codereview.chromium.org/573663002/diff/20001/Source/modules/credentia...
File Source/modules/credentialmanager/CredentialManagerClient.h (right):

https://codereview.chromium.org/573663002/diff/20001/Source/modules/credentia...
Source/modules/credentialmanager/CredentialManagerClient.h:30: static
CredentialManagerClient* from(ScriptState*);
I think I would prefer it if we kept this free of ScriptState*, using
ExecutionContext* instead (same goes for canAccessCredentialManagerAPI())

https://codereview.chromium.org/573663002/diff/20001/Source/modules/credentia...
File Source/modules/credentialmanager/CredentialsContainer.cpp (right):

https://codereview.chromium.org/573663002/diff/20001/Source/modules/credentia...
Source/modules/credentialmanager/CredentialsContainer.cpp:126:
CredentialManagerClient* client = CredentialManagerClient::from(scriptState);
There's a fair bit of duplicated code in these methods & notifications; possible
to reduce?

https://codereview.chromium.org/573663002/diff/20001/Source/modules/credentia...
File Source/modules/credentialmanager/LocalCredential.h (right):

https://codereview.chromium.org/573663002/diff/20001/Source/modules/credentia...
Source/modules/credentialmanager/LocalCredential.h:19: static LocalCredential*
create(const String& id, const String& name, const KURL& avatar, const String&
password);
Wouldn't this ExceptionState-less version be easier to use as create(const
WebCredential&) ?

[tkent, 2014-09-16 00:02:17]

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

[tkent, 2014-09-16 00:02:19]

basically lgtm.  Please address sof's comments.

https://codereview.chromium.org/573663002/diff/20001/public/platform/WebCrede...
File public/platform/WebCredential.h (right):

https://codereview.chromium.org/573663002/diff/20001/public/platform/WebCrede...
public/platform/WebCredential.h:31: BLINK_PLATFORM_EXPORT WebCredential&
operator=(PlatformCredential*);
nit:
|explicit WebCredential()| above and this operator= look inconsistent.
We need an explicit constructor to create WebCredential with PlatformCredential.
    WebCredential web = WebCredential(platform);
But we don't need explicit conversion to assign.
    web = platform;

Is this intentional?

[Mike West, 2014-09-16 07:53:20]

tkent: Thanks! I've dropped the cast operator, in favor of an explicit getter
(implicit casts worry me). I hope that's a reasonable choice.

sof: Mind taking another look? I've reworked the bits you suggested.

https://codereview.chromium.org/573663002/diff/20001/Source/modules/credentia...
File Source/modules/credentialmanager/CredentialManagerClient.cpp (right):

https://codereview.chromium.org/573663002/diff/20001/Source/modules/credentia...
Source/modules/credentialmanager/CredentialManagerClient.cpp:35: return
from(toDocument(scriptState->executionContext())->page());
On 2014/09/15 07:57:17, sof wrote:
> If the document has become detached, page() will be 0 -- add a null check?

That's taken care of in `Supplementable::from()`: it will return 0 rather than a
CredentialManagerClient.

https://codereview.chromium.org/573663002/diff/20001/Source/modules/credentia...
File Source/modules/credentialmanager/CredentialManagerClient.h (right):

https://codereview.chromium.org/573663002/diff/20001/Source/modules/credentia...
Source/modules/credentialmanager/CredentialManagerClient.h:30: static
CredentialManagerClient* from(ScriptState*);
On 2014/09/15 07:57:17, sof wrote:
> I think I would prefer it if we kept this free of ScriptState*, using
> ExecutionContext* instead (same goes for canAccessCredentialManagerAPI())

How about we compromise, and keep ScriptState out of public APIs? Since
`canAccessCredentialManagerAPI` is an internal implementation detail of
CredentialsContainer, changing it to use ExecutionContext* would just mean that
I call the `executionContext()` getter in more places. I'm not sure that gains
us any clarity.

https://codereview.chromium.org/573663002/diff/20001/Source/modules/credentia...
Source/modules/credentialmanager/CredentialManagerClient.h:30: static
CredentialManagerClient* from(ScriptState*);
Eh. Ended up rewriting most of this anyway. :)

https://codereview.chromium.org/573663002/diff/20001/Source/modules/credentia...
File Source/modules/credentialmanager/LocalCredential.h (right):

https://codereview.chromium.org/573663002/diff/20001/Source/modules/credentia...
Source/modules/credentialmanager/LocalCredential.h:19: static LocalCredential*
create(const String& id, const String& name, const KURL& avatar, const String&
password);
On 2014/09/15 07:57:17, sof wrote:
> Wouldn't this ExceptionState-less version be easier to use as create(const
> WebCredential&) ?

Hrm. Maybe!

[sof, 2014-09-16 12:36:11]

lgtm

https://codereview.chromium.org/573663002/diff/20001/Source/modules/credentia...
File Source/modules/credentialmanager/CredentialManagerClient.cpp (right):

https://codereview.chromium.org/573663002/diff/20001/Source/modules/credentia...
Source/modules/credentialmanager/CredentialManagerClient.cpp:35: return
from(toDocument(scriptState->executionContext())->page());
On 2014/09/16 07:53:20, Mike West wrote:
> On 2014/09/15 07:57:17, sof wrote:
> > If the document has become detached, page() will be 0 -- add a null check?
> 
> That's taken care of in `Supplementable::from()`: it will return 0 rather than
a
> CredentialManagerClient.

It does (but a bit tight); I won't insist.

https://codereview.chromium.org/573663002/diff/20001/Source/modules/credentia...
File Source/modules/credentialmanager/CredentialManagerClient.h (right):

https://codereview.chromium.org/573663002/diff/20001/Source/modules/credentia...
Source/modules/credentialmanager/CredentialManagerClient.h:30: static
CredentialManagerClient* from(ScriptState*);
On 2014/09/16 07:53:20, Mike West wrote:
> On 2014/09/15 07:57:17, sof wrote:
> > I think I would prefer it if we kept this free of ScriptState*, using
> > ExecutionContext* instead (same goes for canAccessCredentialManagerAPI())
> 
> How about we compromise, and keep ScriptState out of public APIs? Since
> `canAccessCredentialManagerAPI` is an internal implementation detail of
> CredentialsContainer, changing it to use ExecutionContext* would just mean
that
> I call the `executionContext()` getter in more places. I'm not sure that gains
> us any clarity.

thanks, that works for me

[Mike West, 2014-09-16 12:53:06]

The CQ bit was checked by mkwst@chromium.org

[commit-bot: I haz the power, 2014-09-16 12:54:15]

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

[commit-bot: I haz the power, 2014-09-16 14:49:06]

Committed patchset #4 (id:60001) as 182052