[not for commit]

[not for commit]

Let WebPrivatePtr<T> select different pointer storage based on whether T inherits from GarbageCollected or not.

BUG=

[zerny-chromium, 2014-02-18 06:42:02]

https://codereview.chromium.org/169653006/diff/60001/public/platform/WebPriva...
File public/platform/WebPrivatePtr.h (right):

https://codereview.chromium.org/169653006/diff/60001/public/platform/WebPriva...
public/platform/WebPrivatePtr.h:114: class PtrStorage : public PtrStorageImpl<T,
WebCore::IsGarbageCollected<T>::value> {
Nit: this could use:

  IsSubclassOfTemplate<T, WebCore::GarbageCollected>::value

in the existing traits (wtf/TypeTraits.h)

We should also clean up our other uses of SFINAE to use these.

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

This is cool, thanks Slava. Before moving ahead with this we should double check
that the delaying of instantiation works with msvc which seems to be the
compiler that does the most eager instantiations.

My only reservation here is that using traits makes this class pretty
complicated. However, it allows us to not have to change the API layer and it is
localized to one file and makes it easy to write new files for the API layer by
just using the same pattern as everywhere else. Overall, this seems like the
right tradeoff.

https://codereview.chromium.org/169653006/diff/60001/Source/heap/Visitor.h
File Source/heap/Visitor.h (right):

https://codereview.chromium.org/169653006/diff/60001/Source/heap/Visitor.h#ne...
Source/heap/Visitor.h:98: struct IsGarbageCollected {
Can we use WTF::IsSubclassOfTemplate here?

WTF::IsSubclassOfTemplate<T, GarbageCollected>::value

https://codereview.chromium.org/169653006/diff/60001/Source/heap/Visitor.h#ne...
Source/heap/Visitor.h:112: struct IsGarbageCollectedFinalized {
I think we can get rid of this one as well and use the WTF version?

[haraken, 2014-02-18 06:58:47]

https://codereview.chromium.org/169653006/diff/60001/public/platform/WebPriva...
File public/platform/WebPrivatePtr.h (right):

https://codereview.chromium.org/169653006/diff/60001/public/platform/WebPriva...
public/platform/WebPrivatePtr.h:130: PtrStorage(const PtrStorage&);

Nit: Add explicit.

https://codereview.chromium.org/169653006/diff/60001/public/platform/WebPriva...
public/platform/WebPrivatePtr.h:234: void* m_storage;

It looks like m_storage is used by INSIDE_BLINK. (m_storage is used by
WebPrivatePtr's constructor and isNull(), but they are doing nothing.)

[Vyacheslav Egorov (Chromium), 2014-02-18 09:37:44]

Here is the latest version. 

1) = 0 assignments to WebPrivatePtr are replaced with = nullptr;

2) Provided PtrStorage that works for transitional type
RefCountedGarbageCollected.

https://codereview.chromium.org/169653006/diff/60001/Source/heap/Visitor.h
File Source/heap/Visitor.h (right):

https://codereview.chromium.org/169653006/diff/60001/Source/heap/Visitor.h#ne...
Source/heap/Visitor.h:98: struct IsGarbageCollected {
On 2014/02/18 06:50:50, Mads Ager (chromium) wrote:
> Can we use WTF::IsSubclassOfTemplate here?
> 
> WTF::IsSubclassOfTemplate<T, GarbageCollected>::value

Done.

https://codereview.chromium.org/169653006/diff/60001/public/platform/WebPriva...
File public/platform/WebPrivatePtr.h (right):

https://codereview.chromium.org/169653006/diff/60001/public/platform/WebPriva...
public/platform/WebPrivatePtr.h:114: class PtrStorage : public PtrStorageImpl<T,
WebCore::IsGarbageCollected<T>::value> {
On 2014/02/18 06:42:02, zerny-chromium wrote:
> Nit: this could use:
> 
>   IsSubclassOfTemplate<T, WebCore::GarbageCollected>::value
> 
> in the existing traits (wtf/TypeTraits.h)
> 
> We should also clean up our other uses of SFINAE to use these.

Done.

https://codereview.chromium.org/169653006/diff/60001/public/platform/WebPriva...
public/platform/WebPrivatePtr.h:130: PtrStorage(const PtrStorage&);
On 2014/02/18 06:58:47, haraken wrote:
> 
> Nit: Add explicit.

I am blocking copy constructor. No need for explicit here.

[sof, 2014-02-18 10:36:56]

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

https://codereview.chromium.org/169653006/diff/130001/public/platform/WebPriv...
public/platform/WebPrivatePtr.h:228: storage().reset();
Shouldn't this be reset() (or storage.release()) ?

[sof, 2014-02-18 11:24:20]

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

https://codereview.chromium.org/169653006/diff/130001/public/platform/WebPriv...
public/platform/WebPrivatePtr.h:132: void assign(const PassRefPtr<T>& val) {
assign(val.get()); }
I'm seeing this cause infinite recursion when running the fast/speech tests.

[Vyacheslav Egorov (Chromium), 2014-02-18 12:02:32]

New version is up.

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

https://codereview.chromium.org/169653006/diff/130001/public/platform/WebPriv...
public/platform/WebPrivatePtr.h:132: void assign(const PassRefPtr<T>& val) {
assign(val.get()); }
On 2014/02/18 11:24:20, sof wrote:
> I'm seeing this cause infinite recursion when running the fast/speech tests.

Fixed & verified.

https://codereview.chromium.org/169653006/diff/130001/public/platform/WebPriv...
public/platform/WebPrivatePtr.h:228: storage().reset();
On 2014/02/18 10:36:56, sof wrote:
> Shouldn't this be reset() (or storage.release()) ?

Killed this overload. It is not actually selected / needed.

[sof, 2014-02-18 12:07:39]

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

https://codereview.chromium.org/169653006/diff/130001/public/platform/WebPriv...
public/platform/WebPrivatePtr.h:228: storage().reset();
On 2014/02/18 12:02:33, Vyacheslav Egorov wrote:
> On 2014/02/18 10:36:56, sof wrote:
> > Shouldn't this be reset() (or storage.release()) ?
> 
> Killed this overload. It is not actually selected / needed.

I don't understand. Aren't we making use of nullptr?

[Vyacheslav Egorov (Chromium), 2014-02-18 12:18:16]

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

https://codereview.chromium.org/169653006/diff/130001/public/platform/WebPriv...
public/platform/WebPrivatePtr.h:228: storage().reset();
On 2014/02/18 12:07:40, sof wrote:
> On 2014/02/18 12:02:33, Vyacheslav Egorov wrote:
> > On 2014/02/18 10:36:56, sof wrote:
> > > Shouldn't this be reset() (or storage.release()) ?
> > 
> > Killed this overload. It is not actually selected / needed.
> 
> I don't understand. Aren't we making use of nullptr?

It can just hit the normal assignment operator and make PassRefPtr/RawPtr out of
the nullptr.

[Vyacheslav Egorov (Chromium), 2014-02-18 12:34:51]

New version: return nullptr_t overload to make sure that it works for RawPtr
case (RawPtr does not have a constructor from nullptr_t).

This time I just do it right: we can't have nullptr_t& only const nullptr_t& or
value. Also switched to calling reset() the right way as it was pointed out by
Sigbjørn.

[sof, 2014-02-18 12:58:56]

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

https://codereview.chromium.org/169653006/diff/130001/public/platform/WebPriv...
public/platform/WebPrivatePtr.h:132: void assign(const PassRefPtr<T>& val) {
assign(val.get()); }
On 2014/02/18 12:02:33, Vyacheslav Egorov wrote:
> On 2014/02/18 11:24:20, sof wrote:
> > I'm seeing this cause infinite recursion when running the fast/speech tests.
> 
> Fixed & verified.

It still crashes due to infinite regress for me, I'm afraid.

Oilpan enabled; I'll try without next.

[Vyacheslav Egorov (Chromium), 2014-02-18 13:12:20]

:-(

I tested both with and without OilPan it worked.

Which compiler do you use?

In any case we can work around by doing rename first

assign(RawPtr<T> ptr) -> assignImpl(T* ptr)

and then the rest should call assignImpl after unwrapping respective smart
pointers:

assign(RawPtr<T> ptr) { assignImpl(ptr.get()); }

[sof, 2014-02-18 13:22:14]

On 2014/02/18 13:12:20, Vyacheslav Egorov wrote:
> :-(
> 
> I tested both with and without OilPan it worked.
> 
> Which compiler do you use?
> 
> In any case we can work around by doing rename first
> 
> assign(RawPtr<T> ptr) -> assignImpl(T* ptr)
> 
> and then the rest should call assignImpl after unwrapping respective smart
> pointers:
> 
> assign(RawPtr<T> ptr) { assignImpl(ptr.get()); }

This is with g++-4.7.3; no problems with Oilpan disabled.

Hmm, to make sure this isn't coming in via ccache somehow, I'll clear & rebuild
next. Will try out your suggestions after that, thanks.

[sof, 2014-02-18 15:27:05]

On 2014/02/18 13:22:14, sof wrote:
> On 2014/02/18 13:12:20, Vyacheslav Egorov wrote:
> > :-(
> > 
> > I tested both with and without OilPan it worked.
> > 
> > Which compiler do you use?
> > 
> > In any case we can work around by doing rename first
> > 
> > assign(RawPtr<T> ptr) -> assignImpl(T* ptr)
> > 
> > and then the rest should call assignImpl after unwrapping respective smart
> > pointers:
> > 
> > assign(RawPtr<T> ptr) { assignImpl(ptr.get()); }
> 
> This is with g++-4.7.3; no problems with Oilpan disabled.
> 
> Hmm, to make sure this isn't coming in via ccache somehow, I'll clear &
rebuild
> next. Will try out your suggestions after that, thanks.

Ended up having to guide method resolution, see
https://codereview.chromium.org/168963003/patch/750023/690021

[Vyacheslav Egorov (Chromium), 2014-02-18 15:35:02]

Thanks for fixing it!

Vyacheslav Egorov


On Tue, Feb 18, 2014 at 4:27 PM, <sigbjornf@opera.com> wrote:

> On 2014/02/18 13:22:14, sof wrote:
>
>> On 2014/02/18 13:12:20, Vyacheslav Egorov wrote:
>> > :-(
>> >
>> > I tested both with and without OilPan it worked.
>> >
>> > Which compiler do you use?
>> >
>> > In any case we can work around by doing rename first
>> >
>> > assign(RawPtr<T> ptr) -> assignImpl(T* ptr)
>> >
>> > and then the rest should call assignImpl after unwrapping respective
>> smart
>> > pointers:
>> >
>> > assign(RawPtr<T> ptr) { assignImpl(ptr.get()); }
>>
>
>  This is with g++-4.7.3; no problems with Oilpan disabled.
>>
>
>  Hmm, to make sure this isn't coming in via ccache somehow, I'll clear &
>>
> rebuild
>
>> next. Will try out your suggestions after that, thanks.
>>
>
> Ended up having to guide method resolution, see
> https://codereview.chromium.org/168963003/patch/750023/690021
>
> https://codereview.chromium.org/169653006/
>

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