Bug 38195 - Bring CLIENT_BASED_GEOLOCATION more in line with non-client based: remove Geolocation::setPosition and use lastPosition()
Summary: Bring CLIENT_BASED_GEOLOCATION more in line with non-client based: remove Geo...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebCore Misc. (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Minor
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2010-04-27 06:39 PDT by Marcus Bulach
Modified: 2010-05-12 01:44 PDT (History)
3 users (show)

See Also:


Attachments
Patch (12.92 KB, patch)
2010-04-27 06:45 PDT, Marcus Bulach
no flags Details | Formatted Diff | Diff
Patch (5.79 KB, patch)
2010-05-04 11:40 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-04-27 06:39:08 PDT
Non-client based geolocation uses "lastPosition()" rather than storing via "setPosition()".
Client-based should have a similar mechanism.
Comment 1 Marcus Bulach 2010-04-27 06:45:42 PDT
Created attachment 54413 [details]
Patch
Comment 2 Marcus Bulach 2010-04-27 07:06:16 PDT
Hi Steve,

This is a follow up on https://bugs.webkit.org/show_bug.cgi?id=36315, thanks for the clarification!

Would you mind taking a look please?
Comment 3 Darin Adler 2010-04-27 13:00:46 PDT
Comment on attachment 54413 [details]
Patch

The whitespace changes in the patch are far larger than the substance of the patch.

Does this change behavior? Is there a way to test this?
Comment 4 Steve Block 2010-04-30 08:55:15 PDT
Comment on attachment 54413 [details]
Patch

> - * OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE. 
> + * OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
I think it's best to leave the whitespace changes out of this patch for clarity.

> +    if (m_lastPosition.get())
> +        return m_lastPosition.get();
Is this caching required for correct behaviour or is it just an optimisation? GeolocationController::lastPosition() should only be called once the client has called GeolocationController::positionChanged(), and it seems that the client should always have a last position available by this time.
Comment 5 Marcus Bulach 2010-05-04 11:40:45 PDT
Created attachment 55031 [details]
Patch
Comment 6 Marcus Bulach 2010-05-04 11:44:06 PDT
(In reply to comment #3)
> (From update of attachment 54413 [details])
> The whitespace changes in the patch are far larger than the substance of the
> patch.

yep, sorry about that: my editor insisted in removing all trailing ws. :(
the latest patch is less invasive.

> 
> Does this change behavior? Is there a way to test this?
it's just a minor refactoring as suggested by Steve at https://bugs.webkit.org/show_bug.cgi?id=36315
there should be no changes in behavior.
Comment 7 Marcus Bulach 2010-05-04 11:48:52 PDT
(In reply to comment #4)
> (From update of attachment 54413 [details])
> > - * OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE. 
> > + * OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
> I think it's best to leave the whitespace changes out of this patch for
> clarity.
yep, reverted the ws changes.

> 
> > +    if (m_lastPosition.get())
> > +        return m_lastPosition.get();
> Is this caching required for correct behaviour or is it just an optimisation?
> GeolocationController::lastPosition() should only be called once the client has
> called GeolocationController::positionChanged(), and it seems that the client
> should always have a last position available by this time.

AFAICT GeolocationController::m_client is optional, which means we need to hold on the position passed via the public method GeolocationController::positionChanged(GeolocationPosition* position), that is, this method may be called directly without a client.

I'm not entirely familiar with this part, so please, let me know if I'm wrong, or who else could help taking a look at it!
Comment 8 WebKit Commit Bot 2010-05-12 01:43:59 PDT
Comment on attachment 55031 [details]
Patch

Clearing flags on attachment: 55031

Committed r59216: <http://trac.webkit.org/changeset/59216>
Comment 9 WebKit Commit Bot 2010-05-12 01:44:04 PDT
All reviewed patches have been landed.  Closing bug.