[webkit-reviews] review denied: [Bug 42027] [Qt] Request for permission before starting Geolocation service : [Attachment 64673] patch

[bugzilla-daemon at webkit.org]

Steve Block <steveblock at google.com> has denied Mahesh Kulkarni
<mahesh.kulkarni at nokia.com>'s request for review:
Bug 42027: [Qt] Request for permission before starting Geolocation service
https://bugs.webkit.org/show_bug.cgi?id=42027

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

------- Additional Comments from Steve Block <steveblock at google.com>
WebCore/ChangeLog:13
 +	    No new tests added because GeolocationServiceMock does not enable
PREEMPT_GEOLOCATION_PERMISSION
I'm not sure what you mean by this. The choice of the preemptive vs
non-preemptive permissions policy concerns the Geolocation object, not its
client/service. So whether a given platform uses the preemptive policy depends
on the build settings, not on whether it's using the mock in DRT. The code that
you're adding is tested by the existing fast/dom/delayed-permission-* tests, so
we should say so here.

It's a shame that these tests are currently skipped on Qt due to Bug 41364.
Have you tested that these tests pass on Qt with your proposed fix for that
bug? 

WebCore/page/Geolocation.cpp:702
 +		if (m_service &&
m_service->startUpdating(notifier->m_options.get()))
Why do you need to check m_service here? We don't check it when making calls to
the service elsewhere.

Also, you could simply Geolocation::startUpdating() here for both the
client-based and non-client-based impls. That would match what we do in
Geolocation::startRequest() and save having to use an ifdef here.