Remove never reached area in GeoNotifier

Remove never reached area in GeoNotifier

Allowance checking in the runSuccessCallback is useless and annoying.
It is already checked right before calling.

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

[kihong, 2014-07-17 02:23:30]

PTAL

[haraken, 2014-07-17 02:27:07]

LGTM

[kihong, 2014-07-17 02:30:01]

On 2014/07/17 02:27:07, haraken wrote:
> LGTM

Thank you for quick replying :-)

[kihong, 2014-07-17 02:30:11]

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

[commit-bot: I haz the power, 2014-07-17 02:31:34]

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

[commit-bot: I haz the power, 2014-07-17 03:51:29]

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

[commit-bot: I haz the power, 2014-07-17 04:25:09]

Change committed as 178317

[Michael van Ouwerkerk, 2014-07-17 10:15:00]

https://codereview.chromium.org/402483003/diff/1/Source/modules/geolocation/G...
File Source/modules/geolocation/GeoNotifier.cpp (left):

https://codereview.chromium.org/402483003/diff/1/Source/modules/geolocation/G...
Source/modules/geolocation/GeoNotifier.cpp:62: CRASH();
I'd have prefered to just replace this with ASSERT(m_geolocation->isAllowed());

While it is true that currently this is dead code, there are multiple paths to
this method. Also, a lot of code in Geolocation is asynchronous. This means that
it seems possible for regressions to occur, and an ASSERT is the easiest way to
read the code and verify the behavior.

Are you removing this to get rid of the m_geolocation member? It seems an
unfortunate side effect to lose code readability and robustness.

[kihong, 2014-07-17 11:47:56]

I am sorry for landing this so quickly. I have thought this is so trivial.

https://codereview.chromium.org/402483003/diff/1/Source/modules/geolocation/G...
File Source/modules/geolocation/GeoNotifier.cpp (left):

https://codereview.chromium.org/402483003/diff/1/Source/modules/geolocation/G...
Source/modules/geolocation/GeoNotifier.cpp:62: CRASH();
On 2014/07/17 10:15:00, Michael van Ouwerkerk wrote:
> I'd have prefered to just replace this with
ASSERT(m_geolocation->isAllowed());
> 
> While it is true that currently this is dead code, there are multiple paths to
> this method. Also, a lot of code in Geolocation is asynchronous. This means
that
> it seems possible for regressions to occur, and an ASSERT is the easiest way
to
> read the code and verify the behavior.

There are only two way to get here.(from makeCachedPositionCallbacks() or
endPosition())
And allowance checking have been done right before calling runSuccessCallback()
synchronously.
So, I don't think there is possible to get here without allowance.
But if you think we need this, I have no problem with reverting or changing to
ASSERT.

[Michael van Ouwerkerk, 2014-07-17 12:15:34]

https://codereview.chromium.org/402483003/diff/1/Source/modules/geolocation/G...
File Source/modules/geolocation/GeoNotifier.cpp (left):

https://codereview.chromium.org/402483003/diff/1/Source/modules/geolocation/G...
Source/modules/geolocation/GeoNotifier.cpp:62: CRASH();
Your change is correct. If the purpose is the break the cycle, then this seems
like a reasonable change. I was just expressing regret at losing some
readability of the code.

[kihong, 2014-07-17 12:21:10]

https://codereview.chromium.org/402483003/diff/1/Source/modules/geolocation/G...
File Source/modules/geolocation/GeoNotifier.cpp (left):

https://codereview.chromium.org/402483003/diff/1/Source/modules/geolocation/G...
Source/modules/geolocation/GeoNotifier.cpp:62: CRASH();
On 2014/07/17 12:15:34, Michael van Ouwerkerk wrote:
> Your change is correct. If the purpose is the break the cycle, then this seems
> like a reasonable change. I was just expressing regret at losing some
> readability of the code.

I got it and thanks for your kind reviewing. :)