[webkit-reviews] review denied: [Bug 42027] [Qt] Request for permission before starting Geolocation service : [Attachment 62214] 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 62214: patch
https://bugs.webkit.org/attachment.cgi?id=62214&action=review

------- Additional Comments from Steve Block <steveblock at google.com>
WebCore/page/Geolocation.cpp:408
 +  #if !ENABLE(CLIENT_BASED_GEOLOCATION)
Why reverse the order of the '#if'? It minimises the diff to keep it as it is.

WebCore/page/Geolocation.cpp:411
 +		    m_oneShots.add(m_startRequestPermissionNotifier);
This is not correct. The notifier pointed to by
m_startRequestPermissionNotifier will already have been added to either
m_oneShots m_watchers in getCurrentPosition() or watchPosition(). So we don't
need to add it again and we can't assume it's a one-shot. I notice that
existing permission denied case, below, suffers from the same problem.

Also, I see a couple of other problems with the existing pre-emptive
permissions logic. I've filed Bug 42811 to track all of these, which we should
fix first. Patches welcome!