Make WebPrivatePtr capable of wrapping garbage collected objects.

Make WebPrivatePtr capable of wrapping garbage collected objects.

Extend WebPrivatePtr to let it safely hold on to garbage collected
objects, via a strong, persistent object reference.

As an example of use, adopt it for the Speech API platform layer
objects.

The implementation of WebPrivatPtr is courtesy of Vyacheslav Egorov <vegorov@chromium.org>;

R=haraken@chromium.org,ager@chromium.org,jochen@chromium.org,tkent@chromium.org
BUG=340522
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=167391

[sof, 2014-02-17 10:49:08]

Please take a look.

We're a bit blocked on how to handle platform layer references to Oilpan
objects. This is an attempt to generalize WebPrivatePtr to accommodate such.

Note:

 - I didn't see how to reduce the #ifdef'ery.
 - It dynamically allocates a Persistent<> wrapper.

[haraken, 2014-02-17 11:11:22]

The change looks good. I want to have one more reviewer.

sof@: Would you add one use case of WebPrivateGarbageCollectedPtr to this CL? We
don't want to land code that is not yet used.

jochen@: What's the difference between INSIDE_BLINK and BLINK_IMPLEMENTATION? Is
it a right thing to use INSIDE_BLINK in this change?

[sof, 2014-02-17 11:34:48]

On 2014/02/17 11:11:22, haraken wrote:
> The change looks good. I want to have one more reviewer.
> 
> sof@: Would you add one use case of WebPrivateGarbageCollectedPtr to this CL?
We
> don't want to land code that is not yet used.
> 

Done, for WebSpeechGrammar.

[haraken, 2014-02-17 11:35:58]

LGTM

https://codereview.chromium.org/168963003/diff/40001/public/web/WebSpeechGram...
File public/web/WebSpeechGrammar.h (right):

https://codereview.chromium.org/168963003/diff/40001/public/web/WebSpeechGram...
public/web/WebSpeechGrammar.h:52: WebSpeechGrammar(const
WTF::PassRefPtrWillBeRawPtr<WebCore::SpeechGrammar>&);

Add explicit.

https://codereview.chromium.org/168963003/diff/40001/public/web/WebSpeechGram...
public/web/WebSpeechGrammar.h:53: WebSpeechGrammar& operator=(const
WTF::PassRefPtrWillBeRawPtr<WebCore::SpeechGrammar>&);

Ditto.

[sof, 2014-02-17 11:36:25]

https://codereview.chromium.org/168963003/diff/40001/public/web/WebSpeechGram...
File public/web/WebSpeechGrammar.h (right):

https://codereview.chromium.org/168963003/diff/40001/public/web/WebSpeechGram...
public/web/WebSpeechGrammar.h:30: #include
"../platform/WebPrivateGarbageCollectedPtr.h"
checkdeps didn't like this #include, I don't yet understand why.

Needs to be addressed.

[sof, 2014-02-17 11:44:18]

https://codereview.chromium.org/168963003/diff/40001/public/web/WebSpeechGram...
File public/web/WebSpeechGrammar.h (right):

https://codereview.chromium.org/168963003/diff/40001/public/web/WebSpeechGram...
public/web/WebSpeechGrammar.h:52: WebSpeechGrammar(const
WTF::PassRefPtrWillBeRawPtr<WebCore::SpeechGrammar>&);
On 2014/02/17 11:35:58, haraken wrote:
> 
> Add explicit.

Done.

https://codereview.chromium.org/168963003/diff/40001/public/web/WebSpeechGram...
public/web/WebSpeechGrammar.h:53: WebSpeechGrammar& operator=(const
WTF::PassRefPtrWillBeRawPtr<WebCore::SpeechGrammar>&);
On 2014/02/17 11:35:58, haraken wrote:
> 
> Ditto.

Need to wait for C++11? (Or do we have a conditional macro for utilizing its
features?)

[zerny-chromium, 2014-02-17 11:45:08]

https://codereview.chromium.org/168963003/diff/40001/public/platform/WebPriva...
File public/platform/WebPrivateGarbageCollectedPtr.h (right):

https://codereview.chromium.org/168963003/diff/40001/public/platform/WebPriva...
public/platform/WebPrivateGarbageCollectedPtr.h:171: T* m_ptr;
Could we simplify this code by changing T* to RawPtrWillBePersistent<T> ?

That would give use the same interface for accessing T and use the existing
infrastructure for selecting the interpretation.

[haraken, 2014-02-17 11:45:16]

https://codereview.chromium.org/168963003/diff/40001/public/web/WebSpeechGram...
File public/web/WebSpeechGrammar.h (right):

https://codereview.chromium.org/168963003/diff/40001/public/web/WebSpeechGram...
public/web/WebSpeechGrammar.h:53: WebSpeechGrammar& operator=(const
WTF::PassRefPtrWillBeRawPtr<WebCore::SpeechGrammar>&);
On 2014/02/17 11:44:18, sof wrote:
> On 2014/02/17 11:35:58, haraken wrote:
> > 
> > Ditto.
> 
> Need to wait for C++11? (Or do we have a conditional macro for utilizing its
> features?)

Sorry, ignore my comment :)

[zerny-chromium, 2014-02-17 11:46:24]

On 2014/02/17 11:45:08, zerny-chromium wrote:
>
https://codereview.chromium.org/168963003/diff/40001/public/platform/WebPriva...
> File public/platform/WebPrivateGarbageCollectedPtr.h (right):
> 
>
https://codereview.chromium.org/168963003/diff/40001/public/platform/WebPriva...
> public/platform/WebPrivateGarbageCollectedPtr.h:171: T* m_ptr;
> Could we simplify this code by changing T* to RawPtrWillBePersistent<T> ?
> 
> That would give use the same interface for accessing T and use the existing
> infrastructure for selecting the interpretation.

Wibling asks: why not use a RefPtrWillBePersistent? Do we need the manual
ref/deref here?

[sof, 2014-02-17 11:55:34]

https://codereview.chromium.org/168963003/diff/40001/public/platform/WebPriva...
File public/platform/WebPrivateGarbageCollectedPtr.h (right):

https://codereview.chromium.org/168963003/diff/40001/public/platform/WebPriva...
public/platform/WebPrivateGarbageCollectedPtr.h:171: T* m_ptr;
On 2014/02/17 11:45:08, zerny-chromium wrote:
> Could we simplify this code by changing T* to RawPtrWillBePersistent<T> ?
> 
> That would give use the same interface for accessing T and use the existing
> infrastructure for selecting the interpretation.

We cannot expose Persistent (or RawPtr) on this object. I think.

[Mads Ager (chromium), 2014-02-17 12:26:29]

https://codereview.chromium.org/168963003/diff/170001/public/platform/WebPriv...
File public/platform/WebPrivateGarbageCollectedPtr.h (right):

https://codereview.chromium.org/168963003/diff/170001/public/platform/WebPriv...
public/platform/WebPrivateGarbageCollectedPtr.h:6: #ifndef
WebPrivateGarbageCollectedPtr_h
We had a chat about this just now. We can probably get rid of most of the ifdef
spaghetti here.

The proposal would be to introduce a transition type for this:

WillBeWebPrivateGarbageCollectedPtr

Then in this file we can have:

#if ENABLE(OILPAN)
#define WillBeWebPrivateGarbageCollectedPtr WebPrivateGarbageCollectedPtr
#else
#define WillBeWebPrivateGarbageCollectedPtr WebPrivatePtr
#endif

(and use template aliases for clang to keep us honest)

