Bug 35031 – [chromium] Implement cancelGeolocationPermissionRequestForFrame

• Component: WebKit Misc.
• Status: RESOLVED FIXED

[Jonathan Dixon]

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)

[Marcus Bulach]

Created attachment 52147 [details]
Patch

[Jeremy Orlow]

Comment on attachment 52147 [details]
Patch

> Index: WebCore/ChangeLog
> ===================================================================
> --- WebCore/ChangeLog	(revision 56830)
> +++ WebCore/ChangeLog	(working copy)
> @@ -1,3 +1,14 @@
> [phone]  Marcus Bulach  <[e-mail]>
> +
> +        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  <[e-mail]>
>  
>          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.

[Marcus Bulach]

Created attachment 52160 [details]
Patch

[Marcus Bulach]

(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 @@
> > [phone]  Marcus Bulach  <[e-mail]>
> > +
> > +        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  <[e-mail]>
> >  
> >          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.

[WebKit Review Bot]

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.

[Darin Fisher (:fishd, Google)]

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.

[Jeremy Orlow]

Comment on attachment 52160 [details]
Patch

Besides Darin's comment, LGTM.

[Marcus Bulach]

Created attachment 52182 [details]
Patch

[Marcus Bulach]

(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.

[Jeremy Orlow]

Comment on attachment 52182 [details]
Patch

r=me

[WebKit Commit Bot]

Comment on attachment 52182 [details]
Patch

Clearing flags on attachment: 52182

Committed r56851: <http://trac.webkit.org/changeset/56851>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Marcus Bulach]

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