Bug 35031 - [chromium] Implement cancelGeolocationPermissionRequestForFrame
Summary: [chromium] Implement cancelGeolocationPermissionRequestForFrame
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit Misc. (show other bugs)
Version: 528+ (Nightly build)
Hardware: PC All
: P2 Normal
Assignee: Marcus Bulach
URL:
Keywords:
: 36833 (view as bug list)
Depends on: 34962
Blocks: 32065
  Show dependency treegraph
 
Reported: 2010-02-17 04:31 PST by Jonathan Dixon
Modified: 2010-03-31 12:32 PDT (History)
7 users (show)

See Also:


Attachments
Patch (4.27 KB, patch)
2010-03-31 03:34 PDT, Marcus Bulach
jorlow: review-
jorlow: commit-queue-
Details | Formatted Diff | Diff
Patch (16.74 KB, patch)
2010-03-31 07:36 PDT, Marcus Bulach
fishd: review-
Details | Formatted Diff | Diff
Patch (17.11 KB, patch)
2010-03-31 10:16 PDT, Marcus Bulach
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Jonathan Dixon 2010-02-17 04:31:49 PST
See https://bugs.webkit.org/show_bug.cgi?id=34962 https://bugs.webkit.org/attachment.cgi?id=48890&action=diff#WebKit/chromium/src/ChromeClientImpl.h_sec1

A new method cancelGeolocationPermissionRequestForFrame  was added, which will need implementing fully instead of the placeholder in WebKit/chromium/src/ChromeClientImpl.h (i.e. call out to IPC to get the browser process to dismiss the infobar)
Comment 1 Marcus Bulach 2010-03-31 03:34:06 PDT
Created attachment 52147 [details]
Patch
Comment 2 Jeremy Orlow 2010-03-31 03:50:23 PDT
Comment on attachment 52147 [details]
Patch

> Index: WebCore/ChangeLog
> ===================================================================
> --- WebCore/ChangeLog	(revision 56830)
> +++ WebCore/ChangeLog	(working copy)
> @@ -1,3 +1,14 @@
> +2010-03-30  Marcus Bulach  <bulach@chromium.org>
> +
> +        Reviewed by NOBODY (OOPS!).
> +
> +        Implements cancelGeolocationPermissionRequestForFrame.
> +

No extra new line

> +        https://bugs.webkit.org/show_bug.cgi?id=35031

Put more of a description here.  Explain why.