Then there will be no need for further ifdefs in this file. This file only has
to provide the implementation that uses Persistent and will only be used when
compiling with ENABLE(OILPAN).

https://codereview.chromium.org/168963003/diff/170001/public/platform/WebPriv...
public/platform/WebPrivateGarbageCollectedPtr.h:22: # if ENABLE(OILPAN)
Please remove the spaces between # and if/define/else. Also, please use template
aliases when using clang to keep us honest.

https://codereview.chromium.org/168963003/diff/170001/public/platform/WebPriv...
public/platform/WebPrivateGarbageCollectedPtr.h:145: m_ptr =
reinterpret_cast<T*>(new WebCore::Persistent<T>(p));
We really shouldn't have to do this. We should allocate only one persistent and
then we should just use assignment. We shouldn't have to deallocate the
persistent and allocate a new one. We just need to have that one persistent
point to a new object. So, instead of using reset here, we should cast m_ptr to
a Persistent<T>* and assign to it.

[sof, 2014-02-17 12:45:03]

https://codereview.chromium.org/168963003/diff/170001/public/platform/WebPriv...
File public/platform/WebPrivateGarbageCollectedPtr.h (right):

https://codereview.chromium.org/168963003/diff/170001/public/platform/WebPriv...
public/platform/WebPrivateGarbageCollectedPtr.h:6: #ifndef
WebPrivateGarbageCollectedPtr_h
On 2014/02/17 12:26:29, Mads Ager (chromium) wrote:
> We had a chat about this just now. We can probably get rid of most of the
ifdef
> spaghetti here.
> 
> The proposal would be to introduce a transition type for this:
> 
> WillBeWebPrivateGarbageCollectedPtr
> 
> Then in this file we can have:
> 
> #if ENABLE(OILPAN)
> #define WillBeWebPrivateGarbageCollectedPtr WebPrivateGarbageCollectedPtr
> #else
> #define WillBeWebPrivateGarbageCollectedPtr WebPrivatePtr
> #endif
> 
> (and use template aliases for clang to keep us honest)
> 
> Then there will be no need for further ifdefs in this file. This file only has
> to provide the implementation that uses Persistent and will only be used when
> compiling with ENABLE(OILPAN).

There's a slight problem with that: uses of WebPrivateGarbageCollectedPtr is
compiled outside of Blink (e.g., WebSpeechGrammar.h), hence won't have access to
ENABLE(OILPAN) -- which it needs to in order to resolve its use of the
transition type WillBeWebPrivateGarbageCollectedPtr.

[Mads Ager (chromium), 2014-02-17 12:54:30]

https://codereview.chromium.org/168963003/diff/170001/public/platform/WebPriv...
File public/platform/WebPrivateGarbageCollectedPtr.h (right):

https://codereview.chromium.org/168963003/diff/170001/public/platform/WebPriv...
public/platform/WebPrivateGarbageCollectedPtr.h:6: #ifndef
WebPrivateGarbageCollectedPtr_h
On 2014/02/17 12:45:03, sof wrote:
> On 2014/02/17 12:26:29, Mads Ager (chromium) wrote:
> > We had a chat about this just now. We can probably get rid of most of the
> ifdef
> > spaghetti here.
> > 
> > The proposal would be to introduce a transition type for this:
> > 
> > WillBeWebPrivateGarbageCollectedPtr
> > 
> > Then in this file we can have:
> > 
> > #if ENABLE(OILPAN)
> > #define WillBeWebPrivateGarbageCollectedPtr WebPrivateGarbageCollectedPtr
> > #else
> > #define WillBeWebPrivateGarbageCollectedPtr WebPrivatePtr
> > #endif
> > 
> > (and use template aliases for clang to keep us honest)
> > 
> > Then there will be no need for further ifdefs in this file. This file only
has
> > to provide the implementation that uses Persistent and will only be used
when
> > compiling with ENABLE(OILPAN).
> 
> There's a slight problem with that: uses of WebPrivateGarbageCollectedPtr is
> compiled outside of Blink (e.g., WebSpeechGrammar.h), hence won't have access
to
> ENABLE(OILPAN) -- which it needs to in order to resolve its use of the
> transition type WillBeWebPrivateGarbageCollectedPtr.

Yeah, I see that now. Hmm. I'll give it a bit more thought.

If we need to keep it this way I think we might need another type down the line.
If we want to ship some of these with oilpan and not others we will need a type
that is always a persistent independently of the ENABLE(OILPAN) define.

If we need to keep it this way, maybe we should make it a bit more verbose by
repeating the method signatures instead of having so many ifdefs?

#if INSIDE_BLINK
#if ENABLE(OILPAN)
// all the oilpan code
#else
// all the ref counting code
#endif
#endif

It would duplicate a bit of code but I think it would be easier to reason about
what is going on in the two implementations?

[haraken, 2014-02-17 12:56:46]

> If we need to keep it this way, maybe we should make it a bit more verbose by
> repeating the method signatures instead of having so many ifdefs?
> 
> #if INSIDE_BLINK
> #if ENABLE(OILPAN)
> // all the oilpan code
> #else
> // all the ref counting code
> #endif
> #endif
> 
> It would duplicate a bit of code but I think it would be easier to reason
about
> what is going on in the two implementations?

I think this would be fine since the duplication happens only in
WebPrivateGarbageCollectedPtr.

[Mads Ager (chromium), 2014-02-17 13:31:31]

Yeah, I don't really see any way around doing it this way for now.

A couple of more concrete comments. We should probably call this type
WillBeWebPrivateGarbageCollectedPtr in order to prepare for having
WebPrivateGarbageCollectedPtr which is based on persistents no matter if the
oilpan flag is supplied.

https://codereview.chromium.org/168963003/diff/170001/public/platform/WebPriv...
File public/platform/WebPrivateGarbageCollectedPtr.h (right):

