Simplify DOMWindowProperty and its unregistration.

Simplify DOMWindowProperty and its unregistration.

As LocalDOMWindow controls the registration lifetime of its
DOMWindowProperties, there's no need to offer a separate unregistration
mechanism. Remove it, simplify notification code + retire now unused
LocalDOMWindow back reference on DOMWindowProperty.

R=haraken,ager
BUG=

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

[sof, 2014-09-10 15:22:43]

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

[sof, 2014-09-10 15:22:44]

Please take a look.

Not oilpan(-reviews) specific, but DOMWindowProperty lifetime details are fresh
in their collective mind. :)

[haraken, 2014-09-10 15:41:58]

LGTM, this is a nice clean-up!

[Mads Ager (chromium), 2014-09-10 15:52:56]

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

[Mads Ager (chromium), 2014-09-10 15:52:56]

https://codereview.chromium.org/558213002/diff/20001/Source/core/frame/LocalD...
File Source/core/frame/LocalDOMWindow.h (right):

https://codereview.chromium.org/558213002/diff/20001/Source/core/frame/LocalD...
Source/core/frame/LocalDOMWindow.h:368:
WillBeHeapHashSet<RawPtrWillBeMember<DOMWindowProperty> > m_properties;
What makes sure that a DOMWindowProperty cannot die before the LocalDOMWindow
and therefore the notification that the global object is being destroyed?

It makes sense for the DOMWindowProperties that are LocalDOMWindow supplements.
However, there are other DOMWIndowProperties such as BarProp that are not
LocalDOMWindow supplements and I can't convince myself that they cannot die
before the LocalDOMWindow in the non-oilpan build?

(That also reopens the question that we had on the oilpan part of this code. I
said that I would prefer a strong relationship between the DOMWindowProperty and
its LocalDOMWindow. However, if there are short lived DOMWindowProperties, we
might not want to have the LocalDOMWindow keep them alive. If that is the case
we should move to weak processing in the oilpan build to remove from this set.)

[Mads Ager (chromium), 2014-09-10 15:55:19]

LGTM!

https://codereview.chromium.org/558213002/diff/20001/Source/core/frame/LocalD...
File Source/core/frame/LocalDOMWindow.h (right):

https://codereview.chromium.org/558213002/diff/20001/Source/core/frame/LocalD...
Source/core/frame/LocalDOMWindow.h:368:
WillBeHeapHashSet<RawPtrWillBeMember<DOMWindowProperty> > m_properties;
On 2014/09/10 15:52:56, Mads Ager (chromium) wrote:
> What makes sure that a DOMWindowProperty cannot die before the LocalDOMWindow
> and therefore the notification that the global object is being destroyed?
> 
> It makes sense for the DOMWindowProperties that are LocalDOMWindow
supplements.
> However, there are other DOMWIndowProperties such as BarProp that are not
> LocalDOMWindow supplements and I can't convince myself that they cannot die
> before the LocalDOMWindow in the non-oilpan build?
> 
> (That also reopens the question that we had on the oilpan part of this code. I
> said that I would prefer a strong relationship between the DOMWindowProperty
and
> its LocalDOMWindow. However, if there are short lived DOMWindowProperties, we
> might not want to have the LocalDOMWindow keep them alive. If that is the case
> we should move to weak processing in the oilpan build to remove from this
set.)

Heh, ignore me. All of those properties are held alive by the list of RefPtrs
below. :-)

[sof, 2014-09-10 18:18:39]

https://codereview.chromium.org/558213002/diff/20001/Source/core/frame/LocalD...
File Source/core/frame/LocalDOMWindow.h (right):

https://codereview.chromium.org/558213002/diff/20001/Source/core/frame/LocalD...
Source/core/frame/LocalDOMWindow.h:368:
WillBeHeapHashSet<RawPtrWillBeMember<DOMWindowProperty> > m_properties;
On 2014/09/10 15:55:19, Mads Ager (chromium) wrote:
> On 2014/09/10 15:52:56, Mads Ager (chromium) wrote:
> > What makes sure that a DOMWindowProperty cannot die before the
LocalDOMWindow
> > and therefore the notification that the global object is being destroyed?
> > 
> > It makes sense for the DOMWindowProperties that are LocalDOMWindow
> supplements.
> > However, there are other DOMWIndowProperties such as BarProp that are not
> > LocalDOMWindow supplements and I can't convince myself that they cannot die
> > before the LocalDOMWindow in the non-oilpan build?
> > 
> > (That also reopens the question that we had on the oilpan part of this code.
I
> > said that I would prefer a strong relationship between the DOMWindowProperty
> and
> > its LocalDOMWindow. However, if there are short lived DOMWindowProperties,
we
> > might not want to have the LocalDOMWindow keep them alive. If that is the
case
> > we should move to weak processing in the oilpan build to remove from this
> set.)
> 
> Heh, ignore me. All of those properties are held alive by the list of RefPtrs
> below. :-)

