Oilpan: make Supplementable tracing more regular.

Oilpan: make Supplementable tracing more regular.

With PersistentHeapSupplementable retired, follow up and only provide a
trace() method for a Supplementable that's on the heap.

No changes made for Supplements: an empty trace() method is still
provided for off-heap supplements, as this is the better solution
while we still are using the transition type WillBeHeapSupplement
for all the supplement objects.

R=haraken,zerny,wibling
BUG=395036
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=182477

[sof, 2014-09-22 21:33:23]

sigbjornf@opera.com changed reviewers:
+ oilpan-reviews@chromium.org

[sof, 2014-09-22 21:33:23]

Please take a look.

[haraken, 2014-09-23 02:33:45]

haraken@chromium.org changed reviewers:
+ haraken@chromium.org

[haraken, 2014-09-23 02:33:46]

LGTM

If we use WillBeHeapSupplement<X>::trace(visitor), it will cause a compile error
in non-oilpan builds, right?

https://codereview.chromium.org/587393002/diff/20001/Source/core/workers/Work...
File Source/core/workers/WorkerClients.h (right):

https://codereview.chromium.org/587393002/diff/20001/Source/core/workers/Work...
Source/core/workers/WorkerClients.h:53: #if ENABLE(OILPAN)

We normally add #if to inside the trace method.

https://codereview.chromium.org/587393002/diff/20001/Source/platform/Suppleme...
File Source/platform/Supplementable.h (right):

https://codereview.chromium.org/587393002/diff/20001/Source/platform/Suppleme...
Source/platform/Supplementable.h:123: // transition type is (or its use is
greatly reduced.)

is => is gone

[sof, 2014-09-23 05:11:51]

On 2014/09/23 02:33:46, haraken wrote:
> LGTM
> 
> If we use WillBeHeapSupplement<X>::trace(visitor), it will cause a compile
error
> in non-oilpan builds, right?
> 

With enable_oilpan=0, WillBeHeapSupplementable<T>::trace(visitor) will fail to
compile if T is not a garbage collected type.

I did go through the exercise of removing Supplement<T>::trace() for non-GC
classes also. The amount of #if usage required for all the instances was
prohibitive, so keeping the empty trace() definition seems right, for now.

[sof, 2014-09-23 05:21:08]

https://codereview.chromium.org/587393002/diff/20001/Source/core/workers/Work...
File Source/core/workers/WorkerClients.h (right):

https://codereview.chromium.org/587393002/diff/20001/Source/core/workers/Work...
Source/core/workers/WorkerClients.h:53: #if ENABLE(OILPAN)
On 2014/09/23 02:33:46, haraken wrote:
> 
> We normally add #if to inside the trace method.

Yes, the reason for not doing so here is to avoid compiler confusion over
OVERRIDE; trace() is only overridden with Oilpan enabled.

https://codereview.chromium.org/587393002/diff/20001/Source/platform/Suppleme...
File Source/platform/Supplementable.h (right):

https://codereview.chromium.org/587393002/diff/20001/Source/platform/Suppleme...
Source/platform/Supplementable.h:123: // transition type is (or its use is
greatly reduced.)
On 2014/09/23 02:33:46, haraken wrote:
> 
> is => is gone

Thanks, done.

[haraken, 2014-09-23 05:21:24]

On 2014/09/23 05:11:51, sof wrote:
> On 2014/09/23 02:33:46, haraken wrote:
> > LGTM
> > 
> > If we use WillBeHeapSupplement<X>::trace(visitor), it will cause a compile
> error
> > in non-oilpan builds, right?
> > 
> 
> With enable_oilpan=0, WillBeHeapSupplementable<T>::trace(visitor) will fail to
> compile if T is not a garbage collected type.
> 
> I did go through the exercise of removing Supplement<T>::trace() for non-GC
> classes also. The amount of #if usage required for all the instances was
> prohibitive, so keeping the empty trace() definition seems right, for now.

Makes sense.

[zerny-chromium, 2014-09-23 06:09:27]

zerny@chromium.org changed reviewers:
+ zerny@chromium.org

[zerny-chromium, 2014-09-23 06:09:27]

Thanks for cleaning that up! LGTM

[wibling-chromium, 2014-09-23 07:29:15]

wibling@chromium.org changed reviewers:
+ wibling@chromium.org

[wibling-chromium, 2014-09-23 07:29:16]

lgtm

[sof, 2014-09-23 07:41:29]

Thanks for the reviews.

tkent, jochen: one of you (overburdened) platform owners willing to look at the
platform/ change? tia

[haraken, 2014-09-23 07:49:01]

On 2014/09/23 07:41:29, sof wrote:
> Thanks for the reviews.
> 
> tkent, jochen: one of you (overburdened) platform owners willing to look at
the
> platform/ change? tia

(I am now an owner of platform/ :-)

[sof, 2014-09-23 07:53:11]

On 2014/09/23 07:49:01, haraken wrote:
> On 2014/09/23 07:41:29, sof wrote:
> > Thanks for the reviews.
> > 
> > tkent, jochen: one of you (overburdened) platform owners willing to look at
> the
> > platform/ change? tia
> 
> (I am now an owner of platform/ :-)

Excellent, didn't know :)

[sof, 2014-09-23 07:53:20]

The CQ bit was checked by sigbjornf@opera.com

[commit-bot: I haz the power, 2014-09-23 07:54:25]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/587393002/40001

[commit-bot: I haz the power, 2014-09-23 09:07:39]

Committed patchset #3 (id:40001) as 182477