[oilpan] Figure out lifetime of remaining Node raw pointers (Part 2)

[oilpan] Figure out lifetime of remaining Node raw pointers (Part 2)

R=ager@chromium.org, zerny@chromium.org

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

[haraken, 2013-10-01 01:10:23]

PTAL

[Mads Ager (chromium), 2013-10-01 10:14:54]

LGTM except for comment change in CheckedRadioButtons.cpp.

https://codereview.chromium.org/25403004/diff/4001/Source/core/dom/CheckedRad...
File Source/core/dom/CheckedRadioButtons.cpp (right):

https://codereview.chromium.org/25403004/diff/4001/Source/core/dom/CheckedRad...
Source/core/dom/CheckedRadioButtons.cpp:48: // FIXME(oilpan): This should be a
weak hash set.
I don't think should be weak. These should be strong pointers. These mappings
are explicitly managed independently of object life-time. Objects are removed
from here when removed from the document and otherwise everything dies together.
No need for weak processing.

[zerny-chromium, 2013-10-01 10:18:37]

lgtm

https://codereview.chromium.org/25403004/diff/4001/Source/core/dom/CheckedRad...
File Source/core/dom/CheckedRadioButtons.cpp (right):

https://codereview.chromium.org/25403004/diff/4001/Source/core/dom/CheckedRad...
Source/core/dom/CheckedRadioButtons.cpp:51: HTMLInputElement* m_checkedButton;
Why should these be weak pointers?

https://codereview.chromium.org/25403004/diff/4001/Source/core/html/HTMLInput...
File Source/core/html/HTMLInputElement.cpp (right):

https://codereview.chromium.org/25403004/diff/4001/Source/core/html/HTMLInput...
Source/core/html/HTMLInputElement.cpp:101: // FIXME(oilpan): This should be a
strong pointer.
Just to be sure: we want a weak pointer from the registry to the listener and
then we can use a strong pointer from the listener to the element?

https://codereview.chromium.org/25403004/diff/4001/Source/core/html/MediaDocu...
File Source/core/html/MediaDocument.cpp (right):

https://codereview.chromium.org/25403004/diff/4001/Source/core/html/MediaDocu...
Source/core/html/MediaDocument.cpp:69: // FIXME(oilpan): This is a strong
pointer.
I'll fix this in a follow-up CL after moving over the parsers.

[haraken, 2013-10-01 16:46:52]

Thanks for reviewing!

https://codereview.chromium.org/25403004/diff/4001/Source/core/dom/CheckedRad...
File Source/core/dom/CheckedRadioButtons.cpp (right):

https://codereview.chromium.org/25403004/diff/4001/Source/core/dom/CheckedRad...
Source/core/dom/CheckedRadioButtons.cpp:48: // FIXME(oilpan): This should be a
weak hash set.
On 2013/10/01 10:14:55, Mads Ager (chromium) wrote:
> I don't think should be weak. These should be strong pointers. These mappings
> are explicitly managed independently of object life-time. Objects are removed
> from here when removed from the document and otherwise everything dies
together.
> No need for weak processing.

Right. Fixed.

https://codereview.chromium.org/25403004/diff/4001/Source/core/dom/CheckedRad...
Source/core/dom/CheckedRadioButtons.cpp:51: HTMLInputElement* m_checkedButton;
On 2013/10/01 10:18:37, zerny-chromium wrote:
> Why should these be weak pointers?

Right. This should be a strong pointer. Thanks.

https://codereview.chromium.org/25403004/diff/4001/Source/core/html/HTMLInput...
File Source/core/html/HTMLInputElement.cpp (right):

https://codereview.chromium.org/25403004/diff/4001/Source/core/html/HTMLInput...
Source/core/html/HTMLInputElement.cpp:101: // FIXME(oilpan): This should be a
strong pointer.
On 2013/10/01 10:18:37, zerny-chromium wrote:
> Just to be sure: we want a weak pointer from the registry to the listener and
> then we can use a strong pointer from the listener to the element?

Yes, updated the comment to clarify that.

[haraken, 2013-10-01 16:47:45]

Committed patchset #2 manually as r158639.

[haraken, 2013-10-01 17:20:07]

On 2013/10/01 10:14:54, Mads Ager (chromium) wrote:
> LGTM except for comment change in CheckedRadioButtons.cpp.
> 
>
https://codereview.chromium.org/25403004/diff/4001/Source/core/dom/CheckedRad...
> File Source/core/dom/CheckedRadioButtons.cpp (right):
> 
>
https://codereview.chromium.org/25403004/diff/4001/Source/core/dom/CheckedRad...
> Source/core/dom/CheckedRadioButtons.cpp:48: // FIXME(oilpan): This should be a
> weak hash set.
> I don't think should be weak. These should be strong pointers. These mappings
> are explicitly managed independently of object life-time. Objects are removed
> from here when removed from the document and otherwise everything dies
together.
> No need for weak processing.

Sorry, I got a bit confused. I wonder if they are weak pointers.

Imagine that there are 3 input elements managed by a CheckedButtonGroup. If the
first input element is removed from the document, the element should be removed
from the hash set (i.e., m_members). However, the CheckedButtonGroup should
still be kept alive and the hash set should contain the other two elements. So
the hash set should be weak, shouldn't it?

[haraken, 2013-10-02 00:22:01]

> Sorry, I got a bit confused. I wonder if they are weak pointers.
> 
> Imagine that there are 3 input elements managed by a CheckedButtonGroup. If
the
> first input element is removed from the document, the element should be
removed
> from the hash set (i.e., m_members). However, the CheckedButtonGroup should
> still be kept alive and the hash set should contain the other two elements. So
> the hash set should be weak, shouldn't it?

Now I understand :) removedFrom() is not related to the lifetime of objects, so
the clear-out is not a weak processing. Sorry about the noise.