https://codereview.chromium.org/168963003/diff/170001/public/platform/WebPriv...
public/platform/WebPrivateGarbageCollectedPtr.h:14: namespace WebCore {
We should be able to remove all of the forward declarations and the duplicate
definitions of PassRefPtrWillBeRawPtr if we just have:

#if INSIDE_BLINK
#include "wtf/PassRefPtr.h"
#include "heap/Handle.h"
#endif

?

https://codereview.chromium.org/168963003/diff/170001/public/platform/WebPriv...
public/platform/WebPrivateGarbageCollectedPtr.h:145: m_ptr =
reinterpret_cast<T*>(new WebCore::Persistent<T>(p));
On 2014/02/17 12:26:29, Mads Ager (chromium) wrote:
> We really shouldn't have to do this. We should allocate only one persistent
and
> then we should just use assignment. We shouldn't have to deallocate the
> persistent and allocate a new one. We just need to have that one persistent
> point to a new object. So, instead of using reset here, we should cast m_ptr
to
> a Persistent<T>* and assign to it.

In addition to reusing the persistent we should be careful to make this work
with isNull. If we are assigning a null pointer to this one we need to
deallocate the persistent. Currently this code will always allocate a persistent
which will break isNull if assigning null.

[sof, 2014-02-17 14:08:59]

(Apologies for the slower turnaround, takes a while to verify changes.)

The latest patchset (5) doesn't rename the type as
WillBeWebPrivateGarbageCollectedPtr; will do that shortly/next.

https://codereview.chromium.org/168963003/diff/170001/public/platform/WebPriv...
File public/platform/WebPrivateGarbageCollectedPtr.h (right):

https://codereview.chromium.org/168963003/diff/170001/public/platform/WebPriv...
public/platform/WebPrivateGarbageCollectedPtr.h:6: #ifndef
WebPrivateGarbageCollectedPtr_h
On 2014/02/17 12:54:30, Mads Ager (chromium) wrote:
> On 2014/02/17 12:45:03, sof wrote:
> > On 2014/02/17 12:26:29, Mads Ager (chromium) wrote:
> > > We had a chat about this just now. We can probably get rid of most of the
> > ifdef
> > > spaghetti here.
> > > 
> > > The proposal would be to introduce a transition type for this:
> > > 
> > > WillBeWebPrivateGarbageCollectedPtr
> > > 
> > > Then in this file we can have:
> > > 
> > > #if ENABLE(OILPAN)
> > > #define WillBeWebPrivateGarbageCollectedPtr WebPrivateGarbageCollectedPtr
> > > #else
> > > #define WillBeWebPrivateGarbageCollectedPtr WebPrivatePtr
> > > #endif
> > > 
> > > (and use template aliases for clang to keep us honest)
> > > 
> > > Then there will be no need for further ifdefs in this file. This file only
> has
> > > to provide the implementation that uses Persistent and will only be used
> when
> > > compiling with ENABLE(OILPAN).
> > 
> > There's a slight problem with that: uses of WebPrivateGarbageCollectedPtr is
> > compiled outside of Blink (e.g., WebSpeechGrammar.h), hence won't have
access
> to
> > ENABLE(OILPAN) -- which it needs to in order to resolve its use of the
> > transition type WillBeWebPrivateGarbageCollectedPtr.
> 
> Yeah, I see that now. Hmm. I'll give it a bit more thought.
> 
> If we need to keep it this way I think we might need another type down the
line.
> If we want to ship some of these with oilpan and not others we will need a
type
> that is always a persistent independently of the ENABLE(OILPAN) define.
> 
> If we need to keep it this way, maybe we should make it a bit more verbose by
> repeating the method signatures instead of having so many ifdefs?
> 
> #if INSIDE_BLINK
> #if ENABLE(OILPAN)
> // all the oilpan code
> #else
> // all the ref counting code
> #endif
> #endif
> 
> It would duplicate a bit of code but I think it would be easier to reason
about
> what is going on in the two implementations?

I think we get out ahead by doing so, switched over.

https://codereview.chromium.org/168963003/diff/170001/public/platform/WebPriv...
public/platform/WebPrivateGarbageCollectedPtr.h:14: namespace WebCore {
On 2014/02/17 13:31:31, Mads Ager (chromium) wrote:
> We should be able to remove all of the forward declarations and the duplicate
> definitions of PassRefPtrWillBeRawPtr if we just have:
> 
> #if INSIDE_BLINK
> #include "wtf/PassRefPtr.h"
> #include "heap/Handle.h"
> #endif
> 
> ?

That would be cleaner, the reason why it's like this atm is that DEPS don't
allow heap/ to be included here. 

The wish/need to reuse an existing Persistent made the duplication slightly
worse, so I've switched over to a direct include of heap/Handle.h -- might this
be allowed by the platform owners?

https://codereview.chromium.org/168963003/diff/170001/public/platform/WebPriv...
public/platform/WebPrivateGarbageCollectedPtr.h:22: # if ENABLE(OILPAN)
On 2014/02/17 12:26:29, Mads Ager (chromium) wrote:
> Please remove the spaces between # and if/define/else. Also, please use
template
> aliases when using clang to keep us honest.

Done.

https://codereview.chromium.org/168963003/diff/170001/public/platform/WebPriv...
public/platform/WebPrivateGarbageCollectedPtr.h:145: m_ptr =
reinterpret_cast<T*>(new WebCore::Persistent<T>(p));
On 2014/02/17 13:31:31, Mads Ager (chromium) wrote:
> On 2014/02/17 12:26:29, Mads Ager (chromium) wrote:
> > We really shouldn't have to do this. We should allocate only one persistent
> and
> > then we should just use assignment. We shouldn't have to deallocate the
> > persistent and allocate a new one. We just need to have that one persistent
> > point to a new object. So, instead of using reset here, we should cast m_ptr
> to
> > a Persistent<T>* and assign to it.
> 
> In addition to reusing the persistent we should be careful to make this work
> with isNull. If we are assigning a null pointer to this one we need to
> deallocate the persistent. Currently this code will always allocate a
persistent
> which will break isNull if assigning null. 

Oh yes, it completely failed to consider the null case. Addressed now + reusing
the persistent in assign().

[sof, 2014-02-17 15:17:21]

On 2014/02/17 14:08:59, sof wrote:
> (Apologies for the slower turnaround, takes a while to verify changes.)
> 
> The latest patchset (5) doesn't rename the type as
> WillBeWebPrivateGarbageCollectedPtr; will do that shortly/next.
> 

Clarification sought: how would this always-persistent type
be different from WebPrivatePtr in a non-oilpan setting?

[Mads Ager (chromium), 2014-02-17 15:18:40]

This is getting there. You are hitting the first use of our transition types
outside of the WebCore namespace. I think there is something fishy here. I don't
think using WTF::BlahWillBeOtherBlah should work on clang where this is a
template alias. I hadn't thought of that before. Maybe that just means that our
transition types need to explicitly state the namespace for the alias and then
we never use the namespace when using the transition types.

https://codereview.chromium.org/168963003/diff/260001/Source/web/WebSpeechGra...
File Source/web/WebSpeechGrammar.cpp (right):

https://codereview.chromium.org/168963003/diff/260001/Source/web/WebSpeechGra...
Source/web/WebSpeechGrammar.cpp:49: WebSpeechGrammar&
WebSpeechGrammar::operator=(const
WTF::PassRefPtrWillBeRawPtr<WebCore::SpeechGrammar>& value)
I think the PassRefPtrWillBeRawPtr is in WebCore and not in WTF? Am I missing
something?

https://codereview.chromium.org/168963003/diff/260001/public/platform/WebPriv...
File public/platform/WebPrivateGarbageCollectedPtr.h (right):

https://codereview.chromium.org/168963003/diff/260001/public/platform/WebPriv...
public/platform/WebPrivateGarbageCollectedPtr.h:55:
WebPrivateGarbageCollectedPtr(const WTF::PassRefPtrWillBeRawPtr<T>& p)
I think we have the transition types in the WebCore namespace? Are you compiling
with GCC or with clang? I'm surprised this works but maybe that is because of
the use of #define on GCC. I don't think this will work with clang. Maybe we
need to change the defines a bit so that they use explicit namespaces so that
you don't have to write WTF:: here but can just use PassRefPtrWillBeRawPtr which
will be an alias for WTF::PassRefPtr or WTF::RawPtr depending on how we compile.

Anyway, we should verify that this compiles with both clang and gcc.

https://codereview.chromium.org/168963003/diff/260001/public/platform/WebPriv...
public/platform/WebPrivateGarbageCollectedPtr.h:140: // Keep an opaque pointer
object pointer to avoid exposing
Too many occurrences of pointer? :)

[sof, 2014-02-17 15:19:10]

https://codereview.chromium.org/168963003/diff/310001/public/platform/WebPriv...
File public/platform/WebPrivateGarbageCollectedPtr.h (right):

https://codereview.chromium.org/168963003/diff/310001/public/platform/WebPriv...
public/platform/WebPrivateGarbageCollectedPtr.h:15: // FIXME: oilpan: If the
WebCore-qualified PassRefPtrWillBeRawPtr
Addressing some clang compilation breakage..referring to transition types from
outside of the WebCore namespace is a bit of a pain, as the names may refer to
(#defined) names that live in WTF. How about having such a "namespace
translation" block in Handle.h instead (next to the transition type #defines)?

(See also https://codereview.chromium.org/164713002/ , where this issue also
surfaced.)

[Mads Ager (chromium), 2014-02-17 15:23:35]

https://codereview.chromium.org/168963003/diff/310001/public/platform/WebPriv...
File public/platform/WebPrivateGarbageCollectedPtr.h (right):

https://codereview.chromium.org/168963003/diff/310001/public/platform/WebPriv...
public/platform/WebPrivateGarbageCollectedPtr.h:15: // FIXME: oilpan: If the
WebCore-qualified PassRefPtrWillBeRawPtr
On 2014/02/17 15:19:10, sof wrote:
> Addressing some clang compilation breakage..referring to transition types from
> outside of the WebCore namespace is a bit of a pain, as the names may refer to
> (#defined) names that live in WTF. How about having such a "namespace
> translation" block in Handle.h instead (next to the transition type #defines)?
> 
> (See also https://codereview.chromium.org/164713002/ , where this issue also
> surfaced.)

How about having the define expand to a fully qualified type with the namespace
always. I think that should remove the need for this at the use-site? I might be
missing something though?

[Mads Ager (chromium), 2014-02-17 15:27:29]

On 2014/02/17 15:17:21, sof wrote:
> On 2014/02/17 14:08:59, sof wrote:
> > (Apologies for the slower turnaround, takes a while to verify changes.)
> > 
> > The latest patchset (5) doesn't rename the type as
> > WillBeWebPrivateGarbageCollectedPtr; will do that shortly/next.
> > 
> 
> Clarification sought: how would this always-persistent type
> be different from WebPrivatePtr in a non-oilpan setting?

There would be three types:

WebPrivatePtr for something where we haven't done any oilpan work (not even
under compile-time flag)
WillBeWebPrivateGCPtr for something where we are using transition types (only in
oilpan heap when enable_oilpan=1)
WebPrivateGCPtr for something that we have decided we are shipping in the oilpan
heap always (disregarding enable_oilpan)

We don't need the last type yet. So the rename would just be so that we don't
have to go through the code and rename a bunch of stuff later if/when we need
it.

[sof, 2014-02-17 15:27:39]

https://codereview.chromium.org/168963003/diff/310001/public/platform/WebPriv...
File public/platform/WebPrivateGarbageCollectedPtr.h (right):

https://codereview.chromium.org/168963003/diff/310001/public/platform/WebPriv...
public/platform/WebPrivateGarbageCollectedPtr.h:15: // FIXME: oilpan: If the
WebCore-qualified PassRefPtrWillBeRawPtr
On 2014/02/17 15:23:35, Mads Ager (chromium) wrote:
> On 2014/02/17 15:19:10, sof wrote:
> > Addressing some clang compilation breakage..referring to transition types
from
> > outside of the WebCore namespace is a bit of a pain, as the names may refer
to
> > (#defined) names that live in WTF. How about having such a "namespace
> > translation" block in Handle.h instead (next to the transition type
#defines)?
> > 
> > (See also https://codereview.chromium.org/164713002/ , where this issue also
> > surfaced.)
> 
> How about having the define expand to a fully qualified type with the
namespace
> always. I think that should remove the need for this at the use-site? I might
be
> missing something though?

I don't see how that would work with template aliases; e.g., in
https://codereview.chromium.org/164713002/, RefPtrWillBeMember<> is not in the
blink namespace, so the name at the point of use will have to be qualified.

[Mads Ager (chromium), 2014-02-17 15:30:14]

On 2014/02/17 15:27:39, sof wrote:
>
https://codereview.chromium.org/168963003/diff/310001/public/platform/WebPriv...
> File public/platform/WebPrivateGarbageCollectedPtr.h (right):
> 
>
https://codereview.chromium.org/168963003/diff/310001/public/platform/WebPriv...
> public/platform/WebPrivateGarbageCollectedPtr.h:15: // FIXME: oilpan: If the
> WebCore-qualified PassRefPtrWillBeRawPtr
> On 2014/02/17 15:23:35, Mads Ager (chromium) wrote:
> > On 2014/02/17 15:19:10, sof wrote:
> > > Addressing some clang compilation breakage..referring to transition types
> from
> > > outside of the WebCore namespace is a bit of a pain, as the names may
refer
> to
> > > (#defined) names that live in WTF. How about having such a "namespace
> > > translation" block in Handle.h instead (next to the transition type
> #defines)?
> > > 
> > > (See also https://codereview.chromium.org/164713002/ , where this issue
also
> > > surfaced.)
> > 
> > How about having the define expand to a fully qualified type with the
> namespace
> > always. I think that should remove the need for this at the use-site? I
might
> be
> > missing something though?
> 
> I don't see how that would work with template aliases; e.g., in
> https://codereview.chromium.org/164713002/, RefPtrWillBeMember<> is not in the
> blink namespace, so the name at the point of use will have to be qualified.

Yeah, I think we are hitting the point where there are just differences between
using defines and using template aliases. That probably means that we should
remove the template aliases and just use the defines (and be careful that we are
only using the defines for simple aliases).

[sof, 2014-02-17 15:32:35]

On 2014/02/17 15:27:29, Mads Ager (chromium) wrote:
...
> There would be three types:
> 
> WebPrivatePtr for something where we haven't done any oilpan work (not even
> under compile-time flag)
> WillBeWebPrivateGCPtr for something where we are using transition types (only
in
> oilpan heap when enable_oilpan=1)
> WebPrivateGCPtr for something that we have decided we are shipping in the
oilpan
> heap always (disregarding enable_oilpan)
> 

Thanks for explaining in detail, interesting last combination.

To avoid running up against the naming scheme in public/, how about
WebWillBePrivateGarbageCollectedPtr<> ?

[Mads Ager (chromium), 2014-02-17 15:34:34]

On 2014/02/17 15:32:35, sof wrote:
> On 2014/02/17 15:27:29, Mads Ager (chromium) wrote:
> ...
> > There would be three types:
> > 
> > WebPrivatePtr for something where we haven't done any oilpan work (not even
> > under compile-time flag)
> > WillBeWebPrivateGCPtr for something where we are using transition types
(only
> in
> > oilpan heap when enable_oilpan=1)
> > WebPrivateGCPtr for something that we have decided we are shipping in the
> oilpan
> > heap always (disregarding enable_oilpan)
> > 
> 
> Thanks for explaining in detail, interesting last combination.
> 
> To avoid running up against the naming scheme in public/, how about
> WebWillBePrivateGarbageCollectedPtr<> ?

SGTM

[Mads Ager (chromium), 2014-02-17 15:37:50]

I think we need to get rid of the template aliases. It was a nice try but it
just doesn't work yet on all compilers so we need to go to the lowest common
denominator.

If you are up for it it would be great if you could do that in this change.
Otherwise, I will be happy to do it as a followup.

https://codereview.chromium.org/168963003/diff/310001/Source/web/WebSpeechGra...
File Source/web/WebSpeechGrammar.cpp (right):

https://codereview.chromium.org/168963003/diff/310001/Source/web/WebSpeechGra...
Source/web/WebSpeechGrammar.cpp:49: WebSpeechGrammar&
WebSpeechGrammar::operator=(const
WTF::PassRefPtrWillBeRawPtr<WebCore::SpeechGrammar>& value)
This probably needs fixing as well?

[sof, 2014-02-17 15:48:37]

On 2014/02/17 15:37:50, Mads Ager (chromium) wrote:
> I think we need to get rid of the template aliases. It was a nice try but it
> just doesn't work yet on all compilers so we need to go to the lowest common
> denominator.
> 
> If you are up for it it would be great if you could do that in this change.
> Otherwise, I will be happy to do it as a followup.
> 

I'll happily take care of that, but prefer to do it separately.

>
https://codereview.chromium.org/168963003/diff/310001/Source/web/WebSpeechGra...
> File Source/web/WebSpeechGrammar.cpp (right):
> 
>
https://codereview.chromium.org/168963003/diff/310001/Source/web/WebSpeechGra...
> Source/web/WebSpeechGrammar.cpp:49: WebSpeechGrammar&
> WebSpeechGrammar::operator=(const
> WTF::PassRefPtrWillBeRawPtr<WebCore::SpeechGrammar>& value)
> This probably needs fixing as well?

Yes, you're faster than a trybot :) (Have the fix sitting in my tree.)

[sof, 2014-02-17 16:02:02]

On 2014/02/17 15:34:34, Mads Ager (chromium) wrote:
> On 2014/02/17 15:32:35, sof wrote:
> > On 2014/02/17 15:27:29, Mads Ager (chromium) wrote:
> > ...
> > > There would be three types:
> > > 
> > > WebPrivatePtr for something where we haven't done any oilpan work (not
even
> > > under compile-time flag)
> > > WillBeWebPrivateGCPtr for something where we are using transition types
> (only
> > in
> > > oilpan heap when enable_oilpan=1)
> > > WebPrivateGCPtr for something that we have decided we are shipping in the
> > oilpan
> > > heap always (disregarding enable_oilpan)
> > > 
> > 
> > Thanks for explaining in detail, interesting last combination.
> > 
> > To avoid running up against the naming scheme in public/, how about
> > WebWillBePrivateGarbageCollectedPtr<> ?
> 
> SGTM

Great, now done.

[sof, 2014-02-17 16:02:15]

A busy CL.. so to summarize for the platform owners, asking the following:

 - Which of INSIDE_BLINK and BLINK_IMPLEMENTATION is preferred?
 - Would it be acceptable to have an #include of "heap/Handle.h" from public/ ?
(From within INSIDE_BLINK/BLINK_IMPLEMENTATION of course.)
 - Similarly, #includes of "../platform/WebWillBePrivateGarbageCollectedPtr.h"
are not permitted from public/web/; is there a better way to refer to it that
doesn't run into checkdeps complaining?

[jochen (gone - plz use gerrit), 2014-02-17 16:12:00]

On 2014/02/17 16:02:15, sof wrote:
> A busy CL.. so to summarize for the platform owners, asking the following:
> 
>  - Which of INSIDE_BLINK and BLINK_IMPLEMENTATION is preferred?

BLINK_IMPLEMENTATION (as that's the one documented in public/README)

>  - Would it be acceptable to have an #include of "heap/Handle.h" from public/
?
> (From within INSIDE_BLINK/BLINK_IMPLEMENTATION of course.)

Will classes in platform/ use this new class? In the current CL, it's class in
web. In that case, it might be preferable to move the pointer to public/web as
well.

>  - Similarly, #includes of "../platform/WebWillBePrivateGarbageCollectedPtr.h"
> are not permitted from public/web/; is there a better way to refer to it that
> doesn't run into checkdeps complaining?

This is a long standing bug :-/

All headers in public/web should use absolute paths to include platform headers.
However, there a bunch of places in chromium that don't set the correct include
paths, so it's a bit painful to fix.

To answer your question, I'd just go with using a relative path for the include
and dcommit the CL once you're done (because the CQ won't let you commit this
with failing presubmit checks)

[sof, 2014-02-17 16:31:33]

On 2014/02/17 16:12:00, jochen wrote:
> On 2014/02/17 16:02:15, sof wrote:
> > A busy CL.. so to summarize for the platform owners, asking the following:
> > 
> >  - Which of INSIDE_BLINK and BLINK_IMPLEMENTATION is preferred?
> 
> BLINK_IMPLEMENTATION (as that's the one documented in public/README)
> 

Thanks, will adjust.

> >  - Would it be acceptable to have an #include of "heap/Handle.h" from
public/
> ?
> > (From within INSIDE_BLINK/BLINK_IMPLEMENTATION of course.)
> 
> Will classes in platform/ use this new class? In the current CL, it's class in
> web. In that case, it might be preferable to move the pointer to public/web as
> well.
> 

I reckon a number could with benefit switch to use it rather than WebPrivatePtr
(e.g., WebCrypto.)

> >  - Similarly, #includes of
"../platform/WebWillBePrivateGarbageCollectedPtr.h"
> > are not permitted from public/web/; is there a better way to refer to it
that
> > doesn't run into checkdeps complaining?
> 
> This is a long standing bug :-/
> 
> All headers in public/web should use absolute paths to include platform
headers.
> However, there a bunch of places in chromium that don't set the correct
include
> paths, so it's a bit painful to fix.
> 
> To answer your question, I'd just go with using a relative path for the
include
> and dcommit the CL once you're done (because the CQ won't let you commit this
> with failing presubmit checks)

ok, will do that if/when we're all ready here.

[Vyacheslav Egorov (Chromium), 2014-02-17 18:14:33]

Have you considered doing something like this: 

https://codereview.chromium.org/169653006/

instead?

[sof, 2014-02-17 18:41:46]

On 2014/02/17 18:14:33, Vyacheslav Egorov wrote:
> Have you considered doing something like this: 
> 
> https://codereview.chromium.org/169653006/
> 
> instead?

My initial attempts were along the line of traits, but without being able to
hide the trait from being parameterized over WebPrivatePtr. You manage to avoid
running into that problem very well; it looks like a pretty solid solution to
me. I can't pick out any problems with it.

[Vyacheslav Egorov (Chromium), 2014-02-17 19:09:37]

There is one problem however, which I did not pay attention before. I
uploaded another version of my CL highlighting the problem. Obviously due
to this it is not usable (we need to alter includes in the files, which is
no go).

In short PtrStorage is instantiated too early and does not see the
definition of the T it refers to so it never picks up garbage collected
variant. Somehow we need to delay instantiation to the moment until after
the definition is seen.

I will think about it more.

Vyacheslav Egorov


On Mon, Feb 17, 2014 at 7:41 PM, <sigbjornf@opera.com> wrote:

> On 2014/02/17 18:14:33, Vyacheslav Egorov wrote:
>
>> Have you considered doing something like this:
>>
>
>  https://codereview.chromium.org/169653006/
>>
>
>  instead?
>>
>
> My initial attempts were along the line of traits, but without being able
> to
> hide the trait from being parameterized over WebPrivatePtr. You manage to
> avoid
> running into that problem very well; it looks like a pretty solid solution
> to
> me. I can't pick out any problems with it.
>
> https://codereview.chromium.org/168963003/
>

To unsubscribe from this group and stop receiving emails from it, send an email
to blink-reviews+unsubscribe@chromium.org.

[Vyacheslav Egorov (Chromium), 2014-02-17 19:42:45]

Ok, here is a version that works.

https://codereview.chromium.org/169653006/diff/60001/public/platform/WebPriva...

Instantiation is delayed by not using dependent type names in constructor
and assignment operation. Instead a I have to make them templated. Then
instantiation is delayed until they are called in the implementation file
where the definition of the T is fully available.

Vyacheslav Egorov


On Mon, Feb 17, 2014 at 8:09 PM, Vyacheslav Egorov <vegorov@chromium.org>wrote:

> There is one problem however, which I did not pay attention before. I
> uploaded another version of my CL highlighting the problem. Obviously due
> to this it is not usable (we need to alter includes in the files, which is
> no go).
>
> In short PtrStorage is instantiated too early and does not see the
> definition of the T it refers to so it never picks up garbage collected
> variant. Somehow we need to delay instantiation to the moment until after
> the definition is seen.
>
> I will think about it more.
>
> Vyacheslav Egorov
>
>
> On Mon, Feb 17, 2014 at 7:41 PM, <sigbjornf@opera.com> wrote:
>
>> On 2014/02/17 18:14:33, Vyacheslav Egorov wrote:
>>
>>> Have you considered doing something like this:
>>>
>>
>>  https://codereview.chromium.org/169653006/
>>>
>>
>>  instead?
>>>
>>
>> My initial attempts were along the line of traits, but without being able
>> to
>> hide the trait from being parameterized over WebPrivatePtr. You manage to
>> avoid
>> running into that problem very well; it looks like a pretty solid
>> solution to
>> me. I can't pick out any problems with it.
>>
>> https://codereview.chromium.org/168963003/
>>
>
>

To unsubscribe from this group and stop receiving emails from it, send an email
to blink-reviews+unsubscribe@chromium.org.

[sof, 2014-02-17 22:30:42]

On 2014/02/17 15:48:37, sof wrote:
> On 2014/02/17 15:37:50, Mads Ager (chromium) wrote:
> > I think we need to get rid of the template aliases. It was a nice try but it
> > just doesn't work yet on all compilers so we need to go to the lowest common
> > denominator.
> > 
> > If you are up for it it would be great if you could do that in this change.
> > Otherwise, I will be happy to do it as a followup.
> > 
> 
> I'll happily take care of that, but prefer to do it separately.
> 

I decided to do this right away & here instead.

[sof, 2014-02-17 22:43:40]

vegorov's WebPrivatePtr is technically superior; if the consensus is to go with
it, then let's definitely do so.

[Mads Ager (chromium), 2014-02-18 06:54:02]

On 2014/02/17 22:43:40, sof wrote:
> vegorov's WebPrivatePtr is technically superior; if the consensus is to go
with
> it, then let's definitely do so.

I think we should. I have added a couple of comments on Slava's code review. Do
you want to integrate Slava's proposal in your change here?

[sof, 2014-02-18 07:13:55]

On 2014/02/18 06:54:02, Mads Ager (chromium) wrote:
> On 2014/02/17 22:43:40, sof wrote:
> > vegorov's WebPrivatePtr is technically superior; if the consensus is to go
> with
> > it, then let's definitely do so.
> 
> I think we should. I have added a couple of comments on Slava's code review.
Do
> you want to integrate Slava's proposal in your change here?

Certainly, if that gets us there faster. I'll check/verify MSVC behavior wrt
instantiation.

[Vyacheslav Egorov (Chromium), 2014-02-18 07:23:22]

There is still one issue with my code, I discovered to late in the night so
I did not send a mail. Right now m_private = 0 will not compile. I am still
thinking how to attack this.
 On Feb 18, 2014 8:13 AM, <sigbjornf@opera.com> wrote:

> On 2014/02/18 06:54:02, Mads Ager (chromium) wrote:
>
>> On 2014/02/17 22:43:40, sof wrote:
>> > vegorov's WebPrivatePtr is technically superior; if the consensus is to
>> go
>> with
>> > it, then let's definitely do so.
>>
>
>  I think we should. I have added a couple of comments on Slava's code
>> review.
>>
> Do
>
>> you want to integrate Slava's proposal in your change here?
>>
>
> Certainly, if that gets us there faster. I'll check/verify MSVC behavior
> wrt
> instantiation.
>
> https://codereview.chromium.org/168963003/
>

To unsubscribe from this group and stop receiving emails from it, send an email
to blink-reviews+unsubscribe@chromium.org.

[sof1, 2014-02-18 07:26:10]

Den 18.02.2014 08:23, skreiv Vyacheslav Egorov:
> There is still one issue with my code, I discovered to late in the night so
> I did not send a mail. Right now m_private = 0 will not compile. I am still
> thinking how to attack this.

Yes, seeing same - reset() is ofc one way to work around the issue.

--sigbjorn

To unsubscribe from this group and stop receiving emails from it, send an email
to blink-reviews+unsubscribe@chromium.org.

[sof1, 2014-02-18 07:33:01]

Den 18.02.2014 08:25, skreiv Sigbjorn Finne:
> Den 18.02.2014 08:23, skreiv Vyacheslav Egorov:
>> There is still one issue with my code, I discovered to late in the
>> night so
>> I did not send a mail. Right now m_private = 0 will not compile. I am
>> still
>> thinking how to attack this.
>
> Yes, seeing same - reset() is ofc one way to work around the issue.
>

Another issue is how to resolve a WebPrivatePtr wrapping of a class 
derived from WebCore::RefCountedGarbageCollected.

--sigbjorn

To unsubscribe from this group and stop receiving emails from it, send an email
to blink-reviews+unsubscribe@chromium.org.

[Mads Ager (chromium), 2014-02-18 07:43:41]

On Tue, Feb 18, 2014 at 8:32 AM, Sigbjorn Finne <sof@opera.com> wrote:

> Den 18.02.2014 08:25, skreiv Sigbjorn Finne:
>
>  Den 18.02.2014 08:23, skreiv Vyacheslav Egorov:
>>
>>> There is still one issue with my code, I discovered to late in the
>>> night so
>>> I did not send a mail. Right now m_private = 0 will not compile. I am
>>> still
>>> thinking how to attack this.
>>>
>>
>> Yes, seeing same - reset() is ofc one way to work around the issue.
>>
>>
> Another issue is how to resolve a WebPrivatePtr wrapping of a class
> derived from WebCore::RefCountedGarbageCollected.


RefCountedGarbageCollected inherits from GarbageCollectedFinalized which
inherits from GarbageCollected, so if the traits work that should
automagically use the Persistent version which is what it should do. What
is the issue that you are seeing?

Let's spend a little time trying to make this work. If we cannot easily
make it work we should go ahead with your current change Sigbjorn so that
we have a solution. Then we can iterate on the traits version. It is easy
to replace when/if we get it working.

Cheers,    -- Mads

To unsubscribe from this group and stop receiving emails from it, send an email
to blink-reviews+unsubscribe@chromium.org.

[sof1, 2014-02-18 07:54:07]

Den 18.02.2014 08:43, skreiv Mads Sig Ager:
> On Tue, Feb 18, 2014 at 8:32 AM, Sigbjorn Finne <sof@opera.com> wrote:
>
>> Den 18.02.2014 08:25, skreiv Sigbjorn Finne:
>>
>>   Den 18.02.2014 08:23, skreiv Vyacheslav Egorov:
>>>
>>>> There is still one issue with my code, I discovered to late in the
>>>> night so
>>>> I did not send a mail. Right now m_private = 0 will not compile. I am
>>>> still
>>>> thinking how to attack this.
>>>>
>>>
>>> Yes, seeing same - reset() is ofc one way to work around the issue.
>>>
>>>
>> Another issue is how to resolve a WebPrivatePtr wrapping of a class
>> derived from WebCore::RefCountedGarbageCollected.
>
>
> RefCountedGarbageCollected inherits from GarbageCollectedFinalized which
> inherits from GarbageCollected, so if the traits work that should
> automagically use the Persistent version which is what it should do. What
> is the issue that you are seeing?
>

Yes, resolution leans that way. However, it implies that the create()
methods for the corresponding types will have to be returning a
RawPtr (and not PassRefPtr), i.e., we don't get to choose but have to
convert them & their objects over at the same time, I think.

An example of this is SpeechRecognition and WebSpeechRecognition.

--sigbjorn

To unsubscribe from this group and stop receiving emails from it, send an email
to blink-reviews+unsubscribe@chromium.org.

[sof1, 2014-02-18 08:09:49]

Den 18.02.2014 08:53, skreiv Sigbjorn Finne:
> Den 18.02.2014 08:43, skreiv Mads Sig Ager:
>> On Tue, Feb 18, 2014 at 8:32 AM, Sigbjorn Finne <sof@opera.com> wrote:
>>
>>> Den 18.02.2014 08:25, skreiv Sigbjorn Finne:
>>>
>>>   Den 18.02.2014 08:23, skreiv Vyacheslav Egorov:
>>>>
>>>>> There is still one issue with my code, I discovered to late in the
>>>>> night so
>>>>> I did not send a mail. Right now m_private = 0 will not compile. I am
>>>>> still
>>>>> thinking how to attack this.
>>>>>
>>>>
>>>> Yes, seeing same - reset() is ofc one way to work around the issue.
>>>>
>>>>
>>> Another issue is how to resolve a WebPrivatePtr wrapping of a class
>>> derived from WebCore::RefCountedGarbageCollected.
>>
>>
>> RefCountedGarbageCollected inherits from GarbageCollectedFinalized which
>> inherits from GarbageCollected, so if the traits work that should
>> automagically use the Persistent version which is what it should do. What
>> is the issue that you are seeing?
>>
>
> Yes, resolution leans that way. However, it implies that the create()
> methods for the corresponding types will have to be returning a
> RawPtr (and not PassRefPtr), i.e., we don't get to choose but have to
> convert them & their objects over at the same time, I think.
>
> An example of this is SpeechRecognition and WebSpeechRecognition.
>

The problem is minor, thankfully. I suspected there were other objects
that had been halfway converted like the Speech API ones, but not so.

Pulling in a subset of https://codereview.chromium.org/169343002/ takes
care of this.

To unsubscribe from this group and stop receiving emails from it, send an email
to blink-reviews+unsubscribe@chromium.org.

[Mads Ager (chromium), 2014-02-18 08:31:02]

On Tue, Feb 18, 2014 at 9:09 AM, Sigbjorn Finne <sof@opera.com> wrote:

> Den 18.02.2014 08:53, skreiv Sigbjorn Finne:
>
>  Den 18.02.2014 08:43, skreiv Mads Sig Ager:
>>
>>> On Tue, Feb 18, 2014 at 8:32 AM, Sigbjorn Finne <sof@opera.com> wrote:
>>>
>>>  Den 18.02.2014 08:25, skreiv Sigbjorn Finne:
>>>>
>>>>   Den 18.02.2014 08:23, skreiv Vyacheslav Egorov:
>>>>
>>>>>
>>>>>  There is still one issue with my code, I discovered to late in the
>>>>>> night so
>>>>>> I did not send a mail. Right now m_private = 0 will not compile. I am
>>>>>> still
>>>>>> thinking how to attack this.
>>>>>>
>>>>>>
>>>>> Yes, seeing same - reset() is ofc one way to work around the issue.
>>>>>
>>>>>
>>>>>  Another issue is how to resolve a WebPrivatePtr wrapping of a class
>>>> derived from WebCore::RefCountedGarbageCollected.
>>>>
>>>
>>>
>>> RefCountedGarbageCollected inherits from GarbageCollectedFinalized which
>>> inherits from GarbageCollected, so if the traits work that should
>>> automagically use the Persistent version which is what it should do. What
>>> is the issue that you are seeing?
>>>
>>>
>> Yes, resolution leans that way. However, it implies that the create()
>> methods for the corresponding types will have to be returning a
>> RawPtr (and not PassRefPtr), i.e., we don't get to choose but have to
>> convert them & their objects over at the same time, I think.
>>
>> An example of this is SpeechRecognition and WebSpeechRecognition.
>>
>>
> The problem is minor, thankfully. I suspected there were other objects
> that had been halfway converted like the Speech API ones, but not so.
>
> Pulling in a subset of https://codereview.chromium.org/169343002/ takes
> care of this.
>

There will be a lot of objects that have been halfway converted when we get
to the Node hierarchy. :-) We can probably just overload and allow both
PassRefPtr and RawPtr in the Persistent version. For the PassRefPtr we need
to assign passRefPtr.get() to the persistent then the PassRefPtr destructor
will deref the object and the persistent will keep it alive.

