Clean up WebDOMEvent ownership of WebCore::Event.

Clean up WebDOMEvent ownership of WebCore::Event.

Previously, it did manual reference counting. This change uses WebPrivatePtr to make the code clearer and safer.

This change also introduces a copy constructor to WebPrivatePtr. Without that, the new WebDOMEvent was quietly using the compiler-provided copy constructor, which does not reference count. This change also disables copy and assign for WebPrivatePtr when WEBKIT_IMPLEMENTATION is not set, to make sure the compiler-provided ones are not used.

BUG=

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

[dmichael (off chromium), 2013-05-20 21:58:43]

https://codereview.chromium.org/15271012/diff/1/Source/WebKit/chromium/src/We...
File Source/WebKit/chromium/src/WebDOMEvent.cpp (left):

https://codereview.chromium.org/15271012/diff/1/Source/WebKit/chromium/src/We...
Source/WebKit/chromium/src/WebDOMEvent.cpp:74: return
static_cast<WebCore::Event*>(m_private);
This actually looks like a bug to me ^^^^

There's one reference count, and two objects that think they own it (this
WebDOMEvent, and the caller). It looks like this was being used here at least:
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...

[abarth-chromium, 2013-05-20 22:04:45]

https://codereview.chromium.org/15271012/diff/1/Source/WebKit/chromium/public...
File Source/WebKit/chromium/public/WebDOMEvent.h (right):

https://codereview.chromium.org/15271012/diff/1/Source/WebKit/chromium/public...
Source/WebKit/chromium/public/WebDOMEvent.h:117: WTF::RefPtr<WebDOMEventPrivate>
m_private;
We can't use WTF::RefPtr directly because the consumer of the API can't see it. 
We should be able to use WebPrivatePtr though.

[dmichael (off chromium), 2013-05-20 22:35:49]

Sorry about that, using WebPrivatePtr now.

https://codereview.chromium.org/15271012/diff/5001/Source/WebKit/chromium/src...
File Source/WebKit/chromium/src/WebDOMEvent.cpp (right):

https://codereview.chromium.org/15271012/diff/5001/Source/WebKit/chromium/src...
Source/WebKit/chromium/src/WebDOMEvent.cpp:68: ASSERT(m_private.get());
If you think it would be better, I could add an appropriate operator for
evaluating WebPrivatePtr in a boolean expression.

[abarth-chromium, 2013-05-20 22:54:50]

OMG, so much better.

https://codereview.chromium.org/15271012/diff/5001/Source/WebKit/chromium/src...
File Source/WebKit/chromium/src/WebDOMEvent.cpp (right):

https://codereview.chromium.org/15271012/diff/5001/Source/WebKit/chromium/src...
Source/WebKit/chromium/src/WebDOMEvent.cpp:68: ASSERT(m_private.get());
On 2013/05/20 22:35:49, dmichael wrote:
> If you think it would be better, I could add an appropriate operator for
> evaluating WebPrivatePtr in a boolean expression.

IMHO, this is fine.

[dmichael (off chromium), 2013-05-20 23:12:02]

So... Does it lgty? ;-)
On May 20, 2013 4:54 PM, <abarth@chromium.org> wrote:

