Credential manager: Introduce platform interface.

Credential manager: Introduce platform interface.

This patch adds a WebCredentialManager interface that the credential
manager module will (eventually) use to call up into its embedder in
response to JavaScript API calls.

These methods don't currently do anything, and aren't actually called;
until the Chromium-side work is done, there's no reason to wire them up
to the JavaScript bindings.

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

[Mike West, 2014-08-11 08:13:19]

tkent@: Would you mind taking a look at the platform interface this patch adds?

yhirano@: Would you mind taking a look at the Promise/WebCallbacks bits?

It depends on https://codereview.chromium.org/457683002/ which is in the CQ, so
I can't throw it to the bots quite yet.

Thanks!

[tkent, 2014-08-11 08:43:50]

https://codereview.chromium.org/456293002/diff/1/Source/modules/credentialman...
File Source/modules/credentialmanager/CredentialsContainer.cpp (right):

https://codereview.chromium.org/456293002/diff/1/Source/modules/credentialman...
Source/modules/credentialmanager/CredentialsContainer.cpp:32: break;
Will the break-then-ASSERT_NO_REACHED work well?

https://codereview.chromium.org/456293002/diff/1/Source/modules/credentialman...
Source/modules/credentialmanager/CredentialsContainer.cpp:33: };
; is unnecessary.

https://codereview.chromium.org/456293002/diff/1/public/platform/WebCredentia...
File public/platform/WebCredentialManager.h (right):

https://codereview.chromium.org/456293002/diff/1/public/platform/WebCredentia...
public/platform/WebCredentialManager.h:26: void dispatchFailedSignIn(const
WebCredential&, NotificationCallbacks*) { }
Should this be virtual?

https://codereview.chromium.org/456293002/diff/1/public/platform/WebCredentia...
File public/platform/WebCredentialManagerError.h (right):

https://codereview.chromium.org/456293002/diff/1/public/platform/WebCredentia...
public/platform/WebCredentialManagerError.h:8: #include "WebString.h"
should be "public/platform/WebString.h"

[yhirano, 2014-08-11 08:58:48]

https://codereview.chromium.org/456293002/diff/1/Source/modules/credentialman...
File Source/modules/credentialmanager/CredentialsContainer.cpp (right):