-- Mads

To unsubscribe from this group and stop receiving emails from it, send an email
to blink-reviews+unsubscribe@chromium.org.

[Vyacheslav Egorov (Chromium), 2014-02-18 08:41:56]

I think I can make 0 work so that we don't have to resort to .reset(), I am
checking my idea right now.

We can also add an assign(const PassRefPtr<T>& obj) to PtrStorage<T, true>
and that will take care of types in transition.

I will upload a new version of my CL shortly to make sure that we are on
the same page.

Vyacheslav Egorov


On Tue, Feb 18, 2014 at 9:30 AM, Mads Sig Ager <ager@chromium.org> wrote:

> On Tue, Feb 18, 2014 at 9:09 AM, Sigbjorn Finne <sof@opera.com> wrote:
>
>> Den 18.02.2014 08:53, skreiv Sigbjorn Finne:
>>
>>  Den 18.02.2014 08:43, skreiv Mads Sig Ager:
>>>
>>>> On Tue, Feb 18, 2014 at 8:32 AM, Sigbjorn Finne <sof@opera.com> wrote:
>>>>
>>>>  Den 18.02.2014 08:25, skreiv Sigbjorn Finne:
>>>>>
>>>>>   Den 18.02.2014 08:23, skreiv Vyacheslav Egorov:
>>>>>
>>>>>>
>>>>>>  There is still one issue with my code, I discovered to late in the
>>>>>>> night so
>>>>>>> I did not send a mail. Right now m_private = 0 will not compile. I am
>>>>>>> still
>>>>>>> thinking how to attack this.
>>>>>>>
>>>>>>>
>>>>>> Yes, seeing same - reset() is ofc one way to work around the issue.
>>>>>>
>>>>>>
>>>>>>  Another issue is how to resolve a WebPrivatePtr wrapping of a class
>>>>> derived from WebCore::RefCountedGarbageCollected.
>>>>>
>>>>
>>>>
>>>> RefCountedGarbageCollected inherits from GarbageCollectedFinalized which
>>>> inherits from GarbageCollected, so if the traits work that should
>>>> automagically use the Persistent version which is what it should do.
>>>> What
>>>> is the issue that you are seeing?
>>>>
>>>>
>>> Yes, resolution leans that way. However, it implies that the create()
>>> methods for the corresponding types will have to be returning a
>>> RawPtr (and not PassRefPtr), i.e., we don't get to choose but have to
>>> convert them & their objects over at the same time, I think.
>>>
>>> An example of this is SpeechRecognition and WebSpeechRecognition.
>>>
>>>
>> The problem is minor, thankfully. I suspected there were other objects
>> that had been halfway converted like the Speech API ones, but not so.
>>
>> Pulling in a subset of https://codereview.chromium.org/169343002/ takes
>> care of this.
>>
>
> There will be a lot of objects that have been halfway converted when we
> get to the Node hierarchy. :-) We can probably just overload and allow both
> PassRefPtr and RawPtr in the Persistent version. For the PassRefPtr we need
> to assign passRefPtr.get() to the persistent then the PassRefPtr destructor
> will deref the object and the persistent will keep it alive.
>
> -- Mads
>

