[webkit-reviews] review denied: [Bug 74440] RefPtr Basics page should talk about OwnPtr : [Attachment 120051] fixed style error

[bugzilla-daemon at webkit.org]

Eric Seidel <eric at webkit.org> has denied Joe Mason <jmason at rim.com>'s request
for review:
Bug 74440: RefPtr Basics page should talk about OwnPtr
https://bugs.webkit.org/show_bug.cgi?id=74440

Attachment 120051: fixed style error
https://bugs.webkit.org/attachment.cgi?id=120051&action=review

------- Additional Comments from Eric Seidel <eric at webkit.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=120051&action=review


> Websites/webkit.org/coding/OwnPtr.html:34
> +by the <span class="class">OwnPtr</span> is automatically deleted when the
> +<span class="class">OwnPtr</span> object goes out of scope. The <span

More generally, when the OwnPtr is itself deconstructed, but this is OK...

> Websites/webkit.org/coding/OwnPtr.html:86
> +    : m_data(new DocumentPrivate)

Moreover, we plan to make this a style error.  There is a bug on file
somewhere.

> Websites/webkit.org/coding/OwnPtr.html:130
> +<span class="comment">// legacy function taking a raw pointer</span>
> +void printNode(const Node* node);

This is not necessarily legacy.  It's normal to pass around raw pointers, when
there is no expected ownership transfer.  We also don't generally use const
with RefCounted classes, like Node, as you can't then refcount them (which may
be your intended implication from this?)

> Websites/webkit.org/coding/OwnPtr.html:133
> +void destroyNode(Node* node);

This is confusing because Node* exists in WebCore and is a RefCounted type.  I
would chose a different name.

> Websites/webkit.org/coding/OwnPtr.html:152
> +<p>You can force an <span class="class">OwnPtr</span> to give up ownership
of a
> +raw pointer by calling the <span class="function">leakPtr</span> function.
The
> +caller of this function must arrange for the raw pointer to be deleted in
> +another way. After the raw pointer has been leaked, the <span
> +class="class">OwnPtr</span> is set to 0, since it no longer owns the
> +pointer.</p>

I think I would have structured this paragraph slightly differently. something
like this:

You can take ownership from an OwnPtr, using leakPtr(), which will null out the
OwnPtr, as well as return the raw pointer it pointed to.  The caller is then
responsible for calling delete() on the returned raw pointer.

I might mention here also that .release() is the preferred method of taking
ownership from an OwnPtr, as it returns a PassRefPtr, which you're about to
cover.

> Websites/webkit.org/coding/OwnPtr.html:172
> +<p><span class="function">adoptPtr</span> and <span
> +class="function">leakPtr</span> can be used together to pass ownership of an

> +object from one <span class="class">OwnPtr</span> to another: the first
pointer
> +leaks the object and the second adopts it.</p>

.release() is the way to do this, no?

> Websites/webkit.org/coding/OwnPtr.html:184
> +    m_title = adoptPtr(title.leakPtr());

.release() is the right way.