[webkit-reviews] review requested: [Bug 35210] [Gtk] implements ChromeClient::requestGeolocationPermissionForFrame : [Attachment 49423] patch v2.4

[bugzilla-daemon at webkit.org]

arno. <arno at renevier.net> has asked  for review:
Bug 35210: [Gtk] implements ChromeClient::requestGeolocationPermissionForFrame
https://bugs.webkit.org/show_bug.cgi?id=35210

Attachment 49423: patch v2.4
https://bugs.webkit.org/attachment.cgi?id=49423&action=review

------- Additional Comments from arno. <arno at renevier.net>
(In reply to comment #13)
> (In reply to comment #11)
> > > So you just need to remove the relevant tests from there =).
> > 
> > Unfortunately, some tests still fail. I suppose (unsure) that's because our

> > implementation denies by default. A few tests pass. I don't known if this
is by
> > only chance. I uncommented the passing tests in updated patch.
> 
> I think it's because our DumpRenderTree needs to be brought up to date with
the
> GeoLocation stuff. If you look at these files, and search for Geo, you'll see

> what needs to be done:
> 
> WebKitTools/DumpRenderTree/gtk/LayoutTestControllerGtk.cpp
> WebKitTools/DumpRenderTree/LayoutTestController.cpp
> 
> I think we should try to get those implemented for this patch, so we can test

> the permission giving, and denying.

Actually, the methods to implement in LayoutTestControllerGtk.cpp are
setMockGeolocationPosition and setMockGeolocationError. But webkit gtk does not
have an api that allow embedder to set location an error messages. I suppose we
first need to implement client-based Geolocation support, so we'll be able to
compile with ENABLE_CLIENT_BASED_GEOLOCATION, and the application will have the
ability to set position and errors. But currently, I think we can't implement
those methods. In my patch, I've still implemented the part that allows or
denies request according to the value
gLayoutTestController->geolocationPermission has been set.

> One more comment about the API. I agree to have the 'cancelled' signal, but
can
> we name it geolocation-policy-decision-cancelled, so it's consistent with the

> other signal?

ok, fixed in updated patch.

My updated patch also include webkit/webkitgeolocationpolicydecision.h from
webkit.h (it was not done in previous patch)