To unsubscribe from this group and stop receiving emails from it, send an email
to blink-reviews+unsubscribe@chromium.org.

[sof, 2014-02-18 09:05:56]

On 2014/02/18 08:41:56, Vyacheslav Egorov wrote:
> I think I can make 0 work so that we don't have to resort to .reset(), I am
> checking my idea right now.
> 
> We can also add an assign(const PassRefPtr<T>& obj) to PtrStorage<T, true>
> and that will take care of types in transition.
> 
> I will upload a new version of my CL shortly to make sure that we are on
> the same page.
> 

Great, even without those adjustments, it's looking very good from this end.
Uploaded a new patchset here which adopts your WebPrivatePtr.

(Will ofc follow up and pull in further improvements.)

[Mads Ager (chromium), 2014-02-18 09:15:38]

We are getting there. The new code you pulled in have a couple of life-time
issues with Vectors of raw pointers that should be heap vectors of members. Once
that is fixed and we get Slava's fix for 0 assignment this looks ready to me.

https://codereview.chromium.org/168963003/diff/540001/Source/heap/Handle.h
File Source/heap/Handle.h (right):

https://codereview.chromium.org/168963003/diff/540001/Source/heap/Handle.h#ne...
Source/heap/Handle.h:517: // [
I would remove the '[' and ']' here.