https://codereview.chromium.org/456293002/diff/1/Source/modules/credentialman...
Source/modules/credentialmanager/CredentialsContainer.cpp:38: class
NotificationCallbacks : public WebCredentialManager::NotificationCallbacks {
How about enclosing this class with an unnamed namespace?

https://codereview.chromium.org/456293002/diff/1/Source/modules/credentialman...
Source/modules/credentialmanager/CredentialsContainer.cpp:49: virtual void
onError(WebCredentialManagerError* reason) OVERRIDE
IIRC WebCallbacks handler parameter ownership is transferred to the callee (e.g.
[1]). Shouldn't you release |reason| here?

https://code.google.com/p/chromium/codesearch#chromium/src/content/renderer/p...

https://codereview.chromium.org/456293002/diff/1/Source/modules/credentialman...
Source/modules/credentialmanager/CredentialsContainer.cpp:58: class
RequestCallbacks : public WebCredentialManager::RequestCallbacks {
ditto

https://codereview.chromium.org/456293002/diff/1/Source/modules/credentialman...
Source/modules/credentialmanager/CredentialsContainer.cpp:64: virtual void
onSuccess(WebCredential* credential) OVERRIDE
ditto

https://codereview.chromium.org/456293002/diff/1/Source/modules/credentialman...
Source/modules/credentialmanager/CredentialsContainer.cpp:69: virtual void
onError(WebCredentialManagerError* reason) OVERRIDE
ditto

[Mike West, 2014-08-11 09:13:41]

Thanks to you both.

https://codereview.chromium.org/456293002/diff/1/Source/modules/credentialman...
File Source/modules/credentialmanager/CredentialsContainer.cpp (right):

https://codereview.chromium.org/456293002/diff/1/Source/modules/credentialman...
Source/modules/credentialmanager/CredentialsContainer.cpp:32: break;
On 2014/08/11 08:43:50, tkent wrote:
> Will the break-then-ASSERT_NO_REACHED work well?

You're right, this is still prototypey-enough that it's not entirely clear what
kind of errors we'll need to support yet. I'll switch it to `default:` for the
moment, and move back to the break/assert mode once this code solidified.

https://codereview.chromium.org/456293002/diff/1/Source/modules/credentialman...
Source/modules/credentialmanager/CredentialsContainer.cpp:33: };
On 2014/08/11 08:43:50, tkent wrote:
> ; is unnecessary.

Done.

https://codereview.chromium.org/456293002/diff/1/Source/modules/credentialman...
Source/modules/credentialmanager/CredentialsContainer.cpp:49: virtual void
onError(WebCredentialManagerError* reason) OVERRIDE
On 2014/08/11 08:58:48, yhirano wrote:
> IIRC WebCallbacks handler parameter ownership is transferred to the callee
(e.g.
> [1]). Shouldn't you release |reason| here?
> 
>
https://code.google.com/p/chromium/codesearch#chromium/src/content/renderer/p...

Hrm. My impression was that CallbackPromiseAdaptor had this relationship, but it
wasn't clear from the code whether WebCallbacks directly took ownership. Similar
code in ServiceWorker doesn't delete, for instance:
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...

https://codereview.chromium.org/456293002/diff/1/public/platform/WebCredentia...
File public/platform/WebCredentialManager.h (right):

https://codereview.chromium.org/456293002/diff/1/public/platform/WebCredentia...
public/platform/WebCredentialManager.h:26: void dispatchFailedSignIn(const
WebCredential&, NotificationCallbacks*) { }
On 2014/08/11 08:43:50, tkent wrote:
> Should this be virtual?

Yes. They all should, thanks!

https://codereview.chromium.org/456293002/diff/1/public/platform/WebCredentia...
File public/platform/WebCredentialManagerError.h (right):

https://codereview.chromium.org/456293002/diff/1/public/platform/WebCredentia...
public/platform/WebCredentialManagerError.h:8: #include "WebString.h"
On 2014/08/11 08:43:50, tkent wrote:
> should be "public/platform/WebString.h"

Done.

[tkent, 2014-08-12 00:30:14]

lgtm

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

https://codereview.chromium.org/456293002/diff/20001/public/platform/WebCrede...
public/platform/WebCredentialManagerError.h:14: DisabledErrorType = 0,
The items should be ErrorTypeDisabled, ErrorTypeUnknown, and ErrorTypeLast.
http://www.chromium.org/blink/public-c-api#TOC-Naming

This rule is not so helpful for enums in classes/structs. However consistency is
important.

[yhirano, 2014-08-12 06:55:29]

https://codereview.chromium.org/456293002/diff/1/Source/modules/credentialman...
File Source/modules/credentialmanager/CredentialsContainer.cpp (right):

https://codereview.chromium.org/456293002/diff/1/Source/modules/credentialman...
Source/modules/credentialmanager/CredentialsContainer.cpp:49: virtual void
onError(WebCredentialManagerError* reason) OVERRIDE
On 2014/08/11 09:13:40, Mike West wrote:
> On 2014/08/11 08:58:48, yhirano wrote:
> > IIRC WebCallbacks handler parameter ownership is transferred to the callee
> (e.g.
> > [1]). Shouldn't you release |reason| here?
> > 
> >
>
https://code.google.com/p/chromium/codesearch#chromium/src/content/renderer/p...
> 
> Hrm. My impression was that CallbackPromiseAdaptor had this relationship, but
it
> wasn't clear from the code whether WebCallbacks directly took ownership.
Similar
> code in ServiceWorker doesn't delete, for instance:
>
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...
> 

OK, I see.

[Mike West, 2014-08-12 07:44:21]

The CQ bit was checked by mkwst@chromium.org

[commit-bot: I haz the power, 2014-08-12 07:44:41]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/mkwst@chromium.org/456293002/40001

[commit-bot: I haz the power, 2014-08-12 07:48:02]

Change committed as 180040