[Webkit-unassigned] [Bug 64970] [Gtk] Support for client-based geolocation

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=64970





--- Comment #12 from Zan Dobersek <zandobersek at gmail.com>  2011-10-07 11:09:54 PST ---
(In reply to comment #11)
> (From update of attachment 103182 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=103182&action=review
> 
> Sorry for letting this sit so long. I didn't really understand what was going on here at first. In general it looks good to me. I think that if this is a feature-preserving replacement for the old system, we should just remove the old system completely.
> 

I'm partly to blame for that - I've thought a bit about using geoclue instead of the usual mock client as described in comment #9 and then pretty much forgot about it. Thinking about it now, it would be an overkill to develop a provider that would then be deployed on the buildbots. It would also probably cause problems when developers would want to run the tests locally. So, to sum it up, I don't believe it would pay off.

> > Source/WebCore/platform/gtk/GeolocationServiceGtk.cpp:22
> > -#if ENABLE(GEOLOCATION)
> > +#if ENABLE(GEOLOCATION) && !ENABLE(CLIENT_BASED_GEOLOCATION)
> 
> Is client-based geolocation is a full replacement for this, why not remove this file and make it always on? Is there any loss in functionality?
> 

There's no loss in functionality, but the service-based system could be left as a backend at least for a short amount of time, until the client-based solution is showing consistent passing of related tests on the bots. After that, the file would eventually be removed.

> > Source/WebKit/gtk/WebCoreSupport/GeolocationClientGtk.cpp:2
> > + * Copyright (C) 2011 Zan Dobersek <zandobersek at gmail.com>
> 
> You need to preserve the copyright from Source/WebCore/platform/gtk/GeolocationServiceGtk.cpp.
> 

You're right, that'd be a bit unfair on my side.

> > Source/WebKit/gtk/WebCoreSupport/GeolocationClientGtk.cpp:97
> > +    }
> > +
> > +    GeoclueAccuracyLevel accuracyLevel = m_enableHighAccuracy ? GEOCLUE_ACCURACY_LEVEL_DETAILED : GEOCLUE_ACCURACY_LEVEL_LOCALITY;
> > +    gboolean result = geoclue_master_client_set_requirements(client.get(), accuracyLevel, 0,
> > +                                                             false, GEOCLUE_RESOURCE_ALL, &error.outPtr());
> > +
> > +    if (!result) {
> > +        errorOccured(error->message);
> > +        return;
> > +    }
> > +
> > +    m_geocluePosition = adoptGRef(geoclue_master_client_create_position(client.get(), &error.outPtr()));
> > +    if (!m_geocluePosition) {
> > +        errorOccured(error->message);
> > +        return;
> > +    }
> > +
> 
> There's lots of extra newlines in the method. Do you think you can clump related ideas together a bit more?
> 

I can avoid assigning the result variable and check the return of geoclue_master_client_set_requiremenets directly. That saves two lines. Other than that, I don't think it's a good idea to try to push through when an error occurs. 

> > Source/WebKit/gtk/WebCoreSupport/GeolocationClientGtk.cpp:125
> > +    if (m_isUpdating) {
> 
> Please use an early return here.
> 

Noted.

> > Tools/ChangeLog:13
> > +        (createWebView): Create web view after declaring DumpRenderTree mode.
> 
> Do you mind explaining why this is important here?
> 

Noted.

> > configure.ac:627
> > +                             [enable support for client-based geolocation [default=yes]]),
> > +              [],[enable_client_based_geolocation="yes"])
> 
> By enabling this by deafult don't you disable the normal Geolocation service for all builds? Wouldn't it be sufficient to enable it only via build-webkit?

Yes, enabling this by default disables the use of service-based geolocation. If there's no motivation to have client-based solution enabled in release versions, I can revert this change.

One more thing on the two different approaches of providing geolocation and their subsequent status after this bug is resolved. Both methods are (still) supported in WebCore, so I see no reason why a configuration option should not be available to decide on the approach to be used. But I'd like to see the client-based geolocation being set as default both in configure.ac and build-webkit - there are more tests that cover it and if it comes to enabling the feature in build-webkit but not in configure.ac, this would meant we're testing client-based geolocation but shipping service-based one. Not a good idea.

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.