Oilpan: Move GeolocationController back to off-heap

Oilpan: Move GeolocationController back to off-heap

r175404 broke oilpan builds, because r175404 switched GeolocationController from being a Page (which is on-heap) supplement to a LocalFrame (which is not yet on-heap) supplement.

As a quick fix, this CL moves GeolocationController back to off-heap.

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

[haraken, 2014-06-04 01:01:52]

PTAL

[haraken, 2014-06-04 01:03:19]

https://codereview.chromium.org/314873002/diff/1/Source/modules/geolocation/G...
File Source/modules/geolocation/GeolocationController.h (right):

https://codereview.chromium.org/314873002/diff/1/Source/modules/geolocation/G...
Source/modules/geolocation/GeolocationController.h:52: void
removeObserver(Geolocation*);

jam@, michael@: I found that no one is calling removeObserver(). Won't it leak
the Geolocation objects in the ObserverSet?

[tkent, 2014-06-04 01:08:32]

lgtm

[haraken, 2014-06-04 01:15:25]

The CQ bit was checked by haraken@chromium.org

[commit-bot: I haz the power, 2014-06-04 01:16:12]

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

[jabdelmalek, 2014-06-04 01:24:19]

thanks (this change looks like magic to me, so i'm not qualified to review, and
i'm also not an owner there)

removeObserver is being called still:
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...

[haraken, 2014-06-04 01:25:26]

On 2014/06/04 01:24:19, jabdelmalek wrote:
> thanks (this change looks like magic to me, so i'm not qualified to review,
and
> i'm also not an owner there)
> 
> removeObserver is being called still:
>
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...

oh, I missed that (the code search didn't point the caller site). Thanks!

[commit-bot: I haz the power, 2014-06-04 02:44:56]

Change committed as 175430

[Mads Ager (chromium), 2014-06-04 06:20:15]

LGTM

https://codereview.chromium.org/314873002/diff/1/Source/modules/geolocation/G...
File Source/modules/geolocation/GeolocationController.cpp (right):

https://codereview.chromium.org/314873002/diff/1/Source/modules/geolocation/G...
Source/modules/geolocation/GeolocationController.cpp:157:
WillBePersistentHeapVector<RefPtrWillBeMember<Geolocation> > observersVector;
This is on the stack, so we should be able to still use WillBeHeapVector here?
There is no need for the persistent on the stack. The conservative stack
scanning will find the temporary vector.

https://codereview.chromium.org/314873002/diff/1/Source/modules/geolocation/G...
Source/modules/geolocation/GeolocationController.cpp:165:
WillBePersistentHeapVector<RefPtrWillBeMember<Geolocation> > observersVector;
Ditto.

[haraken, 2014-06-04 09:45:28]

On 2014/06/04 06:20:15, Mads Ager (chromium) wrote:
> LGTM
> 
>
https://codereview.chromium.org/314873002/diff/1/Source/modules/geolocation/G...
> File Source/modules/geolocation/GeolocationController.cpp (right):
> 
>
https://codereview.chromium.org/314873002/diff/1/Source/modules/geolocation/G...
> Source/modules/geolocation/GeolocationController.cpp:157:
> WillBePersistentHeapVector<RefPtrWillBeMember<Geolocation> > observersVector;
> This is on the stack, so we should be able to still use WillBeHeapVector here?
> There is no need for the persistent on the stack. The conservative stack
> scanning will find the temporary vector.
> 
>
https://codereview.chromium.org/314873002/diff/1/Source/modules/geolocation/G...
> Source/modules/geolocation/GeolocationController.cpp:165:
> WillBePersistentHeapVector<RefPtrWillBeMember<Geolocation> > observersVector;
> Ditto.

Thanks for review. Addressed it in a follow-up:
https://codereview.chromium.org/316823003/