Credential Manager: Add a 'formData' property to LocalCredential.

Credential Manager: Add a 'formData' property to LocalCredential.

As specced at http://mikewest.github.io/credentialmanagement/spec/#interfaces-credential-types-localcredential

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

[Mike West, 2014-10-20 08:05:10]

mkwst@chromium.org changed reviewers:
+ oilpan-reviews@chromium.org, vabr@chromium.org

[Mike West, 2014-10-20 08:05:11]

Hello, Oilpanners! Would you please sanity check this change? I'm adding a
RefPtr property to LocalCredential, and it's not entirely clear to me how best
to do that. :)

Vaclav, would you mind taking a look at the rest of the change?

-mike

https://codereview.chromium.org/645023005/diff/1/Source/modules/credentialman...
File Source/modules/credentialmanager/Credential.h (right):

https://codereview.chromium.org/645023005/diff/1/Source/modules/credentialman...
Source/modules/credentialmanager/Credential.h:19: class Credential : public
GarbageCollectedFinalized<Credential>, public ScriptWrappable {
Is there a way to avoid finalizing Credential? If not, do I need to add an empty
destructor?

[vabr (Chromium), 2014-10-20 08:16:01]

Hi Mike,

This LGTM, as what I see in the spec looks like what you add in the code. I'm of
no help with your Blink questions, but you did not expect that, did you? :)

Cheers,
Vaclav

[haraken, 2014-10-20 08:16:02]

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

[haraken, 2014-10-20 08:16:03]

https://codereview.chromium.org/645023005/diff/1/Source/modules/credentialman...
File Source/modules/credentialmanager/Credential.h (right):

https://codereview.chromium.org/645023005/diff/1/Source/modules/credentialman...
Source/modules/credentialmanager/Credential.h:19: class Credential : public
GarbageCollectedFinalized<Credential>, public ScriptWrappable {
On 2014/10/20 08:05:11, Mike West wrote:
> Is there a way to avoid finalizing Credential? If not, do I need to add an
empty
> destructor?

Why can't you simply use GarbageCollected<Credential>? I wonder why you need to
use GarbageCollectedFinalized.

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

https://codereview.chromium.org/645023005/diff/1/Source/modules/credentialman...
Source/modules/credentialmanager/LocalCredential.cpp:48:
Credential::trace(visitor);

We normally make this a tail call.

https://codereview.chromium.org/645023005/diff/1/Source/modules/credentialman...
File Source/modules/credentialmanager/LocalCredential.h (right):

https://codereview.chromium.org/645023005/diff/1/Source/modules/credentialman...
Source/modules/credentialmanager/LocalCredential.h:46:
RefPtrWillBeRawPtr<DOMFormData> m_formData;

This should be RefPtrWillBeMember<DOMFormData>.

[sof, 2014-10-20 08:18:53]

sigbjornf@opera.com changed reviewers:
+ sigbjornf@opera.com

[sof, 2014-10-20 08:18:53]

https://codereview.chromium.org/645023005/diff/1/Source/modules/credentialman...
File Source/modules/credentialmanager/Credential.h (right):

https://codereview.chromium.org/645023005/diff/1/Source/modules/credentialman...
Source/modules/credentialmanager/Credential.h:19: class Credential : public
GarbageCollectedFinalized<Credential>, public ScriptWrappable {
On 2014/10/20 08:16:03, haraken wrote:
> On 2014/10/20 08:05:11, Mike West wrote:
> > Is there a way to avoid finalizing Credential? If not, do I need to add an
> empty
> > destructor?
> 
> Why can't you simply use GarbageCollected<Credential>? I wonder why you need
to
> use GarbageCollectedFinalized.

It's due to the LocalCredential change made here. I suggest you leave it as
*Finalized, but add a FIXME next to the DOMFormData field to reconsider its use
once that becomes Member<>.

[sof, 2014-10-20 08:35:46]

https://codereview.chromium.org/645023005/diff/1/LayoutTests/http/tests/crede...
File LayoutTests/http/tests/credentialmanager/localcredential-basics.html
(right):

https://codereview.chromium.org/645023005/diff/1/LayoutTests/http/tests/crede...
LayoutTests/http/tests/credentialmanager/localcredential-basics.html:17:
assert_true(credential.formData instanceof FormData);
Can't you test this via the other typechecks in verifyInterface() instead?

[Mike West, 2014-10-20 09:47:38]

Thanks.

https://codereview.chromium.org/645023005/diff/1/LayoutTests/http/tests/crede...
File LayoutTests/http/tests/credentialmanager/localcredential-basics.html
(right):

https://codereview.chromium.org/645023005/diff/1/LayoutTests/http/tests/crede...
LayoutTests/http/tests/credentialmanager/localcredential-basics.html:17:
assert_true(credential.formData instanceof FormData);
On 2014/10/20 08:35:46, sof wrote:
> Can't you test this via the other typechecks in verifyInterface() instead?

Sure. I was originally intending to check the values here, but as it turns out,
we don't expose FormData getters. Which is weird, but happily exactly what I
want anyway. :)

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

https://codereview.chromium.org/645023005/diff/1/Source/modules/credentialman...
Source/modules/credentialmanager/LocalCredential.cpp:48:
Credential::trace(visitor);
On 2014/10/20 08:16:03, haraken wrote:
> 
> We normally make this a tail call.

Done.

https://codereview.chromium.org/645023005/diff/1/Source/modules/credentialman...
File Source/modules/credentialmanager/LocalCredential.h (right):

https://codereview.chromium.org/645023005/diff/1/Source/modules/credentialman...
Source/modules/credentialmanager/LocalCredential.h:46:
RefPtrWillBeRawPtr<DOMFormData> m_formData;
On 2014/10/20 08:16:03, haraken wrote:
> 
> This should be RefPtrWillBeMember<DOMFormData>.

Done.

[sof, 2014-10-20 09:53:52]

lgtm

[Mike West, 2014-10-20 09:58:00]

The CQ bit was checked by mkwst@chromium.org

[commit-bot: I haz the power, 2014-10-20 09:58:50]

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

[haraken, 2014-10-20 10:17:38]

LGTM

[commit-bot: I haz the power, 2014-10-20 10:28:04]

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

[commit-bot: I haz the power, 2014-10-20 10:28:05]

Try jobs failed on following builders:
  win_gpu_triggered_tests on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/win_gpu_triggered...)

[Mike West, 2014-10-20 10:30:50]

The CQ bit was checked by mkwst@chromium.org

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

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

[commit-bot: I haz the power, 2014-10-20 10:34:51]

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

[commit-bot: I haz the power, 2014-10-20 10:34:51]

Try jobs failed on following builders:
  win_gpu_triggered_tests on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/win_gpu_triggered...)

[Mike West, 2014-10-23 13:13:16]

The CQ bit was checked by mkwst@chromium.org

[commit-bot: I haz the power, 2014-10-23 13:13:51]

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

[commit-bot: I haz the power, 2014-10-23 15:19:49]

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

[commit-bot: I haz the power, 2014-10-23 15:19:50]

Try jobs failed on following builders:
  win_blink_rel on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/win_blink_rel/builds/33294)

[Mike West, 2014-10-23 15:21:22]

The CQ bit was checked by mkwst@chromium.org

[commit-bot: I haz the power, 2014-10-23 15:22:01]

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

[commit-bot: I haz the power, 2014-10-23 16:00:36]

Committed patchset #3 (id:40001) as 184280