Separate GeolocationWatchers from Geolocation

Separate GeolocationWatchers from Geolocation

There are some classes in the Geolocation.h/cpp files.
It makes hard to read geolocation implementation, therefore separate
GeolocationWatchers from Geolocation is better to understand
geolocation's logic.

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

[kihong, 2014-07-17 08:03:22]

PTAL

[Michael van Ouwerkerk, 2014-07-18 12:07:39]

lgtm with nits, I trust you will address them (fix or further discussion) before
submitting.

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

https://codereview.chromium.org/402563002/diff/1/Source/modules/geolocation/G...
Source/modules/geolocation/GeoNotifier.h:9: #include
"modules/geolocation/PositionErrorCallback.h"
These changes do not seem necessary. Using forward declarations is a good thing.

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

https://codereview.chromium.org/402563002/diff/1/Source/modules/geolocation/G...
Source/modules/geolocation/Geolocation.h:34: #include
"modules/geolocation/GeolocationWatchers.h"
I think you can forward declare GeolocationWatchers instead of including it
here.

[kihong, 2014-07-18 14:28:25]

PTAL, Thank you~ Michael :-)

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

https://codereview.chromium.org/402563002/diff/1/Source/modules/geolocation/G...
Source/modules/geolocation/GeoNotifier.h:9: #include
"modules/geolocation/PositionErrorCallback.h"
On 2014/07/18 12:07:39, Michael van Ouwerkerk wrote:
> These changes do not seem necessary. Using forward declarations is a good
thing.

It's better to use forward declarations, but we cannot do that because these are
working with OwnPtr.
They makes build error when it included by GeolocationWhatcher.cpp.
If we change OwnPtr to Member, we could use forward declarations but current
callback implementation using code generator only support OwnPtr.
That's why I changed them to "include" instead of forward declarations.

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

https://codereview.chromium.org/402563002/diff/1/Source/modules/geolocation/G...
Source/modules/geolocation/Geolocation.h:34: #include
"modules/geolocation/GeolocationWatchers.h"
On 2014/07/18 12:07:39, Michael van Ouwerkerk wrote:
> I think you can forward declare GeolocationWatchers instead of including it
> here.

You are right, I made a mistake.
I will change it before landing this patch.

[kihong, 2014-07-18 15:36:06]

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

https://codereview.chromium.org/402563002/diff/1/Source/modules/geolocation/G...
Source/modules/geolocation/Geolocation.h:34: #include
"modules/geolocation/GeolocationWatchers.h"
On 2014/07/18 14:28:25, kihong wrote:
> On 2014/07/18 12:07:39, Michael van Ouwerkerk wrote:
> > I think you can forward declare GeolocationWatchers instead of including it
> > here.
> 
> You are right, I made a mistake.
> I will change it before landing this patch.

I have got misunderstanding, I think you've got it also.
m_watchers is not a pointer variable, so we cannot use it by forward
declaration. :)

[kihong, 2014-07-18 15:36:59]

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

https://codereview.chromium.org/402563002/diff/1/Source/modules/geolocation/G...
Source/modules/geolocation/Geolocation.cpp:39: #include
"modules/geolocation/GeolocationWatchers.h"
This is duplicated include.
I will remove this before landing this.

[kihong, 2014-07-22 09:03:58]

Hi Michael,
Could you take a look at my comment please?
IMHO, it's better to land this after getting your opinion. :-)

[Michael van Ouwerkerk, 2014-07-22 09:49:53]

Oh I'm sorry - I agree with you, please go ahead with your plan!

[kihong, 2014-07-22 23:48:23]

The CQ bit was checked by kihong.kwon@samsung.com

[commit-bot: I haz the power, 2014-07-22 23:49:21]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/kihong.kwon@samsung.com/402563002/20001

[commit-bot: I haz the power, 2014-07-22 23:52:41]

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

[commit-bot: I haz the power, 2014-07-22 23:55:10]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-07-22 23:55:11]

Try jobs failed on following builders:
  blink_presubmit on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/blink_presubmit/builds/1...)

[kihong, 2014-07-23 01:54:40]

The CQ bit was checked by kihong.kwon@samsung.com

[commit-bot: I haz the power, 2014-07-23 01:55:30]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/kihong.kwon@samsung.com/402563002/40001

[commit-bot: I haz the power, 2014-07-23 06:21:14]

Change committed as 178737