Avoid object slicing in WebCredential::create

Avoid object slicing in WebCredential::create

WebCredential::create currently returns a WebCredential. It tries to create one
of its subclasses, WebPasswordCredential or WebFederatedCredential,
respectively, but those object get sliced to their common base upon returning,
because a function returning A cannot return B even if B is a subclass of A.

To fix that, this CL changes create() to return a pointer to a WebCredential.
This allows creating subclasses of it while keeping the return type as
declared.

R=mkwst@chromium.org
BUG=610646
Committed: https://crrev.com/9818ab57e995473e2d41d5bb40b6a9abd2682151
Cr-Commit-Position: refs/heads/master@{#392959}

[vabr (Chromium), 2016-05-11 13:38:33]

Hi Mike,

Could you PTAL?

Thanks!
Vaclav

[haraken, 2016-05-11 14:16:11]

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

[haraken, 2016-05-11 14:16:11]

https://codereview.chromium.org/1967693003/diff/1/third_party/WebKit/Source/p...
File third_party/WebKit/Source/platform/exported/WebCredential.cpp (right):

https://codereview.chromium.org/1967693003/diff/1/third_party/WebKit/Source/p...
third_party/WebKit/Source/platform/exported/WebCredential.cpp:18: return
base::WrapUnique(new WebPasswordCredential(credential));

yutak@: We don't want to use base::WrapUnique, right?

std::unique_ptr(new WebPasswordCredential(credential));

https://codereview.chromium.org/1967693003/diff/1/third_party/WebKit/Source/p...
third_party/WebKit/Source/platform/exported/WebCredential.cpp:22: return
base::WrapUnique(new WebFederatedCredential(credential));

Ditto.

[haraken, 2016-05-11 14:16:34]

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

[vabr (Chromium), 2016-05-11 14:42:59]

https://codereview.chromium.org/1967693003/diff/1/third_party/WebKit/Source/p...
File third_party/WebKit/Source/platform/exported/WebCredential.cpp (right):

https://codereview.chromium.org/1967693003/diff/1/third_party/WebKit/Source/p...
third_party/WebKit/Source/platform/exported/WebCredential.cpp:18: return
base::WrapUnique(new WebPasswordCredential(credential));
On 2016/05/11 14:16:11, haraken wrote:
> 
> yutak@: We don't want to use base::WrapUnique, right?
> 
> std::unique_ptr(new WebPasswordCredential(credential));

What about using wrapUnique from Source/wtf/PtrUtil.h, introduced in
https://codereview.chromium.org/1865913005?
That would avoid the type duplication in 
std::unique_ptr<X>(new X)

[vabr (Chromium), 2016-05-11 15:09:31]

base::WrapUnique -> WTF::wrapUnique

[vabr (Chromium), 2016-05-11 15:10:43]

I changed the CL to use WTF's wrapUnique instead.

haraken & mkwst: PTAL

Thanks!
Vaclav

[Mike West, 2016-05-11 15:20:45]

Using WTF::wrapUnique LGTM. Thanks for fixing this, Vaclav.

[vabr (Chromium), 2016-05-11 15:46:32]

Adding the hilarious namespace :)

[vabr (Chromium), 2016-05-11 15:48:44]

On 2016/05/11 15:20:45, Mike West wrote:
> Using WTF::wrapUnique LGTM. Thanks for fixing this, Vaclav.

Thanks, Mike!

I was not sure if you meant that I should add the WTF:: prefix, or that you are
just expressing being OK with using that function independently of relying on
"using" declarations. So I added the prefix just to be sure, and am sending this
to the CQ.

Cheers,
Vaclav

[vabr (Chromium), 2016-05-11 15:48:51]

The CQ bit was checked by vabr@chromium.org

[vabr (Chromium), 2016-05-11 15:48:52]

The patchset sent to the CQ was uploaded after l-g-t-m from mkwst@chromium.org
Link to the patchset: https://codereview.chromium.org/1967693003/#ps40001
(title: "Adding the hilarious namespace :)")

[commit-bot: I haz the power, 2016-05-11 15:49:17]

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

[haraken, 2016-05-11 15:54:19]

I'll defer the review to yutak@.

As far as I read the document
(https://chromium.googlesource.com/chromium/src/+/master/third_party/WebKit/So...),
it seems that std::unique_ptr() is recommended.

[commit-bot: I haz the power, 2016-05-11 17:01:40]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2016-05-11 17:03:32]

Description was changed from

==========
Avoid object slicing in WebCredential::create

WebCredential::create currently returns a WebCredential. It tries to create one
of its subclasses, WebPasswordCredential or WebFederatedCredential,
respectively, but those object get sliced to their common base upon returning,
because a function returning A cannot return B even if B is a subclass of A.

To fix that, this CL changes create() to return a pointer to a WebCredential.
This allows creating subclasses of it while keeping the return type as
declared.

R=mkwst@chromium.org
BUG=610646
==========

to

==========
Avoid object slicing in WebCredential::create

WebCredential::create currently returns a WebCredential. It tries to create one
of its subclasses, WebPasswordCredential or WebFederatedCredential,
respectively, but those object get sliced to their common base upon returning,
because a function returning A cannot return B even if B is a subclass of A.

