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.
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.
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.
Created attachment 312758 [details] Patch
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.
(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.
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.
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?
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.
It would be OK to make it so you can WTFMove a UniqueRef into a unique_ptr, but not the other way around.
(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.
(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.
By using a rvalue reference we make it more explicit that the ownership is transferred and we avoid the parameter construction.
Created attachment 312860 [details] Patch
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?
Committed r218259: <http://trac.webkit.org/changeset/218259>