Oilpan: Replace RefPtrs to Document in core/{accessibility,animation,fetch} with Oilpan transition …

Oilpan: Replace RefPtrs to Document in core/{accessibility,animation,fetch} with Oilpan transition types.

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

[tkent, 2014-06-04 06:06:02]

Please review this.

[haraken, 2014-06-04 06:09:47]

LGTM

[dmazzoni, 2014-06-04 06:27:26]

https://codereview.chromium.org/313023003/diff/1/Source/core/accessibility/AX...
File Source/core/accessibility/AXObjectCache.cpp (right):

https://codereview.chromium.org/313023003/diff/1/Source/core/accessibility/AX...
Source/core/accessibility/AXObjectCache.cpp:694: RefPtrWillBeRawPtr<Document>
protectorForCacheOwner(m_document);
In this particular case, the RefPtr is purely there to protect m_document from
being destroyed due to losing all reference counts - post-Oilpan we won't need
this line at all. Is there a transitional type to indicate that, or does it
deserve a comment? Or are we just going to fix these individually?

[Mads Ager (chromium), 2014-06-04 06:30:13]

LGTM

https://codereview.chromium.org/313023003/diff/1/Source/core/accessibility/AX...
File Source/core/accessibility/AXObjectCache.cpp (right):

https://codereview.chromium.org/313023003/diff/1/Source/core/accessibility/AX...
Source/core/accessibility/AXObjectCache.cpp:694: RefPtrWillBeRawPtr<Document>
protectorForCacheOwner(m_document);
On 2014/06/04 06:27:26, dmazzoni wrote:
> In this particular case, the RefPtr is purely there to protect m_document from
> being destroyed due to losing all reference counts - post-Oilpan we won't need
> this line at all. Is there a transitional type to indicate that, or does it
> deserve a comment? Or are we just going to fix these individually?

Yes, exactly, this just have to go when we can turn oilpan on by default.

Most of them are called protect*, so when we turn on oilpan we will run through
the code base looking for raw pointers called protect* and get rid of them. So
in order to not have the same comment all over the source base we are letting
the protect name be the comment for us. :)

[haraken, 2014-06-04 06:31:50]

https://codereview.chromium.org/313023003/diff/1/Source/core/accessibility/AX...
File Source/core/accessibility/AXObjectCache.cpp (right):

https://codereview.chromium.org/313023003/diff/1/Source/core/accessibility/AX...
Source/core/accessibility/AXObjectCache.cpp:694: RefPtrWillBeRawPtr<Document>
protectorForCacheOwner(m_document);
On 2014/06/04 06:27:26, dmazzoni wrote:
> In this particular case, the RefPtr is purely there to protect m_document from
> being destroyed due to losing all reference counts - post-Oilpan we won't need
> this line at all. Is there a transitional type to indicate that, or does it
> deserve a comment? Or are we just going to fix these individually?

You're right. We've discussed introducing RefPtrWillBeRemoved, adding a FIXME
etc, but we decided to leave RefPtrWillBeRawPtr as is, because it would be easy
to grep & remove these pointers when switching to oilpan.

Other examples we'll remove in post-oilpan is:

void foo(PassRefPtrWillBeRawPtr<X> prp) {
  RefPtrWillBeRawPtr<X> p = prp;
  ...;
}

[zerny-chromium, 2014-06-04 06:31:52]

lgtm

https://codereview.chromium.org/313023003/diff/1/Source/core/accessibility/AX...
File Source/core/accessibility/AXObjectCache.cpp (right):

https://codereview.chromium.org/313023003/diff/1/Source/core/accessibility/AX...
Source/core/accessibility/AXObjectCache.cpp:694: RefPtrWillBeRawPtr<Document>
protectorForCacheOwner(m_document);
On 2014/06/04 06:27:26, dmazzoni wrote:
> In this particular case, the RefPtr is purely there to protect m_document from
> being destroyed due to losing all reference counts - post-Oilpan we won't need
> this line at all. Is there a transitional type to indicate that, or does it
> deserve a comment? Or are we just going to fix these individually?

The RefPtrWillBeRaw<T> protect(foo) is sufficient. After transitioning this will
become T* protect(foo) and we will remove it completely then. (I think clang
will give us an unused warning for code like that.)

[dmazzoni, 2014-06-04 06:32:12]

On Tue, Jun 3, 2014 at 11:30 PM, <ager@chromium.org> wrote:

> Most of them are called protect*, so when we turn on oilpan we will run
> through the code base looking for raw pointers called protect* and get
> rid of them. So in order to not have the same comment all over the
> source base we are letting the protect name be the comment for us. :)
>

Great, thanks for the explanation.

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

[tkent, 2014-06-04 07:53:57]

The CQ bit was checked by tkent@chromium.org

[commit-bot: I haz the power, 2014-06-04 07:55:03]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/tkent@chromium.org/313023003/1

[commit-bot: I haz the power, 2014-06-04 09:01:30]

Change committed as 175456