Credential Manager: Add platform-exposed Credential objects.

Credential Manager: Add platform-exposed Credential objects.

We're going to need to pass credentials up into Chromium, and back down
into Blink. This patch makes that possible by adding a shared
"Platform*Credential" type which can be used by both the Blink module's
"*Credential" objects, and the platform's "Platform*Credential" objects.

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

[Mike West, 2014-08-08 14:11:41]

Sigbjorn, since you're apparently not on vacation anymore, can you take a look
at this patch? I've modeled it on the speech APIs that I think you were
responsible for oilpanning. I'd appreciate it if you could take a look at this
to ensure it's sane. :)

dglazkov@: Can you stamp the public/platform and Source/platform additions?

Thanks!

[dglazkov, 2014-08-08 15:30:54]

lgtm

[Mike West, 2014-08-09 12:17:40]

Widening to oilpan-reviews. Would you specifically take a look at the
static_cast bits in Source/modules/*Credential.cpp and Source/platform
versions/exported/Web*Credential.cpp? It seems like I'm doing the right thing,
but I'd appreciate confirmation/suggestions. :)

[sof, 2014-08-10 05:54:57]

lgtm, some details below worth tidying up first.

https://codereview.chromium.org/457683002/diff/20001/Source/modules/credentia...
File Source/modules/credentialmanager/Credential.h (left):

https://codereview.chromium.org/457683002/diff/20001/Source/modules/credentia...
Source/modules/credentialmanager/Credential.h:14: class Credential : public
GarbageCollectedFinalized<Credential>, public ScriptWrappable {
This base class no longer needs to be finalized (which transitively holds for
its hierarchy), so this can be just GarbageCollected<Credential> -- the
finalization of the Strings now happening via the platform Member objects
instead.

The dtor decl+defn also needs to be removed (here and for the derived classes.)

If you feel like the classes haven't stabilized and you may well end up adding
fields that do require finalization, this change is clearly not worth doing, at
least not now.

(The clang plugin/extension supporting Oilpan will catch and report classes that
grow fields that do require finalization, so this won't silently break.)

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

https://codereview.chromium.org/457683002/diff/20001/Source/modules/credentia...
Source/modules/credentialmanager/Credential.h:30:
Credential(PlatformCredential*);
Perhaps you want an implicit conversion, but if not add 'explicit'?

https://codereview.chromium.org/457683002/diff/20001/Source/modules/credentia...
Source/modules/credentialmanager/Credential.h:33: Member<PlatformCredential>
m_private;
Possible to come up with a better name?

https://codereview.chromium.org/457683002/diff/20001/Source/platform/credenti...
File Source/platform/credentialmanager/PlatformCredential.h (right):

https://codereview.chromium.org/457683002/diff/20001/Source/platform/credenti...
Source/platform/credentialmanager/PlatformCredential.h:25: virtual void
trace(Visitor*) { };
nit: stray semi.

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

https://codereview.chromium.org/457683002/diff/20001/public/platform/WebCrede...
public/platform/WebCredential.h:13: class PlatformCredential;
nit: move this into the namespace block below?

https://codereview.chromium.org/457683002/diff/20001/public/platform/WebFeder...
File public/platform/WebFederatedCredential.h (right):

https://codereview.chromium.org/457683002/diff/20001/public/platform/WebFeder...
public/platform/WebFederatedCredential.h:10: #include "WebPrivatePtr.h"
Redundant #include.

https://codereview.chromium.org/457683002/diff/20001/public/platform/WebLocal...
File public/platform/WebLocalCredential.h (right):

https://codereview.chromium.org/457683002/diff/20001/public/platform/WebLocal...
public/platform/WebLocalCredential.h:10: #include "WebPrivatePtr.h"
Redundant #include.

[haraken, 2014-08-10 08:19:41]

LGTM from the oilpan perspective.

[Mike West, 2014-08-10 11:39:53]

Thanks to you both!

https://codereview.chromium.org/457683002/diff/20001/Source/modules/credentia...
File Source/modules/credentialmanager/Credential.h (left):

https://codereview.chromium.org/457683002/diff/20001/Source/modules/credentia...
Source/modules/credentialmanager/Credential.h:14: class Credential : public
GarbageCollectedFinalized<Credential>, public ScriptWrappable {
On 2014/08/10 05:54:57, sof wrote:
> This base class no longer needs to be finalized (which transitively holds for
> its hierarchy), so this can be just GarbageCollected<Credential> -- the
> finalization of the Strings now happening via the platform Member objects
> instead.

In the future, I don't expect Credential to change much, but I do expect
subclasses to sprout nontrivial members (LocalCredential will almost certainly
grow a FormData at some point). What's the right way to model that from an
oilpan perspective?

[sof, 2014-08-10 13:14:15]

https://codereview.chromium.org/457683002/diff/20001/Source/modules/credentia...
File Source/modules/credentialmanager/Credential.h (left):

https://codereview.chromium.org/457683002/diff/20001/Source/modules/credentia...
Source/modules/credentialmanager/Credential.h:14: class Credential : public
GarbageCollectedFinalized<Credential>, public ScriptWrappable {
On 2014/08/10 11:39:52, Mike West wrote:
> On 2014/08/10 05:54:57, sof wrote:
> > This base class no longer needs to be finalized (which transitively holds
for
> > its hierarchy), so this can be just GarbageCollected<Credential> -- the
> > finalization of the Strings now happening via the platform Member objects
> > instead.
> 
> In the future, I don't expect Credential to change much, but I do expect
> subclasses to sprout nontrivial members (LocalCredential will almost certainly
> grow a FormData at some point). What's the right way to model that from an
> oilpan perspective?

Assuming FormData isn't turned into a GCed object by then (as part of moving
Resource-related objects to the heap), the Credential base class will have to
become *Finalized again.

For now, add a "FIXME: Oilpan:" comment about considering the use of the
non-finalized GarbageCollected<> base class once this module has settled down a
bit?

[Mike West, 2014-08-11 07:09:19]

The CQ bit was checked by mkwst@chromium.org

[commit-bot: I haz the power, 2014-08-11 07:10:55]

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

[Mike West, 2014-08-11 07:56:50]

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

https://codereview.chromium.org/457683002/diff/20001/Source/modules/credentia...
Source/modules/credentialmanager/Credential.h:30:
Credential(PlatformCredential*);
On 2014/08/10 05:54:57, sof wrote:
> Perhaps you want an implicit conversion, but if not add 'explicit'?

Done.

https://codereview.chromium.org/457683002/diff/20001/Source/modules/credentia...
Source/modules/credentialmanager/Credential.h:33: Member<PlatformCredential>
m_private;
On 2014/08/10 05:54:57, sof wrote:
> Possible to come up with a better name?

Done.

https://codereview.chromium.org/457683002/diff/20001/Source/platform/credenti...
File Source/platform/credentialmanager/PlatformCredential.h (right):

https://codereview.chromium.org/457683002/diff/20001/Source/platform/credenti...
Source/platform/credentialmanager/PlatformCredential.h:25: virtual void
trace(Visitor*) { };
On 2014/08/10 05:54:57, sof wrote:
> nit: stray semi.

Done.

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

https://codereview.chromium.org/457683002/diff/20001/public/platform/WebCrede...
public/platform/WebCredential.h:13: class PlatformCredential;
On 2014/08/10 05:54:57, sof wrote:
> nit: move this into the namespace block below?

Done.

https://codereview.chromium.org/457683002/diff/20001/public/platform/WebFeder...
File public/platform/WebFederatedCredential.h (right):

https://codereview.chromium.org/457683002/diff/20001/public/platform/WebFeder...
public/platform/WebFederatedCredential.h:10: #include "WebPrivatePtr.h"
On 2014/08/10 05:54:57, sof wrote:
> Redundant #include.

Done.

https://codereview.chromium.org/457683002/diff/20001/public/platform/WebLocal...
File public/platform/WebLocalCredential.h (right):

https://codereview.chromium.org/457683002/diff/20001/public/platform/WebLocal...
public/platform/WebLocalCredential.h:10: #include "WebPrivatePtr.h"
On 2014/08/10 05:54:57, sof wrote:
> Redundant #include.

Done.

[commit-bot: I haz the power, 2014-08-11 08:17:32]

Change committed as 179919