> +        * page/Geolocation.cpp:
> +        (WebCore::Geolocation::disconnectFrame):
> +
>  2010-03-31  John Gregg  <johnnyg@google.com>
>  
>          Reviewed by Darin Fisher.
> Index: WebCore/page/Geolocation.cpp
> ===================================================================
> --- WebCore/page/Geolocation.cpp	(revision 56830)
> +++ WebCore/page/Geolocation.cpp	(working copy)
> @@ -225,6 +225,7 @@ void Geolocation::disconnectFrame()
>      if (m_frame) {
>          if (m_frame->document())
>              m_frame->document()->setUsingGeolocation(false);
> +        // FIXME: pass "this" to cancelGeolocationPermissionRequestForFrame, similar to requestGeolocationPermissionForFrame.

Why not do this now?  You can change the consumers of this API at the WebKit layer.
Comment 3 Marcus Bulach 2010-03-31 07:36:05 PDT
Created attachment 52160 [details]
Patch
Comment 4 Marcus Bulach 2010-03-31 07:48:58 PDT
(In reply to comment #2)
> (From update of attachment 52147 [details])
> > Index: WebCore/ChangeLog
> > ===================================================================
> > --- WebCore/ChangeLog	(revision 56830)
> > +++ WebCore/ChangeLog	(working copy)
> > @@ -1,3 +1,14 @@
> > +2010-03-30  Marcus Bulach  <bulach@chromium.org>
> > +
> > +        Reviewed by NOBODY (OOPS!).
> > +
> > +        Implements cancelGeolocationPermissionRequestForFrame.
> > +
> 
> No extra new line
> 
> > +        https://bugs.webkit.org/show_bug.cgi?id=35031
> 
> Put more of a description here.  Explain why.

Done, let me know if it's clear enough.

> 
> > +        * page/Geolocation.cpp:
> > +        (WebCore::Geolocation::disconnectFrame):
> > +
> >  2010-03-31  John Gregg  <johnnyg@google.com>
> >  
> >          Reviewed by Darin Fisher.
> > Index: WebCore/page/Geolocation.cpp
> > ===================================================================
> > --- WebCore/page/Geolocation.cpp	(revision 56830)
> > +++ WebCore/page/Geolocation.cpp	(working copy)
> > @@ -225,6 +225,7 @@ void Geolocation::disconnectFrame()
> >      if (m_frame) {
> >          if (m_frame->document())
> >              m_frame->document()->setUsingGeolocation(false);
> > +        // FIXME: pass "this" to cancelGeolocationPermissionRequestForFrame, similar to requestGeolocationPermissionForFrame.
> 
> Why not do this now?  You can change the consumers of this API at the WebKit
> layer.

Thanks for pointing this out! Done on all APIs I could find.
Comment 5 WebKit Review Bot 2010-03-31 07:52:26 PDT
Attachment 52160 [details] did not pass style-queue:

Failed to run "WebKitTools/Scripts/check-webkit-style" exit_code: 1
WebCore/ChangeLog:8:  Line contains tab character.  [whitespace/tab] [5]
Total errors found: 1 in 23 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 6 Darin Fisher (:fishd, Google) 2010-03-31 08:02:39 PDT
Comment on attachment 52160 [details]
Patch

> Index: WebKit/chromium/src/ChromeClientImpl.cpp
> ===================================================================
> --- WebKit/chromium/src/ChromeClientImpl.cpp	(revision 56830)
> +++ WebKit/chromium/src/ChromeClientImpl.cpp	(working copy)
> @@ -707,6 +707,12 @@ void ChromeClientImpl::requestGeolocatio
>      m_webView->client()->geolocationService()->requestPermissionForFrame(geolocationService->geolocationServiceBridge()->getBridgeId(), frame->document()->url());
>  }
>  
> +void ChromeClientImpl::cancelGeolocationPermissionRequestForFrame(Frame* frame, Geolocation* geolocation)
> +{
> +    GeolocationServiceChromium* geolocationService = reinterpret_cast<GeolocationServiceChromium*>(geolocation->getGeolocationService());

That reinterpret_cast can be a static_cast, no?  Use static_cast when there exists an "is a" relationship between the types you are casting.  Not doing so can cause memory errors if you ever start using multiple inheritance.
Comment 7 Jeremy Orlow 2010-03-31 08:57:26 PDT
Comment on attachment 52160 [details]
Patch

Besides Darin's comment, LGTM.
Comment 8 Marcus Bulach 2010-03-31 10:16:59 PDT
Created attachment 52182 [details]
Patch
Comment 9 Marcus Bulach 2010-03-31 10:17:32 PDT
(In reply to comment #6)
> (From update of attachment 52160 [details])
> > Index: WebKit/chromium/src/ChromeClientImpl.cpp
> > ===================================================================
> > --- WebKit/chromium/src/ChromeClientImpl.cpp	(revision 56830)
> > +++ WebKit/chromium/src/ChromeClientImpl.cpp	(working copy)
> > @@ -707,6 +707,12 @@ void ChromeClientImpl::requestGeolocatio
> >      m_webView->client()->geolocationService()->requestPermissionForFrame(geolocationService->geolocationServiceBridge()->getBridgeId(), frame->document()->url());
> >  }
> >  
> > +void ChromeClientImpl::cancelGeolocationPermissionRequestForFrame(Frame* frame, Geolocation* geolocation)
> > +{
> > +    GeolocationServiceChromium* geolocationService = reinterpret_cast<GeolocationServiceChromium*>(geolocation->getGeolocationService());
> 
> That reinterpret_cast can be a static_cast, no?  Use static_cast when there
> exists an "is a" relationship between the types you are casting.  Not doing so
> can cause memory errors if you ever start using multiple inheritance.

Oh, very good catch, thanks! Done.
Comment 10 Jeremy Orlow 2010-03-31 10:20:01 PDT
Comment on attachment 52182 [details]
Patch

r=me
Comment 11 WebKit Commit Bot 2010-03-31 10:47:41 PDT
Comment on attachment 52182 [details]
Patch

Clearing flags on attachment: 52182

Committed r56851: <http://trac.webkit.org/changeset/56851>
Comment 12 WebKit Commit Bot 2010-03-31 10:47:47 PDT
All reviewed patches have been landed.  Closing bug.
Comment 13 Marcus Bulach 2010-03-31 12:32:11 PDT
*** Bug 36833 has been marked as a duplicate of this bug. ***