> OMG, so much better.
>
>
> https://codereview.chromium.**org/15271012/diff/5001/Source/**
>
WebKit/chromium/src/**WebDOMEvent.cpp<https://codereview.chromium.org/15271012/diff/5001/Source/WebKit/chromium/src/WebDOMEvent.cpp>
> File Source/WebKit/chromium/src/**WebDOMEvent.cpp (right):
>
> https://codereview.chromium.**org/15271012/diff/5001/Source/**
>
WebKit/chromium/src/**WebDOMEvent.cpp#newcode68<https://codereview.chromium.org/15271012/diff/5001/Source/WebKit/chromium/src/WebDOMEvent.cpp#newcode68>
> Source/WebKit/chromium/src/**WebDOMEvent.cpp:68: ASSERT(m_private.get());
> On 2013/05/20 22:35:49, dmichael wrote:
>
>> If you think it would be better, I could add an appropriate operator
>>
> for
>
>> evaluating WebPrivatePtr in a boolean expression.
>>
>
> IMHO, this is fine.
>
>
https://codereview.chromium.**org/15271012/<https://codereview.chromium.org/1...
>

[dmichael (off chromium), 2013-05-22 16:08:28]

I had to do a little more work to get the tests passing; WebPrivatePtr exposed
the compiler-provided copy ctor, which was breaking stuff.

PTAL.

https://codereview.chromium.org/15271012/diff/27001/Source/WebKit/chromium/pu...
File Source/WebKit/chromium/public/WebDOMEvent.h (right):

https://codereview.chromium.org/15271012/diff/27001/Source/WebKit/chromium/pu...
Source/WebKit/chromium/public/WebDOMEvent.h:57: WebDOMEvent(const WebDOMEvent&);
I moved this to the body so that I could use the WebPrivatePtr copy constructor
but still provide this copy constructor to clients (it's used in Chromium code
already).

[dmichael (off chromium), 2013-05-22 16:21:09]

Gah... sorry, might as well still hold off. It looks like some other stuff
depends on visibility to the broken compiler-provided WebPrivatePtr ctor. Will
fix and reupload.

[abarth-chromium, 2013-05-22 16:21:49]

https://codereview.chromium.org/15271012/diff/27001/Source/WebKit/chromium/pu...
File Source/WebKit/chromium/public/WebDOMEvent.h (right):

https://codereview.chromium.org/15271012/diff/27001/Source/WebKit/chromium/pu...
Source/WebKit/chromium/public/WebDOMEvent.h:57: WebDOMEvent(const WebDOMEvent&);
On 2013/05/22 16:08:28, dmichael wrote:
> I moved this to the body so that I could use the WebPrivatePtr copy
constructor
> but still provide this copy constructor to clients (it's used in Chromium code
> already).

It's probably better to do this using the same pattern we use for WebNode:

    WebNode(const WebNode& n) { assign(n); }

https://codereview.chromium.org/15271012/diff/27001/public/platform/WebPrivat...
File public/platform/WebPrivatePtr.h (right):

https://codereview.chromium.org/15271012/diff/27001/public/platform/WebPrivat...
public/platform/WebPrivatePtr.h:60: 
We shouldn't need to modify WebPrivatePtr

[dmichael (off chromium), 2013-05-22 17:00:28]

Okay...  I think it's actually good this time :-)

https://codereview.chromium.org/15271012/diff/33001/Source/WebKit/chromium/pu...
File Source/WebKit/chromium/public/WebSpeechInputResult.h (right):

https://codereview.chromium.org/15271012/diff/33001/Source/WebKit/chromium/pu...
Source/WebKit/chromium/public/WebSpeechInputResult.h:52: }
The compiler-provided assignment was being used in a mock object in
DumpRenderTree, and that was implicitly using the compiler-provided assignment
of WebPrivatePtr.

[abarth-chromium, 2013-05-22 17:16:11]

https://codereview.chromium.org/15271012/diff/33001/Source/WebKit/chromium/sr...
File Source/WebKit/chromium/src/WebDOMEvent.cpp (right):

https://codereview.chromium.org/15271012/diff/33001/Source/WebKit/chromium/sr...
Source/WebKit/chromium/src/WebDOMEvent.cpp:49: }
Is there some reason you don't want to use the same pattern we use for WebNode? 
There we put the copy constructor inline in the header and implement it using
"assign"

https://codereview.chromium.org/15271012/diff/33001/public/platform/WebPrivat...
File public/platform/WebPrivatePtr.h (right):

https://codereview.chromium.org/15271012/diff/33001/public/platform/WebPrivat...
public/platform/WebPrivatePtr.h:59: }
We shouldn't need to modify this class.

[dmichael (off chromium), 2013-05-22 17:37:33]

https://codereview.chromium.org/15271012/diff/33001/Source/WebKit/chromium/sr...
File Source/WebKit/chromium/src/WebDOMEvent.cpp (right):

https://codereview.chromium.org/15271012/diff/33001/Source/WebKit/chromium/sr...
Source/WebKit/chromium/src/WebDOMEvent.cpp:49: }
On 2013/05/22 17:16:11, abarth wrote:
> Is there some reason you don't want to use the same pattern we use for
WebNode? 
> There we put the copy constructor inline in the header and implement it using
> "assign"
This copy constructor has to have WEBKIT_EXPORT, so can't be inlined. I could
use assign(), but it's usually considered best practice to use the initializer
list when you can (rather than allow m_private to be default initialized, then
assign to it). But that's pretty minor, so I'm happy to change  it if you want.

https://codereview.chromium.org/15271012/diff/33001/public/platform/WebPrivat...
File public/platform/WebPrivatePtr.h (right):

https://codereview.chromium.org/15271012/diff/33001/public/platform/WebPrivat...
public/platform/WebPrivatePtr.h:59: }
On 2013/05/22 17:16:11, abarth wrote:
> We shouldn't need to modify this class.

See the updated CL description. WebPrivatePtr did not define a copy constructor,
so the compiler-provided version was being used, and it didn't handle the
reference count. We can certainly avoid using it in this CL (using assign() as
you suggested in WebDOMEvent), but if we don't fix this, it will bite somebody
else. We *at least* need to disable the copy ctor. I think it's better to just
make it work; I don't see a downside.

Side note that WebSpeechInputRequest, when used from chromium or DumpRenderTree,
was using the compiler-provided assignment operator (since the real
implementation is WEBKIT_IMPLEMENTATION only). So disabling that below seems
appropriate, too.

[abarth-chromium, 2013-05-22 18:06:23]

https://codereview.chromium.org/15271012/diff/33001/Source/WebKit/chromium/sr...
File Source/WebKit/chromium/src/WebDOMEvent.cpp (right):

https://codereview.chromium.org/15271012/diff/33001/Source/WebKit/chromium/sr...
Source/WebKit/chromium/src/WebDOMEvent.cpp:49: }
On 2013/05/22 17:37:33, dmichael wrote:
> On 2013/05/22 17:16:11, abarth wrote:
> > Is there some reason you don't want to use the same pattern we use for
> WebNode? 
> > There we put the copy constructor inline in the header and implement it
using
> > "assign"
>
> This copy constructor has to have WEBKIT_EXPORT, so can't be inlined. I could
> use assign(), but it's usually considered best practice to use the initializer
> list when you can (rather than allow m_private to be default initialized, then
> assign to it). But that's pretty minor, so I'm happy to change  it if you
want.

If you move the copy constructor inline in the header, then it won't need
WEBKIT_EXPORT because the caller will inline it.

I don't fully understand why we prefer to inline constructors and export assign
functions instead.  We could ask Darin, but I don't think there's any harm in
continuing the pattern.

https://codereview.chromium.org/15271012/diff/33001/public/platform/WebPrivat...
File public/platform/WebPrivatePtr.h (right):

https://codereview.chromium.org/15271012/diff/33001/public/platform/WebPrivat...
public/platform/WebPrivatePtr.h:59: }
On 2013/05/22 17:37:33, dmichael wrote:
> On 2013/05/22 17:16:11, abarth wrote:
> > We shouldn't need to modify this class.
> 
> See the updated CL description. WebPrivatePtr did not define a copy
constructor,
> so the compiler-provided version was being used, and it didn't handle the
> reference count. We can certainly avoid using it in this CL (using assign() as
> you suggested in WebDOMEvent), but if we don't fix this, it will bite somebody
> else. We *at least* need to disable the copy ctor. I think it's better to just
> make it work; I don't see a downside.
> 
> Side note that WebSpeechInputRequest, when used from chromium or
DumpRenderTree,
> was using the compiler-provided assignment operator (since the real
> implementation is WEBKIT_IMPLEMENTATION only). So disabling that below seems
> appropriate, too.

Good point.  Let's try disabling the copy constructor.

[dmichael (off chromium), 2013-05-22 18:44:26]

https://codereview.chromium.org/15271012/diff/33001/Source/WebKit/chromium/sr...
File Source/WebKit/chromium/src/WebDOMEvent.cpp (right):

https://codereview.chromium.org/15271012/diff/33001/Source/WebKit/chromium/sr...
Source/WebKit/chromium/src/WebDOMEvent.cpp:49: }
On 2013/05/22 18:06:23, abarth wrote:
> On 2013/05/22 17:37:33, dmichael wrote:
> > On 2013/05/22 17:16:11, abarth wrote:
> > > Is there some reason you don't want to use the same pattern we use for
> > WebNode? 
> > > There we put the copy constructor inline in the header and implement it
> using
> > > "assign"
> >
> > This copy constructor has to have WEBKIT_EXPORT, so can't be inlined. I
could
> > use assign(), but it's usually considered best practice to use the
initializer
> > list when you can (rather than allow m_private to be default initialized,
then
> > assign to it). But that's pretty minor, so I'm happy to change  it if you
> want.
> 
> If you move the copy constructor inline in the header, then it won't need
> WEBKIT_EXPORT because the caller will inline it.
> 
> I don't fully understand why we prefer to inline constructors and export
assign
> functions instead.  We could ask Darin, but I don't think there's any harm in
> continuing the pattern.
Fine with me; consistency is valuable. Done.

https://codereview.chromium.org/15271012/diff/33001/public/platform/WebPrivat...
File public/platform/WebPrivatePtr.h (right):

https://codereview.chromium.org/15271012/diff/33001/public/platform/WebPrivat...
public/platform/WebPrivatePtr.h:59: }
On 2013/05/22 18:06:23, abarth wrote:
> On 2013/05/22 17:37:33, dmichael wrote:
> > On 2013/05/22 17:16:11, abarth wrote:
> > > We shouldn't need to modify this class.
> > 
> > See the updated CL description. WebPrivatePtr did not define a copy
> constructor,
> > so the compiler-provided version was being used, and it didn't handle the
> > reference count. We can certainly avoid using it in this CL (using assign()
as
> > you suggested in WebDOMEvent), but if we don't fix this, it will bite
somebody
> > else. We *at least* need to disable the copy ctor. I think it's better to
just
> > make it work; I don't see a downside.
> > 
> > Side note that WebSpeechInputRequest, when used from chromium or
> DumpRenderTree,
> > was using the compiler-provided assignment operator (since the real
> > implementation is WEBKIT_IMPLEMENTATION only). So disabling that below seems
> > appropriate, too.
> 
> Good point.  Let's try disabling the copy constructor.
Done. I kind of liked fixing it better; I wasn't sure how to comment on a copy
ctor that's disabled, for a class that really looks like it should have a copy
constructor. I think it will be confusing to the next person who tries to use it
 (Usual practice is the "rule of three":
http://www.drdobbs.com/c-made-easier-the-rule-of-three/184401400 ). Though the
Google style guide isn't clear on the matter.
"If your class needs to be copyable, prefer providing a copy method, such as
CopyFrom() or Clone(), rather than a copy constructor, because such methods
cannot be invoked implicitly. If a copy method is insufficient in your situation
(e.g. for performance reasons, or because your class needs to be stored by value
in an STL container), provide both a copy constructor and assignment operator."
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml#Copy_Constructors

...and WebKit/Blink style guides are silent on the matter.

[darin1, 2013-05-22 18:58:24]

Sorry for the awkwardness.  This system is designed for: 1) making WebFoo
classes act like RefPtr, 2) enabling you to write less code.  Just mimic
the existing pattern FTW.

(I chose not to export ctors b/c some systems generate multiple DLL entry
points when you export a single ctor, to distinguish new WebFoo and new
WebFoo[n]… or something like that).
On May 22, 2013 11:44 AM, <dmichael@chromium.org> wrote:

>
> https://codereview.chromium.**org/15271012/diff/33001/**
>
Source/WebKit/chromium/src/**WebDOMEvent.cpp<https://codereview.chromium.org/15271012/diff/33001/Source/WebKit/chromium/src/WebDOMEvent.cpp>
> File Source/WebKit/chromium/src/**WebDOMEvent.cpp (right):
>
> https://codereview.chromium.**org/15271012/diff/33001/**
>
Source/WebKit/chromium/src/**WebDOMEvent.cpp#newcode49<https://codereview.chromium.org/15271012/diff/33001/Source/WebKit/chromium/src/WebDOMEvent.cpp#newcode49>
> Source/WebKit/chromium/src/**WebDOMEvent.cpp:49: }
> On 2013/05/22 18:06:23, abarth wrote:
>
>> On 2013/05/22 17:37:33, dmichael wrote:
>> > On 2013/05/22 17:16:11, abarth wrote:
>> > > Is there some reason you don't want to use the same pattern we use
>>
> for
>
>> > WebNode?
>> > > There we put the copy constructor inline in the header and
>>
> implement it
>
>> using
>> > > "assign"
>> >
>> > This copy constructor has to have WEBKIT_EXPORT, so can't be
>>
> inlined. I could
>
>> > use assign(), but it's usually considered best practice to use the
>>
> initializer
>
>> > list when you can (rather than allow m_private to be default
>>
> initialized, then
>
>> > assign to it). But that's pretty minor, so I'm happy to change  it
>>
> if you
>
>> want.
>>
>
>  If you move the copy constructor inline in the header, then it won't
>>
> need
>
>> WEBKIT_EXPORT because the caller will inline it.
>>
>
>  I don't fully understand why we prefer to inline constructors and
>>
> export assign
>
>> functions instead.  We could ask Darin, but I don't think there's any
>>
> harm in
>
>> continuing the pattern.
>>
> Fine with me; consistency is valuable. Done.
>
> https://codereview.chromium.**org/15271012/diff/33001/**
>
public/platform/WebPrivatePtr.**h<https://codereview.chromium.org/15271012/diff/33001/public/platform/WebPrivatePtr.h>
> File public/platform/WebPrivatePtr.**h (right):
>
> https://codereview.chromium.**org/15271012/diff/33001/**
>
public/platform/WebPrivatePtr.**h#newcode59<https://codereview.chromium.org/15271012/diff/33001/public/platform/WebPrivatePtr.h#newcode59>
> public/platform/WebPrivatePtr.**h:59: }
> On 2013/05/22 18:06:23, abarth wrote:
>
>> On 2013/05/22 17:37:33, dmichael wrote:
>> > On 2013/05/22 17:16:11, abarth wrote:
>> > > We shouldn't need to modify this class.
>> >
>> > See the updated CL description. WebPrivatePtr did not define a copy
>> constructor,
>> > so the compiler-provided version was being used, and it didn't
>>
> handle the
>
>> > reference count. We can certainly avoid using it in this CL (using
>>
> assign() as
>
>> > you suggested in WebDOMEvent), but if we don't fix this, it will
>>
> bite somebody
>
>> > else. We *at least* need to disable the copy ctor. I think it's
>>
> better to just
>
>> > make it work; I don't see a downside.
>> >
>> > Side note that WebSpeechInputRequest, when used from chromium or
>> DumpRenderTree,
>> > was using the compiler-provided assignment operator (since the real
>> > implementation is WEBKIT_IMPLEMENTATION only). So disabling that
>>
> below seems
>
>> > appropriate, too.
>>
>
>  Good point.  Let's try disabling the copy constructor.
>>
> Done. I kind of liked fixing it better; I wasn't sure how to comment on
> a copy ctor that's disabled, for a class that really looks like it
> should have a copy constructor. I think it will be confusing to the next
> person who tries to use it  (Usual practice is the "rule of three":
>
http://www.drdobbs.com/c-made-**easier-the-rule-of-three/**184401400<http://w...).
> Though the Google style guide isn't clear on the matter.
> "If your class needs to be copyable, prefer providing a copy method,
> such as CopyFrom() or Clone(), rather than a copy constructor, because
> such methods cannot be invoked implicitly. If a copy method is
> insufficient in your situation (e.g. for performance reasons, or because
> your class needs to be stored by value in an STL container), provide
> both a copy constructor and assignment operator."
> http://google-styleguide.**googlecode.com/svn/trunk/**
>
cppguide.xml#Copy_Constructors<http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml#Copy_Constructors>
>
> ...and WebKit/Blink style guides are silent on the matter.
>
>
https://codereview.chromium.**org/15271012/<https://codereview.chromium.org/1...
>

[darin1, 2013-05-22 19:03:48]

Plus operator= and copy ctor can both be implemented in terms of assign.
That avoids another export.

Note:  It was probably premature optimization to worry about number of
exports.  In the early days of the API, I considered the possibility of
someone shipping this API as a webkit DLL.  That has since become an
anti-usecase.
On May 22, 2013 11:58 AM, "Darin Fisher" <darin@google.com> wrote:

> Sorry for the awkwardness.  This system is designed for: 1) making WebFoo
> classes act like RefPtr, 2) enabling you to write less code.  Just mimic
> the existing pattern FTW.
>
> (I chose not to export ctors b/c some systems generate multiple DLL entry
> points when you export a single ctor, to distinguish new WebFoo and new
> WebFoo[n]… or something like that).
> On May 22, 2013 11:44 AM, <dmichael@chromium.org> wrote:
>
>>
>> https://codereview.chromium.**org/15271012/diff/33001/**
>>
Source/WebKit/chromium/src/**WebDOMEvent.cpp<https://codereview.chromium.org/15271012/diff/33001/Source/WebKit/chromium/src/WebDOMEvent.cpp>
>> File Source/WebKit/chromium/src/**WebDOMEvent.cpp (right):
>>
>> https://codereview.chromium.**org/15271012/diff/33001/**
>>
Source/WebKit/chromium/src/**WebDOMEvent.cpp#newcode49<https://codereview.chromium.org/15271012/diff/33001/Source/WebKit/chromium/src/WebDOMEvent.cpp#newcode49>
>> Source/WebKit/chromium/src/**WebDOMEvent.cpp:49: }
>> On 2013/05/22 18:06:23, abarth wrote:
>>
>>> On 2013/05/22 17:37:33, dmichael wrote:
>>> > On 2013/05/22 17:16:11, abarth wrote:
>>> > > Is there some reason you don't want to use the same pattern we use
>>>
>> for
>>
>>> > WebNode?
>>> > > There we put the copy constructor inline in the header and
>>>
>> implement it
>>
>>> using
>>> > > "assign"
>>> >
>>> > This copy constructor has to have WEBKIT_EXPORT, so can't be
>>>
>> inlined. I could
>>
>>> > use assign(), but it's usually considered best practice to use the
>>>
>> initializer
>>
>>> > list when you can (rather than allow m_private to be default
>>>
>> initialized, then
>>
>>> > assign to it). But that's pretty minor, so I'm happy to change  it
>>>
>> if you
>>
>>> want.
>>>
>>
>>  If you move the copy constructor inline in the header, then it won't
>>>
>> need
>>
>>> WEBKIT_EXPORT because the caller will inline it.
>>>
>>
>>  I don't fully understand why we prefer to inline constructors and
>>>
>> export assign
>>
>>> functions instead.  We could ask Darin, but I don't think there's any
>>>
>> harm in
>>
>>> continuing the pattern.
>>>
>> Fine with me; consistency is valuable. Done.
>>
>> https://codereview.chromium.**org/15271012/diff/33001/**
>>
public/platform/WebPrivatePtr.**h<https://codereview.chromium.org/15271012/diff/33001/public/platform/WebPrivatePtr.h>
>> File public/platform/WebPrivatePtr.**h (right):
>>
>> https://codereview.chromium.**org/15271012/diff/33001/**
>>
public/platform/WebPrivatePtr.**h#newcode59<https://codereview.chromium.org/15271012/diff/33001/public/platform/WebPrivatePtr.h#newcode59>
>> public/platform/WebPrivatePtr.**h:59: }
>> On 2013/05/22 18:06:23, abarth wrote:
>>
>>> On 2013/05/22 17:37:33, dmichael wrote:
>>> > On 2013/05/22 17:16:11, abarth wrote:
>>> > > We shouldn't need to modify this class.
>>> >
>>> > See the updated CL description. WebPrivatePtr did not define a copy
>>> constructor,
>>> > so the compiler-provided version was being used, and it didn't
>>>
>> handle the
>>
>>> > reference count. We can certainly avoid using it in this CL (using
>>>
>> assign() as
>>
>>> > you suggested in WebDOMEvent), but if we don't fix this, it will
>>>
>> bite somebody
>>
>>> > else. We *at least* need to disable the copy ctor. I think it's
>>>
>> better to just
>>
>>> > make it work; I don't see a downside.
>>> >
>>> > Side note that WebSpeechInputRequest, when used from chromium or
>>> DumpRenderTree,
>>> > was using the compiler-provided assignment operator (since the real
>>> > implementation is WEBKIT_IMPLEMENTATION only). So disabling that
>>>
>> below seems
>>
>>> > appropriate, too.
>>>
>>
>>  Good point.  Let's try disabling the copy constructor.
>>>
>> Done. I kind of liked fixing it better; I wasn't sure how to comment on
>> a copy ctor that's disabled, for a class that really looks like it
>> should have a copy constructor. I think it will be confusing to the next
>> person who tries to use it  (Usual practice is the "rule of three":
>>
http://www.drdobbs.com/c-made-**easier-the-rule-of-three/**184401400<http://w...).
>> Though the Google style guide isn't clear on the matter.
>> "If your class needs to be copyable, prefer providing a copy method,
>> such as CopyFrom() or Clone(), rather than a copy constructor, because
>> such methods cannot be invoked implicitly. If a copy method is
>> insufficient in your situation (e.g. for performance reasons, or because
>> your class needs to be stored by value in an STL container), provide
>> both a copy constructor and assignment operator."
>> http://google-styleguide.**googlecode.com/svn/trunk/**
>>
cppguide.xml#Copy_Constructors<http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml#Copy_Constructors>
>>
>> ...and WebKit/Blink style guides are silent on the matter.
>>
>>
https://codereview.chromium.**org/15271012/<https://codereview.chromium.org/1...
>>
>

[dmichael (off chromium), 2013-05-22 19:36:58]

On Wed, May 22, 2013 at 1:03 PM, Darin Fisher <darin@google.com> wrote:

> Plus operator= and copy ctor can both be implemented in terms of assign.
> That avoids another export.
>
I buy that. Neither option is clearly better to me, so I'll side with
consistency.

Do you have any opinions about having a copy constructor in WebPrivatePtr?
The current state prior to this patch is that there's a compiler provided
one that does the wrong thing; clearly that should be fixed. I was
originally adding one that [I think] does the right thing w.r.t. reference
counting. But another option (which Adam prefers) is to simply disable it.
It's just very unusual (and possibly confusing) for a class to provide a
custom operator=, a custom destructor, and no copy ctor. From an API
standpoint, I prefer adding a correct copy ctor. (But definitely the most
important thing is to not the incorrect compiler-provided version)

 Note:  It was probably premature optimization to worry about number of
> exports.  In the early days of the API, I considered the possibility of
> someone shipping this API as a webkit DLL.  That has since become an
> anti-usecase.
> On May 22, 2013 11:58 AM, "Darin Fisher" <darin@google.com> wrote:
>
>> Sorry for the awkwardness.  This system is designed for: 1) making WebFoo
>> classes act like RefPtr, 2) enabling you to write less code.  Just mimic
>> the existing pattern FTW.
>>
>> (I chose not to export ctors b/c some systems generate multiple DLL entry
>> points when you export a single ctor, to distinguish new WebFoo and new
>> WebFoo[n]… or something like that).
>> On May 22, 2013 11:44 AM, <dmichael@chromium.org> wrote:
>>
>>>
>>> https://codereview.chromium.**org/15271012/diff/33001/**
>>>
Source/WebKit/chromium/src/**WebDOMEvent.cpp<https://codereview.chromium.org/15271012/diff/33001/Source/WebKit/chromium/src/WebDOMEvent.cpp>
>>> File Source/WebKit/chromium/src/**WebDOMEvent.cpp (right):
>>>
>>> https://codereview.chromium.**org/15271012/diff/33001/**
>>>
Source/WebKit/chromium/src/**WebDOMEvent.cpp#newcode49<https://codereview.chromium.org/15271012/diff/33001/Source/WebKit/chromium/src/WebDOMEvent.cpp#newcode49>
>>> Source/WebKit/chromium/src/**WebDOMEvent.cpp:49: }
>>> On 2013/05/22 18:06:23, abarth wrote:
>>>
>>>> On 2013/05/22 17:37:33, dmichael wrote:
>>>> > On 2013/05/22 17:16:11, abarth wrote:
>>>> > > Is there some reason you don't want to use the same pattern we use
>>>>
>>> for
>>>
>>>> > WebNode?
>>>> > > There we put the copy constructor inline in the header and
>>>>
>>> implement it
>>>
>>>> using
>>>> > > "assign"
>>>> >
>>>> > This copy constructor has to have WEBKIT_EXPORT, so can't be
>>>>
>>> inlined. I could
>>>
>>>> > use assign(), but it's usually considered best practice to use the
>>>>
>>> initializer
>>>
>>>> > list when you can (rather than allow m_private to be default
>>>>
>>> initialized, then
>>>
>>>> > assign to it). But that's pretty minor, so I'm happy to change  it
>>>>
>>> if you
>>>
>>>> want.
>>>>
>>>
>>>  If you move the copy constructor inline in the header, then it won't
>>>>
>>> need
>>>
>>>> WEBKIT_EXPORT because the caller will inline it.
>>>>
>>>
>>>  I don't fully understand why we prefer to inline constructors and
>>>>
>>> export assign
>>>
>>>> functions instead.  We could ask Darin, but I don't think there's any
>>>>
>>> harm in
>>>
>>>> continuing the pattern.
>>>>
>>> Fine with me; consistency is valuable. Done.
>>>
>>> https://codereview.chromium.**org/15271012/diff/33001/**
>>>
public/platform/WebPrivatePtr.**h<https://codereview.chromium.org/15271012/diff/33001/public/platform/WebPrivatePtr.h>
>>> File public/platform/WebPrivatePtr.**h (right):
>>>
>>> https://codereview.chromium.**org/15271012/diff/33001/**
>>>
public/platform/WebPrivatePtr.**h#newcode59<https://codereview.chromium.org/15271012/diff/33001/public/platform/WebPrivatePtr.h#newcode59>
>>> public/platform/WebPrivatePtr.**h:59: }
>>> On 2013/05/22 18:06:23, abarth wrote:
>>>
>>>> On 2013/05/22 17:37:33, dmichael wrote:
>>>> > On 2013/05/22 17:16:11, abarth wrote:
>>>> > > We shouldn't need to modify this class.
>>>> >
>>>> > See the updated CL description. WebPrivatePtr did not define a copy
>>>> constructor,
>>>> > so the compiler-provided version was being used, and it didn't
>>>>
>>> handle the
>>>
>>>> > reference count. We can certainly avoid using it in this CL (using
>>>>
>>> assign() as
>>>
>>>> > you suggested in WebDOMEvent), but if we don't fix this, it will
>>>>
>>> bite somebody
>>>
>>>> > else. We *at least* need to disable the copy ctor. I think it's
>>>>
>>> better to just
>>>
>>>> > make it work; I don't see a downside.
>>>> >
>>>> > Side note that WebSpeechInputRequest, when used from chromium or
>>>> DumpRenderTree,
>>>> > was using the compiler-provided assignment operator (since the real
>>>> > implementation is WEBKIT_IMPLEMENTATION only). So disabling that
>>>>
>>> below seems
>>>
>>>> > appropriate, too.
>>>>
>>>
>>>  Good point.  Let's try disabling the copy constructor.
>>>>
>>> Done. I kind of liked fixing it better; I wasn't sure how to comment on
>>> a copy ctor that's disabled, for a class that really looks like it
>>> should have a copy constructor. I think it will be confusing to the next
>>> person who tries to use it  (Usual practice is the "rule of three":
>>>
http://www.drdobbs.com/c-made-**easier-the-rule-of-three/**184401400<http://w...).
>>> Though the Google style guide isn't clear on the matter.
>>> "If your class needs to be copyable, prefer providing a copy method,
>>> such as CopyFrom() or Clone(), rather than a copy constructor, because
>>> such methods cannot be invoked implicitly. If a copy method is
>>> insufficient in your situation (e.g. for performance reasons, or because
>>> your class needs to be stored by value in an STL container), provide
>>> both a copy constructor and assignment operator."
>>> http://google-styleguide.**googlecode.com/svn/trunk/**
>>>
cppguide.xml#Copy_Constructors<http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml#Copy_Constructors>
>>>
>>> ...and WebKit/Blink style guides are silent on the matter.
>>>
>>>
https://codereview.chromium.**org/15271012/<https://codereview.chromium.org/1...
>>>
>>

[darin1, 2013-05-22 20:08:25]

That seems like a reasonable change to WebPrivatePtr.  WebPrivatePtr was
invented to help with folks writing classes that follow the WebNode
pattern.  It is just about helping us type less code and follow the
pattern.  It seems like your changes help avoid mistakes and encourage
following the pattern.  SGTM.


On Wed, May 22, 2013 at 12:36 PM, David Michael <dmichael@chromium.org>wrote:

> On Wed, May 22, 2013 at 1:03 PM, Darin Fisher <darin@google.com> wrote:
>
>> Plus operator= and copy ctor can both be implemented in terms of assign.
>> That avoids another export.
>>
> I buy that. Neither option is clearly better to me, so I'll side with
> consistency.
>
> Do you have any opinions about having a copy constructor in WebPrivatePtr?
> The current state prior to this patch is that there's a compiler provided
> one that does the wrong thing; clearly that should be fixed. I was
> originally adding one that [I think] does the right thing w.r.t. reference
> counting. But another option (which Adam prefers) is to simply disable it.
> It's just very unusual (and possibly confusing) for a class to provide a
> custom operator=, a custom destructor, and no copy ctor. From an API
> standpoint, I prefer adding a correct copy ctor. (But definitely the most
> important thing is to not the incorrect compiler-provided version)
>
>  Note:  It was probably premature optimization to worry about number of
>> exports.  In the early days of the API, I considered the possibility of
>> someone shipping this API as a webkit DLL.  That has since become an
>> anti-usecase.
>>  On May 22, 2013 11:58 AM, "Darin Fisher" <darin@google.com> wrote:
>>
>>> Sorry for the awkwardness.  This system is designed for: 1) making
>>> WebFoo classes act like RefPtr, 2) enabling you to write less code.  Just
>>> mimic the existing pattern FTW.
>>>
>>> (I chose not to export ctors b/c some systems generate multiple DLL
>>> entry points when you export a single ctor, to distinguish new WebFoo and
>>> new WebFoo[n]… or something like that).
>>> On May 22, 2013 11:44 AM, <dmichael@chromium.org> wrote:
>>>
>>>>
>>>> https://codereview.chromium.**org/15271012/diff/33001/**
>>>>
Source/WebKit/chromium/src/**WebDOMEvent.cpp<https://codereview.chromium.org/15271012/diff/33001/Source/WebKit/chromium/src/WebDOMEvent.cpp>
>>>> File Source/WebKit/chromium/src/**WebDOMEvent.cpp (right):
>>>>
>>>> https://codereview.chromium.**org/15271012/diff/33001/**
>>>>
Source/WebKit/chromium/src/**WebDOMEvent.cpp#newcode49<https://codereview.chromium.org/15271012/diff/33001/Source/WebKit/chromium/src/WebDOMEvent.cpp#newcode49>
>>>> Source/WebKit/chromium/src/**WebDOMEvent.cpp:49: }
>>>> On 2013/05/22 18:06:23, abarth wrote:
>>>>
>>>>> On 2013/05/22 17:37:33, dmichael wrote:
>>>>> > On 2013/05/22 17:16:11, abarth wrote:
>>>>> > > Is there some reason you don't want to use the same pattern we use
>>>>>
>>>> for
>>>>
>>>>> > WebNode?
>>>>> > > There we put the copy constructor inline in the header and
>>>>>
>>>> implement it
>>>>
>>>>> using
>>>>> > > "assign"
>>>>> >
>>>>> > This copy constructor has to have WEBKIT_EXPORT, so can't be
>>>>>
>>>> inlined. I could
>>>>
>>>>> > use assign(), but it's usually considered best practice to use the
>>>>>
>>>> initializer
>>>>
>>>>> > list when you can (rather than allow m_private to be default
>>>>>
>>>> initialized, then
>>>>
>>>>> > assign to it). But that's pretty minor, so I'm happy to change  it
>>>>>
>>>> if you
>>>>
>>>>> want.
>>>>>
>>>>
>>>>  If you move the copy constructor inline in the header, then it won't
>>>>>
>>>> need
>>>>
>>>>> WEBKIT_EXPORT because the caller will inline it.
>>>>>
>>>>
>>>>  I don't fully understand why we prefer to inline constructors and
>>>>>
>>>> export assign
>>>>
>>>>> functions instead.  We could ask Darin, but I don't think there's any
>>>>>
>>>> harm in
>>>>
>>>>> continuing the pattern.
>>>>>
>>>> Fine with me; consistency is valuable. Done.
>>>>
>>>> https://codereview.chromium.**org/15271012/diff/33001/**
>>>>
public/platform/WebPrivatePtr.**h<https://codereview.chromium.org/15271012/diff/33001/public/platform/WebPrivatePtr.h>
>>>> File public/platform/WebPrivatePtr.**h (right):
>>>>
>>>> https://codereview.chromium.**org/15271012/diff/33001/**
>>>>
public/platform/WebPrivatePtr.**h#newcode59<https://codereview.chromium.org/15271012/diff/33001/public/platform/WebPrivatePtr.h#newcode59>
>>>> public/platform/WebPrivatePtr.**h:59: }
>>>> On 2013/05/22 18:06:23, abarth wrote:
>>>>
>>>>> On 2013/05/22 17:37:33, dmichael wrote:
>>>>> > On 2013/05/22 17:16:11, abarth wrote:
>>>>> > > We shouldn't need to modify this class.
>>>>> >
>>>>> > See the updated CL description. WebPrivatePtr did not define a copy
>>>>> constructor,
>>>>> > so the compiler-provided version was being used, and it didn't
>>>>>
>>>> handle the
>>>>
>>>>> > reference count. We can certainly avoid using it in this CL (using
>>>>>
>>>> assign() as
>>>>
>>>>> > you suggested in WebDOMEvent), but if we don't fix this, it will
>>>>>
>>>> bite somebody
>>>>
>>>>> > else. We *at least* need to disable the copy ctor. I think it's
>>>>>
>>>> better to just
>>>>
>>>>> > make it work; I don't see a downside.
>>>>> >
>>>>> > Side note that WebSpeechInputRequest, when used from chromium or
>>>>> DumpRenderTree,
>>>>> > was using the compiler-provided assignment operator (since the real
>>>>> > implementation is WEBKIT_IMPLEMENTATION only). So disabling that
>>>>>
>>>> below seems
>>>>
>>>>> > appropriate, too.
>>>>>
>>>>
>>>>  Good point.  Let's try disabling the copy constructor.
>>>>>
>>>> Done. I kind of liked fixing it better; I wasn't sure how to comment on
>>>> a copy ctor that's disabled, for a class that really looks like it
>>>> should have a copy constructor. I think it will be confusing to the next
>>>> person who tries to use it  (Usual practice is the "rule of three":
>>>>
http://www.drdobbs.com/c-made-**easier-the-rule-of-three/**184401400<http://w...).
>>>> Though the Google style guide isn't clear on the matter.
>>>> "If your class needs to be copyable, prefer providing a copy method,
>>>> such as CopyFrom() or Clone(), rather than a copy constructor, because
>>>> such methods cannot be invoked implicitly. If a copy method is
>>>> insufficient in your situation (e.g. for performance reasons, or because
>>>> your class needs to be stored by value in an STL container), provide
>>>> both a copy constructor and assignment operator."
>>>> http://google-styleguide.**googlecode.com/svn/trunk/**
>>>>
cppguide.xml#Copy_Constructors<http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml#Copy_Constructors>
>>>>
>>>> ...and WebKit/Blink style guides are silent on the matter.
>>>>
>>>>
https://codereview.chromium.**org/15271012/<https://codereview.chromium.org/1...
>>>>
>>>
>

[dmichael (off chromium), 2013-05-22 20:17:38]

Given that you want to encourage following the pattern (in particular
implementing copy ctors using assign()), I agree with Adam's preference of
disabling the copy ctor of WebPrivatePtr. I revised the comment to try to help
encourage that pattern.

PTAL.

[darin (slow to review), 2013-05-22 20:28:22]

LGTM

https://codereview.chromium.org/15271012/diff/29004/public/platform/WebPrivat...
File public/platform/WebPrivatePtr.h (right):

https://codereview.chromium.org/15271012/diff/29004/public/platform/WebPrivat...
public/platform/WebPrivatePtr.h:44: // wrap a reference counted WebCore class.
optional:  it might be nice to provide sample code of how to properly use
WebPrivatePtr.

https://codereview.chromium.org/15271012/diff/29004/public/platform/WebPrivat...
public/platform/WebPrivatePtr.h:101: // Disable assign; we define it above for
when WEBKIT_IMPLEMENTATION is set,
nit: confusing to call this operator "assign" since there is an assign method. 
How about calling this "assignment operator" instead?

https://codereview.chromium.org/15271012/diff/29004/public/platform/WebPrivat...
public/platform/WebPrivatePtr.h:106: // Disable copy; classes that contain a
WebPrivatePtr should implement their
ditto: copy -> copy constructor

[dmichael (off chromium), 2013-05-22 20:54:29]

https://codereview.chromium.org/15271012/diff/29004/public/platform/WebPrivat...
File public/platform/WebPrivatePtr.h (right):

https://codereview.chromium.org/15271012/diff/29004/public/platform/WebPrivat...
public/platform/WebPrivatePtr.h:44: // wrap a reference counted WebCore class.
On 2013/05/22 20:28:22, darin wrote:
> optional:  it might be nice to provide sample code of how to properly use
> WebPrivatePtr.
Good suggestion. Done.

https://codereview.chromium.org/15271012/diff/29004/public/platform/WebPrivat...
public/platform/WebPrivatePtr.h:101: // Disable assign; we define it above for
when WEBKIT_IMPLEMENTATION is set,
On 2013/05/22 20:28:22, darin wrote:
> nit: confusing to call this operator "assign" since there is an assign method.

> How about calling this "assignment operator" instead?
Good point, done.

https://codereview.chromium.org/15271012/diff/29004/public/platform/WebPrivat...
public/platform/WebPrivatePtr.h:106: // Disable copy; classes that contain a
WebPrivatePtr should implement their
On 2013/05/22 20:28:22, darin wrote:
> ditto: copy -> copy constructor

Done.

[commit-bot: I haz the power, 2013-05-22 21:24:41]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/dmichael@chromium.org/15271012/33007

[commit-bot: I haz the power, 2013-05-22 23:47:59]

Change committed as 150933