Oilpan: turn DOMWindow into a heap supplementable.

Oilpan: turn DOMWindow into a heap supplementable.

With DOMWindow now a garbage collected object, follow up and also turn
it into a heap-residing Supplementable, along with its Supplements.

R=haraken@chromium.org
BUG=340522
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=170146

[sof, 2014-03-26 19:25:33]

When you get the chance, please take a look.

The explicit clearing of PagePopupClient in ~DOMWindowPagePopup has been moved
to uninstall() instead to avoid touching a potentially-finalized GC object. I
believe that takes care of the problem behind 8daa82a5 (
http://bugs.webkit.org/show_bug.cgi?id=100454 ), which introduced this
detach/dispose operation.

[sof, 2014-03-26 19:28:29]

https://codereview.chromium.org/212933005/diff/1/Source/core/page/DOMWindowPa...
File Source/core/page/DOMWindowPagePopup.h (right):

https://codereview.chromium.org/212933005/diff/1/Source/core/page/DOMWindowPa...
Source/core/page/DOMWindowPagePopup.h:43: class DOMWindowPagePopup FINAL :
public NoBaseWillBeGarbageCollectedFinalized<DOMWindowPagePopup>, public
WillBeHeapSupplement<DOMWindow> {
I don't think this needs to be *Finalizable, but the GC plugin insists that no
destructor should be present if so. That looks unsafe for the non-Oilpan case,
so I fell back to using *Finalizable.

[haraken, 2014-03-27 00:59:03]

LGTM with comments.

https://codereview.chromium.org/212933005/diff/1/Source/core/page/DOMWindowPa...
File Source/core/page/DOMWindowPagePopup.cpp (right):

https://codereview.chromium.org/212933005/diff/1/Source/core/page/DOMWindowPa...
Source/core/page/DOMWindowPagePopup.cpp:70: if (supplement)

Is it possible that supplement is 0 ?

If no, I'd just write:

  pagePopupController(window)->clearPagePopupClient();

https://codereview.chromium.org/212933005/diff/1/Source/core/page/DOMWindowPa...
File Source/core/page/DOMWindowPagePopup.h (right):

https://codereview.chromium.org/212933005/diff/1/Source/core/page/DOMWindowPa...
Source/core/page/DOMWindowPagePopup.h:43: class DOMWindowPagePopup FINAL :
public NoBaseWillBeGarbageCollectedFinalized<DOMWindowPagePopup>, public
WillBeHeapSupplement<DOMWindow> {
On 2014/03/26 19:28:29, sof wrote:
> I don't think this needs to be *Finalizable, but the GC plugin insists that no
> destructor should be present if so. That looks unsafe for the non-Oilpan case,
> so I fell back to using *Finalizable.

You can use DECLARE_EMPTY_VIRTUAL_DESTROCTOR and DEFINE_EMPTY_DESTRUCTOR, and
then make it NoBaseWillBeGarbageCollected.

https://codereview.chromium.org/212933005/diff/1/Source/modules/crypto/DOMWin...
File Source/modules/crypto/DOMWindowCrypto.h (right):

https://codereview.chromium.org/212933005/diff/1/Source/modules/crypto/DOMWin...
Source/modules/crypto/DOMWindowCrypto.h:43: class DOMWindowCrypto FINAL : public
NoBaseWillBeGarbageCollectedFinalized<DOMWindowCrypto>, public
WillBeHeapSupplement<DOMWindow>, public DOMWindowProperty {

Ditto. By using DECLARE_EMPTY_VIRTUAL_DESTRUCTOR and DEFINE_EMPTY_DESTRUCTOR,
you can make it NoBaseWillBeGarbageCollected.

https://codereview.chromium.org/212933005/diff/1/Source/modules/imagebitmap/I...
File Source/modules/imagebitmap/ImageBitmapFactories.h (right):

https://codereview.chromium.org/212933005/diff/1/Source/modules/imagebitmap/I...
Source/modules/imagebitmap/ImageBitmapFactories.h:57: class ImageBitmapFactories
FINAL : public NoBaseWillBeGarbageCollectedFinalized<ImageBitmapFactories>,
public WillBeHeapSupplement<DOMWindow>, public
WillBeHeapSupplement<WorkerGlobalScope> {

Ditto.

https://codereview.chromium.org/212933005/diff/1/Source/modules/imagebitmap/I...
Source/modules/imagebitmap/ImageBitmapFactories.h:61: class ImageBitmapLoader;

Nit: I think you can just move this to the above list of class forward
declarations.

https://codereview.chromium.org/212933005/diff/1/Source/modules/indexeddb/DOM...
File Source/modules/indexeddb/DOMWindowIndexedDatabase.h (right):

https://codereview.chromium.org/212933005/diff/1/Source/modules/indexeddb/DOM...
Source/modules/indexeddb/DOMWindowIndexedDatabase.h:38: class
DOMWindowIndexedDatabase FINAL : public
NoBaseWillBeGarbageCollectedFinalized<DOMWindowIndexedDatabase>, public
WillBeHeapSupplement<DOMWindow>, public DOMWindowProperty {

Ditto.

https://codereview.chromium.org/212933005/diff/1/Source/modules/quota/DOMWind...
File Source/modules/quota/DOMWindowQuota.h (right):

https://codereview.chromium.org/212933005/diff/1/Source/modules/quota/DOMWind...
Source/modules/quota/DOMWindowQuota.h:43: class DOMWindowQuota FINAL : public
NoBaseWillBeGarbageCollectedFinalized<DOMWindowQuota>, public
WillBeHeapSupplement<DOMWindow>, public DOMWindowProperty {

Ditto.

https://codereview.chromium.org/212933005/diff/1/Source/modules/speech/DOMWin...
File Source/modules/speech/DOMWindowSpeechSynthesis.h (right):

https://codereview.chromium.org/212933005/diff/1/Source/modules/speech/DOMWin...
Source/modules/speech/DOMWindowSpeechSynthesis.h:38: class
DOMWindowSpeechSynthesis FINAL : public
NoBaseWillBeGarbageCollectedFinalized<DOMWindowSpeechSynthesis>, public
WillBeHeapSupplement<DOMWindow>, public DOMWindowProperty {

Ditto.

[sof, 2014-03-27 07:25:51]

https://codereview.chromium.org/212933005/diff/1/Source/core/page/DOMWindowPa...
File Source/core/page/DOMWindowPagePopup.cpp (right):

https://codereview.chromium.org/212933005/diff/1/Source/core/page/DOMWindowPa...
Source/core/page/DOMWindowPagePopup.cpp:70: if (supplement)
On 2014/03/27 00:59:03, haraken wrote:
> 
> Is it possible that supplement is 0 ?
> 
> If no, I'd just write:
> 
>   pagePopupController(window)->clearPagePopupClient();

Much better that way; done.

https://codereview.chromium.org/212933005/diff/1/Source/core/page/DOMWindowPa...
File Source/core/page/DOMWindowPagePopup.h (right):

https://codereview.chromium.org/212933005/diff/1/Source/core/page/DOMWindowPa...
Source/core/page/DOMWindowPagePopup.h:43: class DOMWindowPagePopup FINAL :
public NoBaseWillBeGarbageCollectedFinalized<DOMWindowPagePopup>, public
WillBeHeapSupplement<DOMWindow> {
On 2014/03/27 00:59:03, haraken wrote:
> On 2014/03/26 19:28:29, sof wrote:
> > I don't think this needs to be *Finalizable, but the GC plugin insists that
no
> > destructor should be present if so. That looks unsafe for the non-Oilpan
case,
> > so I fell back to using *Finalizable.
> 
> You can use DECLARE_EMPTY_VIRTUAL_DESTROCTOR and DEFINE_EMPTY_DESTRUCTOR, and
> then make it NoBaseWillBeGarbageCollected.

Ah - switched, thanks. (I wasn't aware that had been added recently as I'm not
seeing oilpan-reviews traffic.)

https://codereview.chromium.org/212933005/diff/1/Source/modules/crypto/DOMWin...
File Source/modules/crypto/DOMWindowCrypto.h (right):

https://codereview.chromium.org/212933005/diff/1/Source/modules/crypto/DOMWin...
Source/modules/crypto/DOMWindowCrypto.h:43: class DOMWindowCrypto FINAL : public
NoBaseWillBeGarbageCollectedFinalized<DOMWindowCrypto>, public
WillBeHeapSupplement<DOMWindow>, public DOMWindowProperty {
On 2014/03/27 00:59:03, haraken wrote:
> 
> Ditto. By using DECLARE_EMPTY_VIRTUAL_DESTRUCTOR and DEFINE_EMPTY_DESTRUCTOR,
> you can make it NoBaseWillBeGarbageCollected.

Same re: DOMWindowProperty.

https://codereview.chromium.org/212933005/diff/1/Source/modules/imagebitmap/I...
File Source/modules/imagebitmap/ImageBitmapFactories.h (right):

https://codereview.chromium.org/212933005/diff/1/Source/modules/imagebitmap/I...
Source/modules/imagebitmap/ImageBitmapFactories.h:57: class ImageBitmapFactories
FINAL : public NoBaseWillBeGarbageCollectedFinalized<ImageBitmapFactories>,
public WillBeHeapSupplement<DOMWindow>, public
WillBeHeapSupplement<WorkerGlobalScope> {
On 2014/03/27 00:59:03, haraken wrote:
> 
> Ditto.

No, we need to release the HashSet on finalization.

https://codereview.chromium.org/212933005/diff/1/Source/modules/imagebitmap/I...
Source/modules/imagebitmap/ImageBitmapFactories.h:61: class ImageBitmapLoader;
On 2014/03/27 00:59:03, haraken wrote:
> 
> Nit: I think you can just move this to the above list of class forward
> declarations.

It's a local class. But the forward decl is avoidable; did that instead.

https://codereview.chromium.org/212933005/diff/1/Source/modules/indexeddb/DOM...
File Source/modules/indexeddb/DOMWindowIndexedDatabase.h (right):

https://codereview.chromium.org/212933005/diff/1/Source/modules/indexeddb/DOM...
Source/modules/indexeddb/DOMWindowIndexedDatabase.h:38: class
DOMWindowIndexedDatabase FINAL : public
NoBaseWillBeGarbageCollectedFinalized<DOMWindowIndexedDatabase>, public
WillBeHeapSupplement<DOMWindow>, public DOMWindowProperty {
On 2014/03/27 00:59:03, haraken wrote:
> 
> Ditto.

Same re: DOMWindowProperty.

https://codereview.chromium.org/212933005/diff/1/Source/modules/quota/DOMWind...
File Source/modules/quota/DOMWindowQuota.h (right):

https://codereview.chromium.org/212933005/diff/1/Source/modules/quota/DOMWind...
Source/modules/quota/DOMWindowQuota.h:43: class DOMWindowQuota FINAL : public
NoBaseWillBeGarbageCollectedFinalized<DOMWindowQuota>, public
WillBeHeapSupplement<DOMWindow>, public DOMWindowProperty {
On 2014/03/27 00:59:03, haraken wrote:
> 
> Ditto.

Same re: DOMWindowProperty.

https://codereview.chromium.org/212933005/diff/1/Source/modules/speech/DOMWin...
File Source/modules/speech/DOMWindowSpeechSynthesis.h (right):

https://codereview.chromium.org/212933005/diff/1/Source/modules/speech/DOMWin...
Source/modules/speech/DOMWindowSpeechSynthesis.h:38: class
DOMWindowSpeechSynthesis FINAL : public
NoBaseWillBeGarbageCollectedFinalized<DOMWindowSpeechSynthesis>, public
WillBeHeapSupplement<DOMWindow>, public DOMWindowProperty {
On 2014/03/27 00:59:03, haraken wrote:
> 
> Ditto.

I think we need to consider changing DOMWindowProperty first before that's
doable.

[haraken, 2014-03-27 07:41:03]

Thanks, LGTM.

[sof, 2014-03-27 08:28:35]

The CQ bit was checked by sigbjornf@opera.com

[commit-bot: I haz the power, 2014-03-27 08:28:37]

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

[commit-bot: I haz the power, 2014-03-27 08:38:31]

Change committed as 170146