https://codereview.chromium.org/168963003/diff/150002/Source/web/SpeechRecogn...
File Source/web/SpeechRecognitionClientProxy.cpp (right):

https://codereview.chromium.org/168963003/diff/150002/Source/web/SpeechRecogn...
Source/web/SpeechRecognitionClientProxy.cpp:107:
Vector<RefPtrWillBeRawPtr<SpeechRecognitionResult> >
finalResultsVector(newFinalResults.size());
These need to be WillBeHeapVector<RefPtrWillBeMember<Speech...> > otherwise the
GC will not visit them. For the same reason the signature of didReceiveResults
needs to take a const reference to a
WillBeHeapVector<RefPtrWillBeMember<Speech...> >.

[sof, 2014-02-18 09:42:22]

https://codereview.chromium.org/168963003/diff/150002/Source/web/SpeechRecogn...
File Source/web/SpeechRecognitionClientProxy.cpp (right):

https://codereview.chromium.org/168963003/diff/150002/Source/web/SpeechRecogn...
Source/web/SpeechRecognitionClientProxy.cpp:107:
Vector<RefPtrWillBeRawPtr<SpeechRecognitionResult> >
finalResultsVector(newFinalResults.size());
On 2014/02/18 09:15:39, Mads Ager (chromium) wrote:
> These need to be WillBeHeapVector<RefPtrWillBeMember<Speech...> > otherwise
the
> GC will not visit them. For the same reason the signature of didReceiveResults
> needs to take a const reference to a
> WillBeHeapVector<RefPtrWillBeMember<Speech...> >.

