Bug 36315 - Remove obsolete Geolocation::m_currentPosition
Summary: Remove obsolete Geolocation::m_currentPosition
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebCore Misc. (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2010-03-18 12:04 PDT by Marcus Bulach
Modified: 2010-04-21 11:54 PDT (History)
6 users (show)

See Also:


Attachments
Patch (3.15 KB, patch)
2010-03-18 14:16 PDT, Marcus Bulach
jorlow: review-
jorlow: commit-queue-
Details | Formatted Diff | Diff
Patch (3.17 KB, patch)
2010-03-29 07:58 PDT, Marcus Bulach
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Marcus Bulach 2010-03-18 12:04:55 PDT
A follow up on
https://bugs.webkit.org/show_bug.cgi?id=30676
needs to remove Geolocation::m_currentPosition.

Main reason:
void Geolocation::setIsAllowed(bool allowed)
...
       436         if (lastPosition())
       437             makeSuccessCallbacks();
...

but makeSuccessCallbacks() uses m_currentPosition rather than lastPosition(), and asserts if it's not set.
Comment 1 Marcus Bulach 2010-03-18 14:16:33 PDT
Created attachment 51091 [details]
Patch
Comment 2 WebKit Review Bot 2010-03-23 10:16:11 PDT
Attachment 51091 [details] did not pass style-queue:

Failed to run "WebKitTools/Scripts/check-webkit-style" exit_code: 1
WebCore/ChangeLog:6:  Line contains tab character.  [whitespace/tab] [5]
WebCore/ChangeLog:7:  Line contains tab character.  [whitespace/tab] [5]
WebCore/ChangeLog:8:  Line contains tab character.  [whitespace/tab] [5]
Total errors found: 3 in 4 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 3 Steve Block 2010-03-23 12:06:21 PDT
Comment on attachment 51091 [details]
Patch

+		Remove obsolete Geolocation::m_currentPosition
+		(follow up on https://bugs.webkit.org/show_bug.cgi?id=30676)
tabs

Looks good to me otherwise. To further clarify the code, we could remove the Geoposition argument from positionChanged() and hence setPosition(). Instead, positionChanged() would just call m_service->lastPosition() directly. This would also bring the client-based code path in line with the existing code path. 

Also, I think that in a future change we should remove m_lastPosition, as it serves no purpose as far as I can see.
Comment 4 Jeremy Orlow 2010-03-23 12:15:41 PDT
Comment on attachment 51091 [details]
Patch

r- per Steve's comments...looks close tho
Comment 5 Marcus Bulach 2010-03-29 07:58:28 PDT
Created attachment 51910 [details]
Patch
Comment 6 Marcus Bulach 2010-03-29 08:05:11 PDT
(In reply to comment #3)
> (From update of attachment 51091 [details])
> +        Remove obsolete Geolocation::m_currentPosition
> +        (follow up on https://bugs.webkit.org/show_bug.cgi?id=30676)
> tabs
> 

Ops, fixed.

> Looks good to me otherwise. To further clarify the code, we could remove the
> Geoposition argument from positionChanged() and hence setPosition(). Instead,
> positionChanged() would just call m_service->lastPosition() directly. This
> would also bring the client-based code path in line with the existing code
> path. 

Hmm, I don't quite understood this bit: setPosition is part of the public API, I'm not sure if I can just remove GeolocationPosition (not Geoposition) from it. If you're saying to remove only from the call to positionChanged(), that would require pushing up "m_positionCache->setCachedPosition(newPosition.get());" to the two APIs.

Since this would be just a further clarification, I hope you don't mind if I do as a follow up?

> 
> Also, I think that in a future change we should remove m_lastPosition, as it
> serves no purpose as far as I can see.

Agreed. Right now, it only serves to ensure ownership of either the service or client-based geoposition, but clarifying it would definitely be a good idea.
Comment 7 Jeremy Orlow 2010-03-29 08:12:59 PDT
Comment on attachment 51910 [details]
Patch

r=me

Thanks for the review Steve.
Comment 8 WebKit Commit Bot 2010-03-29 09:56:09 PDT
Comment on attachment 51910 [details]
Patch

Clearing flags on attachment: 51910

Committed r56726: <http://trac.webkit.org/changeset/56726>
Comment 9 WebKit Commit Bot 2010-03-29 09:56:14 PDT
All reviewed patches have been landed.  Closing bug.
Comment 10 Steve Block 2010-04-21 11:54:28 PDT
> > Looks good to me otherwise. To further clarify the code, we could remove the
> > Geoposition argument from positionChanged() and hence setPosition(). Instead,
> > positionChanged() would just call m_service->lastPosition() directly. This
> > would also bring the client-based code path in line with the existing code
> > path. 
> 
> Hmm, I don't quite understood this bit: setPosition is part of the public API,
> I'm not sure if I can just remove GeolocationPosition (not Geoposition) from
> it. If you're saying to remove only from the call to positionChanged(), that
> would require pushing up
> "m_positionCache->setCachedPosition(newPosition.get());" to the two APIs.
Sorry for not being more clear. My point is: given that we no longer use m_currentPosition, and makeSuccessCallbacks() now calls lastPosition() to get the position directly from the service, positionChanged() could do this too - i.e. call lastPosition() rather than needing the position to be passed in as a parameter. So we would remove the Geoposition parameter from positionChanged().

This would allow us to remove the GeolocationPosition parameter from setPosition(). This would bring the client-based implementation (which uses setPosition()) into line with the existing implementation (which uses geolocationServicePositionChanged(), which does not take a position parameter).

Does that make sense?

> Since this would be just a further clarification, I hope you don't mind if I do
> as a follow up?
Sure, no problem