Bug 173266 – API clients should not be passed by value to the setters

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKit2

[Carlos Garcia Campos]

The pattern we follow for all the clients is that when a client is unset (nullptr is passed to the setter) we create a new empty client, so the member is never nullptr.

[Carlos Garcia Campos]

Well, we are actually passing nullptr to unset the client, so we need to assign nullptr to the parameter, which is not possible with UniqueRef. To use UniqueRef we would need to provide another method to unset/reset clients (or use std::optional?). I'm not sure it's worth it, to be honest.

[Carlos Garcia Campos]

Ok, there's another possibility, if we add 

UniqueRef(std::unique_ptr<U>&& otherRef);

we can keep using a std::unique_ptr for the parameter, and don't need to change the code in the callers either.

[Carlos Garcia Campos]

Created attachment 312758 [details]
Patch

[Build Bot]

Attachment 312758 [details] did not pass style-queue:


ERROR: Source/WebKit2/WebProcess/WebPage/WebPage.cpp:341:  Code inside a namespace should not be indented.  [whitespace/indent] [4]
Total errors found: 1 in 15 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Carlos Garcia Campos]

(In reply to Build Bot from comment #4)
> Attachment 312758 [details] did not pass style-queue:
> 
> 
> ERROR: Source/WebKit2/WebProcess/WebPage/WebPage.cpp:341:  Code inside a
> namespace should not be indented.  [whitespace/indent] [4]

I think style checker was confused by the previous line, code is correctly indented there.

> Total errors found: 1 in 15 files
> 
> 
> If any of these errors are false positives, please file a bug against
> check-webkit-style.

[Alex Christensen]

Comment on attachment 312758 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=312758&action=review

> Source/WTF/wtf/UniqueRef.h:52
> +    UniqueRef(std::unique_ptr<U>&& otherRef)

Right now a null UniqueRef is syntactically impossible to make.  This makes that not the case.  This is not what UniqueRef is supposed to be.

[Carlos Garcia Campos]

Comment on attachment 312758 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=312758&action=review

>> Source/WTF/wtf/UniqueRef.h:52
>> +    UniqueRef(std::unique_ptr<U>&& otherRef)
> 
> Right now a null UniqueRef is syntactically impossible to make.  This makes that not the case.  This is not what UniqueRef is supposed to be.

Well, that's why there's ASSERT(m_ref) below. I agree it would be better a compile time check. Do you have any other suggestion? or better not to use UniqueRef for the clients in the end?

[Alex Christensen]

I think it's better to use UniqueRef and to use makeUniqueRef to make them.  If you can't go to the source of these std::unique_ptrs and replace them with makeUniqueRef, let's leave them as std::unique_ptrs for now.

[Alex Christensen]

It would be OK to make it so you can WTFMove a UniqueRef into a unique_ptr, but not the other way around.

[Carlos Garcia Campos]

(In reply to Alex Christensen from comment #8)
> I think it's better to use UniqueRef and to use makeUniqueRef to make them. 
> If you can't go to the source of these std::unique_ptrs and replace them
> with makeUniqueRef, let's leave them as std::unique_ptrs for now.

The problem is not the source of the unique_ptrs, but the setters. This is what we do:

void setClient(std::unique_ptr<Client> client)
{
    if (!client)
        m_client = std::make_unique<Client>();
    else
        m_client = WTFMove(client);
}

There's no problem in using makeUniqueRef there, the problem is that the setter can't receive a null client if we use UniqueRef as the parameter. I guess we can still do this, keeping the unique_ptr as the parameter and leaking it to adopt it by the UniqueRef. I added the UniqueRef just for convenience.

[Carlos Garcia Campos]

(In reply to Carlos Garcia Campos from comment #10)
> (In reply to Alex Christensen from comment #8)
> > I think it's better to use UniqueRef and to use makeUniqueRef to make them. 
> > If you can't go to the source of these std::unique_ptrs and replace them
> > with makeUniqueRef, let's leave them as std::unique_ptrs for now.
> 
> The problem is not the source of the unique_ptrs, but the setters. This is
> what we do:
> 
> void setClient(std::unique_ptr<Client> client)
> {
>     if (!client)
>         m_client = std::make_unique<Client>();
>     else
>         m_client = WTFMove(client);
> }
> 
> There's no problem in using makeUniqueRef there, the problem is that the
> setter can't receive a null client if we use UniqueRef as the parameter. I
> guess we can still do this, keeping the unique_ptr as the parameter and
> leaking it to adopt it by the UniqueRef. I added the UniqueRef just for
> convenience.

hmm, I don't think this is possible either, and using std::optional for the parameter would only make it more complex, IMO, so I'm going to leave the unique_ptrs. I can reuse this bug to not pass the clients by value and use an rvalue ref like you and Darin suggested.

[Carlos Garcia Campos]

By using a rvalue reference we make it more explicit that the ownership is transferred and we avoid the parameter construction.

[Carlos Garcia Campos]

Created attachment 312860 [details]
Patch

[Michael Catanzaro]

I haven't looked at this closely, and maybe my question is obviously wrong, but here goes: why std::unique_ptr<API::InjectedBundle::EditorClient>&& and not just API::InjectedBundle::EditorClient&&? There's no polymorphism here, so is the pointer type really needed?

[Carlos Garcia Campos]

Committed r218259: <http://trac.webkit.org/changeset/218259>