Thanks, fixed.

[Mads Ager (chromium), 2014-02-18 10:33:13]

LGTM when Slava's latest changes to use nullptr for WebPrivatePtr have been
applied.

[tkent, 2014-02-18 12:57:38]

public and web lgtm

[jochen (gone - plz use gerrit), 2014-02-18 13:36:10]

rubberstamp lgtm

[sof, 2014-02-18 15:59:01]

Last batch of WebPrivatePtr improvements applied. No oilpan trybots available
here, but passes tests when run locally.

public/ owners: I added "+heap" to public/DEPS - could you bless (or not) ?
Thanks.

[jochen (gone - plz use gerrit), 2014-02-18 16:01:15]

On 2014/02/18 15:59:01, sof wrote:
> Last batch of WebPrivatePtr improvements applied. No oilpan trybots available
> here, but passes tests when run locally.
> 
> public/ owners: I added "+heap" to public/DEPS - could you bless (or not) ?
> Thanks.

I bless it

[sof, 2014-02-18 16:04:31]

Great, thanks everyone. Let's give it a go.

[sof, 2014-02-18 16:04:44]

The CQ bit was checked by sigbjornf@opera.com

[commit-bot: I haz the power, 2014-02-18 16:04:55]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/sigbjornf@opera.com/168963003/750023