Yes, it looks doable to elaborate m_properties to avoid having that long list of
strong references. But it makes for convenient&fast lookups as it is.

(A micro optimization with Oilpan would be not to re-trace() them.)

[sof, 2014-09-10 18:18:55]

The CQ bit was checked by sigbjornf@opera.com

[commit-bot: I haz the power, 2014-09-10 18:19:07]

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

[commit-bot: I haz the power, 2014-09-10 19:22:42]

Committed patchset #2 (id:20001) as 181758

[sof, 2014-09-12 08:21:07]

On 2014/09/10 15:55:19, Mads Ager (chromium) wrote:
> LGTM!
> 
>
https://codereview.chromium.org/558213002/diff/20001/Source/core/frame/LocalD...
> File Source/core/frame/LocalDOMWindow.h (right):
> 
>
https://codereview.chromium.org/558213002/diff/20001/Source/core/frame/LocalD...
> Source/core/frame/LocalDOMWindow.h:368:
> WillBeHeapHashSet<RawPtrWillBeMember<DOMWindowProperty> > m_properties;
> On 2014/09/10 15:52:56, Mads Ager (chromium) wrote:
> > What makes sure that a DOMWindowProperty cannot die before the
LocalDOMWindow
> > and therefore the notification that the global object is being destroyed?
> > 
> > It makes sense for the DOMWindowProperties that are LocalDOMWindow
> supplements.
> > However, there are other DOMWIndowProperties such as BarProp that are not
> > LocalDOMWindow supplements and I can't convince myself that they cannot die
> > before the LocalDOMWindow in the non-oilpan build?
> > 
> > (That also reopens the question that we had on the oilpan part of this code.
I
> > said that I would prefer a strong relationship between the DOMWindowProperty
> and
> > its LocalDOMWindow. However, if there are short lived DOMWindowProperties,
we
> > might not want to have the LocalDOMWindow keep them alive. If that is the
case
> > we should move to weak processing in the oilpan build to remove from this
> set.)
> 
> Heh, ignore me. All of those properties are held alive by the list of RefPtrs
> below. :-)

Bad idea not take that concern fully aboard :) DOMSelection objects live & die
independently of any LocalDOMWindow, so they will need to  be able to
detach/unregister as a DOMWindowProperty when being finalized. Other
DOMWindowProperty-derived objects appear kept alive via LocalDOMWindow.

So this optimization isn't valid in general _and_ I think we do need to move to
having LocalDOMWindow::m_properties be a weak set. (The use of strong references
hid this DOMSelection-involved bug with Oilpan enabled.)

[Mads Ager (chromium), 2014-09-12 08:47:17]

On 2014/09/12 08:21:07, sof wrote:
> On 2014/09/10 15:55:19, Mads Ager (chromium) wrote:
> > LGTM!
> > 
> >
>
https://codereview.chromium.org/558213002/diff/20001/Source/core/frame/LocalD...
> > File Source/core/frame/LocalDOMWindow.h (right):
> > 
> >
>
https://codereview.chromium.org/558213002/diff/20001/Source/core/frame/LocalD...
> > Source/core/frame/LocalDOMWindow.h:368:
> > WillBeHeapHashSet<RawPtrWillBeMember<DOMWindowProperty> > m_properties;
> > On 2014/09/10 15:52:56, Mads Ager (chromium) wrote:
> > > What makes sure that a DOMWindowProperty cannot die before the
> LocalDOMWindow
> > > and therefore the notification that the global object is being destroyed?
> > > 
> > > It makes sense for the DOMWindowProperties that are LocalDOMWindow
> > supplements.
> > > However, there are other DOMWIndowProperties such as BarProp that are not
> > > LocalDOMWindow supplements and I can't convince myself that they cannot
die
> > > before the LocalDOMWindow in the non-oilpan build?
> > > 
> > > (That also reopens the question that we had on the oilpan part of this
code.
> I
> > > said that I would prefer a strong relationship between the
DOMWindowProperty
> > and
> > > its LocalDOMWindow. However, if there are short lived DOMWindowProperties,
> we
> > > might not want to have the LocalDOMWindow keep them alive. If that is the
> case
> > > we should move to weak processing in the oilpan build to remove from this
> > set.)
> > 
> > Heh, ignore me. All of those properties are held alive by the list of
RefPtrs
> > below. :-)
> 
> Bad idea not take that concern fully aboard :) DOMSelection objects live & die
> independently of any LocalDOMWindow, so they will need to  be able to
> detach/unregister as a DOMWindowProperty when being finalized. Other
> DOMWindowProperty-derived objects appear kept alive via LocalDOMWindow.
> 
> So this optimization isn't valid in general _and_ I think we do need to move
to
> having LocalDOMWindow::m_properties be a weak set. (The use of strong
references
> hid this DOMSelection-involved bug with Oilpan enabled.)

Yeah, that makes sense. Sorry for not being more pushy on this. I checked a
handful that looked safe; I should have checked them all. Let's make it a weak
set for oilpan.