Move GeolocationClient to the Oilpan heap.

Move GeolocationClient to the Oilpan heap.

And as a result, willBeDestroyed() over Supplementables and Supplements
can now be removed.

R=ager,haraken,tkent
BUG=340522
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=180944

[sof, 2014-08-21 09:43:42]

Please take a look.

I think this is now ready -- complete test results for a 'development' version
of this CL can currently be found via https://codereview.chromium.org/490983004/

https://codereview.chromium.org/490143002/diff/1/Source/modules/geolocation/G...
File Source/modules/geolocation/GeolocationClient.h (left):

https://codereview.chromium.org/490143002/diff/1/Source/modules/geolocation/G...
Source/modules/geolocation/GeolocationClient.h:38: virtual void
geolocationDestroyed() = 0;
If this CL goes ahead, geolocationDestroyed() can be removed also. Will have to
be dealt with in one (or more) followups.

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

https://codereview.chromium.org/490143002/diff/1/Source/modules/geolocation/G...
Source/modules/geolocation/GeolocationClient.h:38: class GeolocationClient :
public NoBaseWillBeGarbageCollectedFinalized<GeolocationClient> {
It is arguably moot to insist on *Finalized here (for the benefit of the 'mock'
subclass only), but I kept it as such for now. Not many of these objects in
existence.

[sof, 2014-08-21 13:31:17]

https://codereview.chromium.org/490143002/diff/1/Source/web/WebLocalFrameImpl...
File Source/web/WebLocalFrameImpl.cpp (left):

https://codereview.chromium.org/490143002/diff/1/Source/web/WebLocalFrameImpl...
Source/web/WebLocalFrameImpl.cpp:506: m_frame->willBeDestroyed();
We've been looking into why this notification was here and why it is safe to
remove it.

The embedder calls close() to delete the frame, and will do this last thing.
Hence, sending a notification back that the geolocation controller is going away
is redundant. The embedder will not touch the geolocation client via its
dispatcher afterwards.

There are/were no other supplement overrides of willBeDestroyed() besides for
GeolocationController, hence this removal is safe. The geolocationDestroyed()
call can be retired in a followup.

[Mads Ager (chromium), 2014-08-21 13:54:45]

LGTM

We might want to make the WebGeolocationController's raw pointer to the
GeolocationController a root for the GC. It will be alive whenever we care about
it via the frame but we should probably trace it here.

Unfortunately it seems like we are leaking the WebGeolocationController because
we are leaking the GeolocationDispatcher? At least I cannot see a matching
delete for the new in render_frame_impl.cc

[sof, 2014-08-21 14:29:40]

On 2014/08/21 13:54:45, Mads Ager (chromium) wrote:
> LGTM
> 
> We might want to make the WebGeolocationController's raw pointer to the
> GeolocationController a root for the GC. It will be alive whenever we care
about
> it via the frame but we should probably trace it here.
> 
> Unfortunately it seems like we are leaking the WebGeolocationController
because
> we are leaking the GeolocationDispatcher? At least I cannot see a matching
> delete for the new in render_frame_impl.cc

GeolocationDispatcher is a RenderFrameObserver, so when it has closed the
WebFrame and goes away, it'll delete itself, so no leak.

(As I just discovered.)

[sof, 2014-08-21 15:26:08]

On 2014/08/21 13:54:45, Mads Ager (chromium) wrote:
> LGTM
> 
> We might want to make the WebGeolocationController's raw pointer to the
> GeolocationController a root for the GC. It will be alive whenever we care
about
> it via the frame but we should probably trace it here.
> 

I've added a comment next to it regarding ownership, for now.

Wrapping up the reference in a WebPrivatePtr<> can't be readily done, it would
at least need another wrapper class to provide a RefCounted<> view of it in the
non-Oilpan case (as GeolocationController and other Supplements are
OwnPtr-based.) I'm not sure that's worth it, will think about it.

I'm hoping tkent has the time to look at this CL, as he did geolocation/
originally + there being Source/{web,platform}/ changes here.

[haraken, 2014-08-21 15:47:25]

LGTM

[sof, 2014-08-22 08:42:27]

https://codereview.chromium.org/490143002/diff/40001/public/web/WebGeolocatio...
File public/web/WebGeolocationClient.h (right):

https://codereview.chromium.org/490143002/diff/40001/public/web/WebGeolocatio...
public/web/WebGeolocationClient.h:42: virtual void geolocationDestroyed() { }
To prepare for any Chromium-side removal of geolocationDestroyed(), made the
default impl empty rather than abstract.

[sof, 2014-08-25 14:35:43]

web/ ping.

[sof, 2014-08-27 05:30:24]

sigbjornf@opera.com changed reviewers:
+ tkent@chromium.org

[sof, 2014-08-27 05:30:24]

tkent@: could you have a look at {web,platform}, please?

[tkent, 2014-08-27 06:06:17]

lgtm

[sof, 2014-08-27 06:41:20]

The CQ bit was checked by sigbjornf@opera.com

[commit-bot: I haz the power, 2014-08-27 06:42:19]

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

[commit-bot: I haz the power, 2014-08-27 07:46:15]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  mac_gpu_retina_triggered_tests on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/mac_gpu_retina_tr...)

[commit-bot: I haz the power, 2014-08-27 08:08:44]

Committed patchset #3 (40001) as 180944