[commit-bot: I haz the power, 2014-02-18 16:59:00]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-02-18 16:59:01]

Retried try job too often on linux_blink for step(s) webkit_tests,
webkit_unit_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_blin...

[sof, 2014-02-18 17:02:32]

linux_blink fails to run tests due to:

 /out/Debug/webkit_unit_tests: symbol lookup error: 
/mnt/.../lib/libblink_web.so: undefined symbol: nullptr

[sof, 2014-02-18 20:04:01]

In order to unblock other Oilpan work that's on hold due to this CL, I'm going
to let https://codereview.chromium.org/170603003/ handle 'nullptr' issues, and
have dropped back to using reset()s instead of 'nullptr' (in the three places
where it was needed here.)

[sof, 2014-02-18 20:04:14]

The CQ bit was checked by sigbjornf@opera.com

[commit-bot: I haz the power, 2014-02-18 20:04:26]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/sigbjornf@opera.com/168963003/950001

[jamesr, 2014-02-18 22:18:52]

https://codereview.chromium.org/168963003/diff/950001/public/platform/WebPriv...
File public/platform/WebPrivatePtr.h (right):

https://codereview.chromium.org/168963003/diff/950001/public/platform/WebPriv...
public/platform/WebPrivatePtr.h:227: WebPrivatePtr<T>&
operator=(std::nullptr_t&)
NAK on this - we can't expose std::nullptr_t through public blink API.  Within
the blink library, we use WTF's emulated std::nullptr_t, but that's actually a
spec violation since it's a userspace implementation of something in namespace
std.  This should not leak to chromium code.

[jamesr, 2014-02-18 22:20:29]

https://codereview.chromium.org/168963003/diff/950001/public/platform/WebPriv...
File public/platform/WebPrivatePtr.h (right):

https://codereview.chromium.org/168963003/diff/950001/public/platform/WebPriv...
public/platform/WebPrivatePtr.h:227: WebPrivatePtr<T>&
operator=(std::nullptr_t&)
Ah - I missed that this was in the INSIDE_BLINK section, so it's probably fine. 
Still it's a bit iffy

[commit-bot: I haz the power, 2014-02-18 22:28:30]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-02-18 22:28:31]

Retried try job too often on linux_blink_rel for step(s) webkit_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_blin...

[sof, 2014-02-18 22:35:54]

The CQ bit was checked by sigbjornf@opera.com

[commit-bot: I haz the power, 2014-02-18 22:36:03]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/sigbjornf@opera.com/168963003/950001

[commit-bot: I haz the power, 2014-02-19 01:45:19]

Change committed as 167391

[sof, 2014-02-19 06:11:42]

https://codereview.chromium.org/168963003/diff/950001/public/platform/WebPriv...
File public/platform/WebPrivatePtr.h (right):

https://codereview.chromium.org/168963003/diff/950001/public/platform/WebPriv...
public/platform/WebPrivatePtr.h:227: WebPrivatePtr<T>&
operator=(std::nullptr_t&)
On 2014/02/18 22:20:30, jamesr wrote:
> Ah - I missed that this was in the INSIDE_BLINK section, so it's probably
fine. 
> Still it's a bit iffy

It will go away once https://codereview.chromium.org/170603003/ rolls in; hope
that's acceptable.