Bug 64970 – [Gtk] Support for client-based geolocation

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKitGTK

[Zan Dobersek]

Currently geolocation support is available through GeolocationService object in WebCore but GeolocationClient should be implemented in WebKit in order to make the use of GeolocationClientMock possible for testing purposes.

[Zan Dobersek]

Created attachment 101628 [details]
Client-based geolocation - the whole package

[Zan Dobersek]

(In reply to comment #1)
> Created an attachment (id=101628) [details]
> Client-based geolocation - the whole package

A bit more description: GeolocationClientGtk utilizes GeoClue in pretty much the same way as GeolocationServiceGtk does. The only significant difference is how position updates and errors are handled and the setting of new requirements when high curracy is requested and client is already updating.

The whole feature is already enabled by default in both build-webkit and configure.ac.

Also included is complete support for testing. Out of 37 tests in fast/dom/Geolocation 36 now pass with one single test remaining skipped because of reported crashing.

Just noted that new files are missing licence headers, will fix.

[Zan Dobersek]

Created attachment 101634 [details]
Same patch but now with license headers in new files

[Gustavo Noronha (kov)]

Comment on attachment 101634 [details]
Same patch but now with license headers in new files

Attachment 101634 [details] did not pass gtk-ews (gtk):
Output: http://queues.webkit.org/results/9192803

[Philippe Normand]

If the feature is enabled in configure and build-webkit why is EWS failing to build the patch? Is a clean build required?

[Zan Dobersek]

(In reply to comment #5)
> If the feature is enabled in configure and build-webkit why is EWS failing to build the patch? Is a clean build required?

Yes, I believe a clean build is required.

[Martin Robinson]

Comment on attachment 101634 [details]
Same patch but now with license headers in new files

View in context: https://bugs.webkit.org/attachment.cgi?id=101634&action=review

This patch looks pretty good in general, and unskips a ton of tests!  I don't quite understand why we want to implement client-based geolocation without exposing an API. Is  it necessary just to unskip the tests? Could we somehow create mock locations by interfacing directly with geoclue? If so that would be great, because we could test the geoclue stuff with layout tests.

> Source/WebKit/gtk/WebCoreSupport/GeolocationClientGtk.cpp:56
> +    if (m_geoclueClient)
> +        g_object_unref(m_geoclueClient);
> +
> +    if (m_geocluePosition)
> +        g_object_unref(m_geocluePosition);

It would be better to use GRefPtr instead of handing reference counts manually.

> Source/WebKit/gtk/WebCoreSupport/GeolocationClientGtk.cpp:69
> +    GeoclueMaster* master = geoclue_master_get_default();

Please use GRefPtr here with adoptGref.

> Source/WebKit/gtk/WebCoreSupport/GeolocationClientGtk.cpp:70
> +    GeoclueMasterClient* client = geoclue_master_create_client(master, 0, 0);

Ditto.

> Source/WebKit/gtk/WebCoreSupport/GeolocationClientGtk.cpp:162
> +                                                GeocluePositionFields fields,
> +                                                int timestamp,
> +                                                double latitude,
> +                                                double longitude,
> +                                                double altitude,
> +                                                GeoclueAccuracy* accuracy,
> +                                                GError* error,
> +                                                GeolocationClient* that)
> +{

We don't typically add newlines to argument lists in function definitions. This function can be static to the file.

> Source/WebKit/gtk/WebCoreSupport/GeolocationClientGtk.cpp:165
> +        g_error_free(error);

Does the callback really need to free the GError?

> Source/WebKit/gtk/WebCoreSupport/GeolocationClientGtk.cpp:186
> +void GeolocationClient::positionChanged(GeocluePosition*, GeocluePositionFields fields, int timestamp, double latitude, double longitude, double altitude, GeoclueAccuracy* accuracy, GeolocationClient* that)
> +{
> +    if (!(fields & GEOCLUE_POSITION_FIELDS_LATITUDE && fields & GEOCLUE_POSITION_FIELDS_LONGITUDE)) {
> +        that->errorOccured("Position could not be determined.");
> +        return;
> +    }
> +
> +    that->m_timestamp = timestamp;
> +    that->m_latitude = latitude;
> +    that->m_longitude = longitude;
> +    that->m_altitude = altitude;
> +
> +    GeoclueAccuracyLevel level;
> +    geoclue_accuracy_get_details(accuracy, &level, &that->m_accuracy, &that->m_altitudeAccuracy);
> +    that->updatePosition();
> +}

Shouldn't this be an instance method?

> Tools/DumpRenderTree/gtk/LayoutTestControllerGtk.cpp:495
> +    gchar* cMessage = JSStringCopyUTF8CString(message);
> +    DumpRenderTreeSupportGtk::setMockGeolocationError(view, code, cMessage);
> +    g_free(cMessage);
>  }

Could you use GOwnPtr here?

[Zan Dobersek]

Comment on attachment 101634 [details]
Same patch but now with license headers in new files

View in context: https://bugs.webkit.org/attachment.cgi?id=101634&action=review

>> Source/WebKit/gtk/WebCoreSupport/GeolocationClientGtk.cpp:56
>> +        g_object_unref(m_geocluePosition);
> 
> It would be better to use GRefPtr instead of handing reference counts manually.

True, that'd look much nicer.

>> Source/WebKit/gtk/WebCoreSupport/GeolocationClientGtk.cpp:70
>> +    GeoclueMasterClient* client = geoclue_master_create_client(master, 0, 0);
> 
> Ditto.

Will be used for both GeoclueMaster and GeoclueMasterClient.

>> Source/WebKit/gtk/WebCoreSupport/GeolocationClientGtk.cpp:162
>> +{
> 
> We don't typically add newlines to argument lists in function definitions. This function can be static to the file.

Noted.

>> Source/WebKit/gtk/WebCoreSupport/GeolocationClientGtk.cpp:165
>> +        g_error_free(error);
> 
> Does the callback really need to free the GError?

The error originates from the provider GeoClue is currently using, and looking at GeoClue code, the error is not freed anywhere. Because of that I think it would be appropriate that we free it.

>> Source/WebKit/gtk/WebCoreSupport/GeolocationClientGtk.cpp:186
>> +}
> 
> Shouldn't this be an instance method?

And instance method will be made. Also, because of that, a new callback will be added, positionChangedCallback, for the 'position-changed' signal. The callback will then call the GeolocationClient's positionChanged method in the same way the getPositionCallback callback does.

>> Tools/DumpRenderTree/gtk/LayoutTestControllerGtk.cpp:495
>>  }
> 
> Could you use GOwnPtr here?