To fix that, this CL changes create() to return a pointer to a WebCredential.
This allows creating subclasses of it while keeping the return type as
declared.

R=mkwst@chromium.org
BUG=610646

Committed: https://crrev.com/9818ab57e995473e2d41d5bb40b6a9abd2682151
Cr-Commit-Position: refs/heads/master@{#392959}
==========

[commit-bot: I haz the power, 2016-05-11 17:03:33]

Patchset 3 (id:??) landed as
https://crrev.com/9818ab57e995473e2d41d5bb40b6a9abd2682151
Cr-Commit-Position: refs/heads/master@{#392959}

[Yuta Kitamura, 2016-05-12 03:58:55]

After-the-fact LGTM. (With nits.)

https://codereview.chromium.org/1967693003/diff/40001/third_party/WebKit/Sour...
File
third_party/WebKit/Source/modules/credentialmanager/CredentialsContainer.cpp
(right):

https://codereview.chromium.org/1967693003/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/credentialmanager/CredentialsContainer.cpp:174:
auto web_credential =
WebCredential::create(credential->getPlatformCredential());
This variable violates our naming convention

https://codereview.chromium.org/1967693003/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/platform/exported/WebCredential.cpp (right):

https://codereview.chromium.org/1967693003/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/exported/WebCredential.cpp:17: return
WTF::wrapUnique(new WebPasswordCredential(credential));
"WTF::" shouldn't be necessary.

[haraken, 2016-05-12 04:05:35]

On 2016/05/12 03:58:55, Yuta Kitamura wrote:
> After-the-fact LGTM. (With nits.)
> 
>
https://codereview.chromium.org/1967693003/diff/40001/third_party/WebKit/Sour...
> File
> third_party/WebKit/Source/modules/credentialmanager/CredentialsContainer.cpp
> (right):
> 
>
https://codereview.chromium.org/1967693003/diff/40001/third_party/WebKit/Sour...
>
third_party/WebKit/Source/modules/credentialmanager/CredentialsContainer.cpp:174:
> auto web_credential =
> WebCredential::create(credential->getPlatformCredential());
> This variable violates our naming convention
> 
>
https://codereview.chromium.org/1967693003/diff/40001/third_party/WebKit/Sour...
> File third_party/WebKit/Source/platform/exported/WebCredential.cpp (right):
> 
>
https://codereview.chromium.org/1967693003/diff/40001/third_party/WebKit/Sour...
> third_party/WebKit/Source/platform/exported/WebCredential.cpp:17: return
> WTF::wrapUnique(new WebPasswordCredential(credential));
> "WTF::" shouldn't be necessary.

Shall we update the document
(https://chromium.googlesource.com/chromium/src/+/master/third_party/WebKit/So...

Your previous email
(https://groups.google.com/a/chromium.org/d/topic/platform-architecture-dev/2U...)
is also implying that you may want to avoid using wrapUnique().

[Yuta Kitamura, 2016-05-12 05:51:43]

On 2016/05/12 04:05:35, haraken wrote:
> 
> Shall we update the document
>
(https://chromium.googlesource.com/chromium/src/+/master/third_party/WebKit/So...)

Yeah, I will.

> 
> Your previous email
>
(https://groups.google.com/a/chromium.org/d/topic/platform-architecture-dev/2U...)
> is also implying that you may want to avoid using wrapUnique().

Given that base::MakeUnique isn't progressing (AFAIK) and
wrapUnique() is added in WTF, I'm leaning towards just using
it where appropriate.

[vabr (Chromium), 2016-05-12 08:42:45]

On 2016/05/12 05:51:43, Yuta Kitamura wrote:
> On 2016/05/12 04:05:35, haraken wrote:
> > 
> > Shall we update the document
> >
>
(https://chromium.googlesource.com/chromium/src/+/master/third_party/WebKit/So...)
> 
> Yeah, I will.
> 
> > 
> > Your previous email
> >
>
(https://groups.google.com/a/chromium.org/d/topic/platform-architecture-dev/2U...)
> > is also implying that you may want to avoid using wrapUnique().
> 
> Given that base::MakeUnique isn't progressing (AFAIK) and
> wrapUnique() is added in WTF, I'm leaning towards just using
> it where appropriate.

Thanks for all the feedback.

I uploaded https://codereview.chromium.org/1973943002/ to address yutak@'s
comments.

Cheers,
Vaclav

[dcheng, 2016-05-12 16:47:50]

On 2016/05/12 at 05:51:43, yutak wrote:
> On 2016/05/12 04:05:35, haraken wrote:
> > 
> > Shall we update the document
> >
(https://chromium.googlesource.com/chromium/src/+/master/third_party/WebKit/So...)
> 
> Yeah, I will.
> 
> > 
> > Your previous email
> >
(https://groups.google.com/a/chromium.org/d/topic/platform-architecture-dev/2U...)
> > is also implying that you may want to avoid using wrapUnique().
> 
> Given that base::MakeUnique isn't progressing (AFAIK) and
> wrapUnique() is added in WTF, I'm leaning towards just using
> it where appropriate.

Actually, base::MakeUnique has already landed. But I haven't announced it
publicly yet because I'm waiting for codesearch to pick up a CL I landed to help
it trace constructor calls through base::MakeUnique.