Will be used.

[Zan Dobersek]

(In reply to comment #7)
> (From update of attachment 101634 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=101634&action=review
> 
> This patch looks pretty good in general, and unskips a ton of tests!  I don't quite understand why we want to implement client-based geolocation without exposing an API. Is  it necessary just to unskip the tests?

Using client-based geolocation eases the use of the mock geolocation client which is needed for _many_ tests and I've started this task pretty much because of that. But if there's an interest for an API I guess it should be made (though, to be honest, I don't have much idea on what it would look like).

> Could we somehow create mock locations by interfacing directly with geoclue? If so that would be great, because we could test the geoclue stuff with layout tests.

Great you've brought that up. It bugs me that the mock clients are used to only test behavior of WebCore-related functionality of a feature but not the functionality of an external library that we use (in this case GeoClue). But we could test GeoClue behavior by creating a fake provider for testing purposes that would then be used on buildbots. This would then remove the necessity of using GeolocationClientMock. I'll look how the provider could be updated from DumpRenderTreeSupportGtk class and if this would be functional at all.

But before that I'll upload the updated patch. And thanks for the review!

[Zan Dobersek]

Created attachment 103182 [details]
Patch

[Martin Robinson]

Comment on attachment 103182 [details]
Patch

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.

> 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?

> Source/WebKit/gtk/WebCoreSupport/GeolocationClientGtk.cpp:2
> + * Copyright (C) 2011 Zan Dobersek <[e-mail]>

You need to preserve the copyright from Source/WebCore/platform/gtk/GeolocationServiceGtk.cpp.

> Source/WebKit/gtk/WebCoreSupport/GeolocationClientGtk.cpp:74
> +    GOwnPtr<GError> error;

Please move this down to where you first use it.

> 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?

> Source/WebKit/gtk/WebCoreSupport/GeolocationClientGtk.cpp:125
> +    if (m_isUpdating) {

Please use an early return here.

> Tools/ChangeLog:13
> +        (createWebView): Create web view after declaring DumpRenderTree mode.

Do you mind explaining why this is important here?

> 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?

[Zan Dobersek]

(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 <[e-mail]>
> 
> 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.

[Martin Robinson]

Comment on attachment 103182 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=103182&action=review

>>> Source/WebKit/gtk/WebCoreSupport/GeolocationClientGtk.cpp:97
>>> +
>> 
>> 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.

Sorry. Here I meant there were a lot of empty lines. I think they could be reduced by grouping related concepts together. For instance, line 86 and 77, 99, 101 and 104. This is just a nit, but it's hard to keep track of methods that take up too much vertical space.

>>> configure.ac:627
>>> +              [],[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.

It sounds like this API is a solid replacement for old one. It also seems like the difference between the two is totally hidden from the user of the library -- it's an implementation detail. If that's the case perhaps we shouldn't even support the old method.

I also dislike the idea of having the same code (effectively) copied in two places. If you still think that keeping the old code around is a good idea, we should find some way to share the geoclue glue between the two implementations. From what you say and the amount of tests you are unskipping with this patch it seems like client-based geolocation is the way to go. Let me know if I'm misunderstanding things here.

[Zan Dobersek]

(In reply to comment #13)
> (From update of attachment 103182 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=103182&action=review
> 
> >>> Source/WebKit/gtk/WebCoreSupport/GeolocationClientGtk.cpp:97
> >>> +
> >> 
> >> 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.
> 
> Sorry. Here I meant there were a lot of empty lines. I think they could be reduced by grouping related concepts together. For instance, line 86 and 77, 99, 101 and 104. This is just a nit, but it's hard to keep track of methods that take up too much vertical space.
> 

Ah, no problem, I'll remove the extra empty lines. But I'll hold onto not assigning a 'result' variable since it would only be used in one place.

> >>> configure.ac:627
> >>> +              [],[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.
> 
> It sounds like this API is a solid replacement for old one. It also seems like the difference between the two is totally hidden from the user of the library -- it's an implementation detail. If that's the case perhaps we shouldn't even support the old method.
> 
> I also dislike the idea of having the same code (effectively) copied in two places. If you still think that keeping the old code around is a good idea, we should find some way to share the geoclue glue between the two implementations. From what you say and the amount of tests you are unskipping with this patch it seems like client-based geolocation is the way to go. Let me know if I'm misunderstanding things here.

Ok, I'm convinced. Doubled code really doesn't sound nice, and, in my opinion, neither does an extra class that shares geoclue code between the two implementations. With client-based geolocation probably being something that eventually all of WebKit ports will move towards, service-based solution wouldn't be needed, so you're right about removing it completely.

With client-based geolocation being default and no configuration.ac flags, I'll still turn on the flag in build-webkit, if that's ok, just to note what the Gtk port is really using, even though it doesn't have effect on compilation.

I'll upload an updated patch tomorrow with all the issues addressed.

[Zan Dobersek]

Created attachment 110268 [details]
Updated patch

[Martin Robinson]

Comment on attachment 110268 [details]
Updated patch

View in context: https://bugs.webkit.org/attachment.cgi?id=110268&action=review

This looks great! I realized that since we need this code for WebKit2 also we'll have to share it anyway. We can tackle that later though. It only requires hoisting the code out into another class. Please fix the style nits before landing.

> Source/WebKit/gtk/WebCoreSupport/DumpRenderTreeSupportGtk.h:126
> +    static void setMockGeolocationPermission(WebKitWebView*, bool);
> +    static void setMockGeolocationPosition(WebKitWebView*, double, double, double);
> +    static void setMockGeolocationError(WebKitWebView*, int, const gchar*);

You should include the parameter names for types that are generic like int, double, and char*.

> Source/WebKit/gtk/WebCoreSupport/GeolocationClientGtk.cpp:33
> +void getPositionCallback(GeocluePosition *position, GeocluePositionFields fields, int timestamp, double latitude, double longitude, double altitude, GeoclueAccuracy* accuracy, GError* error, GeolocationClient* client)

The asterisk in GeocluePosition *position is in the wrong place.

> Source/WebKit/gtk/WebCoreSupport/GeolocationClientGtk.cpp:56
> +    , m_latitude(0.0)
> +    , m_longitude(0.0)
> +    , m_altitude(0.0)
> +    , m_accuracy(0.0)
> +    , m_altitudeAccuracy(0.0)

Here you should use 0 instead of 0.0.

> Source/WebKit/gtk/WebCoreSupport/GeolocationClientGtk.cpp:75
> +        errorOccured("Could not connect to location provider.");

I think this string should be prefixed by an underscore so that it can be localized.

> Source/WebKit/gtk/WebCoreSupport/GeolocationClientGtk.cpp:82
> +                                                 false, GEOCLUE_RESOURCE_ALL, &error.outPtr())) {

The indentation looks slightly off here.

> Source/WebKit/gtk/WebCoreSupport/GeolocationClientGtk.cpp:94
> +    geoclue_position_get_position_async(m_geocluePosition.get(), (GeocluePositionCallback)getPositionCallback, this);

Here you should use reinterpret_cast.

> Source/WebKit/gtk/WebCoreSupport/GeolocationClientGtk.cpp:165
> +                                                          true, m_altitude, true, m_altitudeAccuracy, false, 0.0, false, 0.0);

The 0.0 here should just be 0.

> Source/WebKit/gtk/WebCoreSupport/GeolocationClientGtk.h:27
> +

No newline necessary here.

> Tools/DumpRenderTree/gtk/DumpRenderTree.cpp:1103
>      DumpRenderTreeSupportGtk::setDumpRenderTreeModeEnabled(true);
>  
> +    WebKitWebView* view = WEBKIT_WEB_VIEW(self_scrolling_webkit_web_view_new());

I think it's also important to put a little comment here like "It's important to create the WebvView after entering DumpRenderTree mode."

> Tools/Scripts/build-webkit:159
> +      define => "ENABLE_CLIENT_BASED_GEOLOCATION", default => (isAppleWebKit() || isGtk()), value => \$clientBasedGeolocationSupport },

Hopefully this doesn't lead to a warning about an unknown parameter. If it does, it could mean our bots always build with warnings.

[Zan Dobersek]

(In reply to comment #16)
> (From update of attachment 110268 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=110268&action=review
> 
> This looks great! I realized that since we need this code for WebKit2 also we'll have to share it anyway. We can tackle that later though. It only requires hoisting the code out into another class. Please fix the style nits before landing.
> 

All the nits are fixed, but I don't have commit access, so either someone else can push the patch or the commit queue is used. By the way, a complete rebuild is required.

> > Tools/Scripts/build-webkit:159
> > +      define => "ENABLE_CLIENT_BASED_GEOLOCATION", default => (isAppleWebKit() || isGtk()), value => \$clientBasedGeolocationSupport },
> 
> Hopefully this doesn't lead to a warning about an unknown parameter. If it does, it could mean our bots always build with warnings.

If you're having configure warnings in mind, by removing --enable-client-based-geolocation option the warnings will pop up anyway. As said before, it's just there to note that Gtk port uses this feature. Similarly enabled in build-webkit but not configurable in configure.ac is the ENABLE_INSPECTOR option, which then also causes unrecognized configure option warnings.

Thanks for the reviews!

[Martin Robinson]

(In reply to comment #17)

> All the nits are fixed, but I don't have commit access, so either someone else can push the patch or the commit queue is used. By the way, a complete rebuild is required.

Do you mind uploading a new version of the patch? We can use the commit queue.

[Zan Dobersek]

Created attachment 111252 [details]
Updated patch

[WebKit Review Bot]

Attachment 111252 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/update-webkit', '--chromium']" exit_code: 2

Last 3072 characters of output:
cessed: True
    deps_parsed: True
    requirements: ('./', 'chromium_deps', 'third_party')
  
    name: third_party/yasm/source/patched-yasm
    url: From('chromium_deps', 'src/third_party/yasm/source/patched-yasm')
    should_process: True
    requirements: ('./', 'chromium_deps', 'third_party')
  
    name: tools/clang
    url: http://src.chromium.org/svn/trunk/src/tools/clang@105777
    parsed_url: http://src.chromium.org/svn/trunk/src/tools/clang@105777
    should_process: True
    processed: True
    deps_parsed: True
    requirements: ('./',)
  
    name: tools/generate_stubs
    url: http://src.chromium.org/svn/trunk/src/tools/generate_stubs@105777
    parsed_url: http://src.chromium.org/svn/trunk/src/tools/generate_stubs@105777
    should_process: True
    processed: True
    deps_parsed: True
    requirements: ('./',)
  
    name: tools/grit
    url: From('chromium_deps', 'src/tools/grit')
    parsed_url: http://grit-i18n.googlecode.com/svn/trunk@6
    should_process: True
    processed: True
    deps_parsed: True
    requirements: ('./', 'chromium_deps')
  
    name: tools/gritsettings
    url: http://src.chromium.org/svn/trunk/src/tools/gritsettings@105777
    parsed_url: http://src.chromium.org/svn/trunk/src/tools/gritsettings@105777
    should_process: True
    processed: True
    deps_parsed: True
    requirements: ('./',)
  
    name: tools/gyp
    url: From('chromium_deps', 'src/tools/gyp')
    parsed_url: http://gyp.googlecode.com/svn/trunk@1074
    should_process: True
    processed: True
    deps_parsed: True
    requirements: ('./', 'chromium_deps')
      name: scons
      url: http://src.chromium.org/svn/trunk/src/third_party/scons@44099
      requirements: ('tools/gyp',)
  
    name: tools/win
    url: http://src.chromium.org/svn/trunk/src/tools/win@105777
    parsed_url: http://src.chromium.org/svn/trunk/src/tools/win@105777
    should_process: True
    processed: True
    deps_parsed: True
    requirements: ('./',)
  
    name: tools/xdisplaycheck
    url: http://src.chromium.org/svn/trunk/src/tools/xdisplaycheck@105777
    parsed_url: http://src.chromium.org/svn/trunk/src/tools/xdisplaycheck@105777
    should_process: True
    processed: True
    deps_parsed: True
    requirements: ('./',)
  
    name: ui
    url: http://src.chromium.org/svn/trunk/src/ui@105777
    parsed_url: http://src.chromium.org/svn/trunk/src/ui@105777
    should_process: True
    processed: True
    deps_parsed: True
    requirements: ('./',)
  
    name: v8
    url: From('chromium_deps', 'src/v8')
    should_process: True
    requirements: ('./', 'chromium_deps')
  
    name: webkit
    url: http://src.chromium.org/svn/trunk/src/webkit@105777
    parsed_url: http://src.chromium.org/svn/trunk/src/webkit@105777
    should_process: True
    processed: True
    deps_parsed: True
    requirements: ('./',)

Error: 'depot_tools/gclient sync' failed 3 tries and returned 256 at Tools/Scripts/update-webkit-chromium line 107.
Re-trying 'depot_tools/gclient sync'
No such file or directory at Tools/Scripts/update-webkit line 104.


If any of these errors are false positives, please file a bug against check-webkit-style.

[Martin Robinson]

Comment on attachment 111252 [details]
Updated patch

View in context: https://bugs.webkit.org/attachment.cgi?id=111252&action=review

> Source/WebKit/gtk/WebCoreSupport/DumpRenderTreeSupportGtk.cpp:857
> +    double timestamp = g_get_real_time() / 1000000.0;

Whoops small style nit here. Should be 1000000. I'll fix this and land it.

[Martin Robinson]

Comment on attachment 111252 [details]
Updated patch

View in context: https://bugs.webkit.org/attachment.cgi?id=111252&action=review

>> Source/WebKit/gtk/WebCoreSupport/DumpRenderTreeSupportGtk.cpp:857
>> +    double timestamp = g_get_real_time() / 1000000.0;
> 
> Whoops small style nit here. Should be 1000000. I'll fix this and land it.

Actually, this is needed to force floating point division. I'll cq+ this.

[Gustavo Noronha (kov)]

Comment on attachment 111252 [details]
Updated patch

Attachment 111252 [details] did not pass gtk-ews (gtk):
Output: http://queues.webkit.org/results/10096188

[Zan Dobersek]

(In reply to comment #23)
> (From update of attachment 111252 [details])
> Attachment 111252 [details] did not pass gtk-ews (gtk):
> Output: http://queues.webkit.org/results/10096188

This error is expected as the tree would need a complete rebuild when the patch would land.

[Philippe Normand]

(In reply to comment #24)
> (In reply to comment #23)
> > (From update of attachment 111252 [details] [details])
> > Attachment 111252 [details] [details] did not pass gtk-ews (gtk):
> > Output: http://queues.webkit.org/results/10096188
> 
> This error is expected as the tree would need a complete rebuild when the patch would land.

Ok let's handle it on IRC this afternoon with Gustavo, I can trigger clean builds on the GTK Debug bots.

No need for cq, we should land it manually I think.

[Philippe Normand]

Comment on attachment 111252 [details]
Updated patch

Clearing flags on attachment: 111252

Committed r97746: <http://trac.webkit.org/changeset/97746>

[Philippe Normand]

All reviewed patches have been landed.  Closing bug.

[Xan Lopez]

The build has been hardcoded to depend on geolocation, but geolocation is not enabled by default in configure.ac. Things now fail with:

In file included from ../../Source/WebKit/gtk/WebCoreSupport/GeolocationClientGtk.cpp:22:0:
../../Source/WebKit/gtk/WebCoreSupport/GeolocationClientGtk.h:27:36: fatal error: geoclue/geoclue-master.h: No such file or directory
compilation terminated.
make[1]: *** [Source/WebKit/gtk/WebCoreSupport/libwebkitgtk_3_0_la-GeolocationClientGtk.lo] Error 1
make[1]: *** Waiting for unfinished jobs....
make[1]: Leaving directory `/home/xan/git/webkit/build/normal'
make: *** [install] Error 2

I believe things are missing ENABLE(GEOLOCATION) all over the place? Probably this works in the bots because build-webkit enables geolocation.

[Zan Dobersek]

(In reply to comment #28)
> The build has been hardcoded to depend on geolocation, but geolocation is not enabled by default in configure.ac. Things now fail with:
> 
> In file included from ../../Source/WebKit/gtk/WebCoreSupport/GeolocationClientGtk.cpp:22:0:
> ../../Source/WebKit/gtk/WebCoreSupport/GeolocationClientGtk.h:27:36: fatal error: geoclue/geoclue-master.h: No such file or directory
> compilation terminated.
> make[1]: *** [Source/WebKit/gtk/WebCoreSupport/libwebkitgtk_3_0_la-GeolocationClientGtk.lo] Error 1
> make[1]: *** Waiting for unfinished jobs....
> make[1]: Leaving directory `/home/xan/git/webkit/build/normal'
> make: *** [install] Error 2
> 
> I believe things are missing ENABLE(GEOLOCATION) all over the place? Probably this works in the bots because build-webkit enables geolocation.

Sorry for the stupid mistake. You're right about the ENABLE flag, but rather than using ENABLE(GEOLOCATION) I'll post a patch that wraps things into ENABLE(CLIENT_BASED_GEOLOCATION). ENABLE_CLIENT_BASED_GEOLOCATION is defined only if geolocation is enabled, so that should be enough.

Patch coming soon.

[Martin Robinson]

*** Bug 36053 has been marked as a duplicate of this bug. ***

[Martin Robinson]

*** Bug 54197 has been marked as a duplicate of this bug. ***