Bug 35031 - [chromium] Implement cancelGeolocationPermissionRequestForFrame
: [chromium] Implement cancelGeolocationPermissionRequestForFrame
Status: RESOLVED FIXED
: WebKit
WebKit Misc.
: 528+ (Nightly build)
: PC All
: P2 Normal
Assigned To:
:
:
: 34962
: 32065
  Show dependency treegraph
 
Reported: 2010-02-17 04:31 PST by
Modified: 2010-03-31 12:32 PST (History)


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


Note

You need to log in before you can comment on or make changes to this bug.


Description From 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 From 2010-03-31 03:34:06 PST -------
Created an attachment (id=52147) [details]
Patch
------- Comment #2 From 2010-03-31 03:50:23 PST -------
(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.

> +        * 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 From 2010-03-31 07:36:05 PST -------
Created an attachment (id=52160) [details]
Patch
------- Comment #4 From 2010-03-31 07:48:58 PST -------
(In reply to comment #2)
> (From update of attachment 52147 [details] [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 From 2010-03-31 07:52:26 PST -------
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 From 2010-03-31 08:02:39 PST -------
(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.
------- Comment #7 From 2010-03-31 08:57:26 PST -------
(From update of attachment 52160 [details])
Besides Darin's comment, LGTM.
------- Comment #8 From 2010-03-31 10:16:59 PST -------
Created an attachment (id=52182) [details]
Patch
------- Comment #9 From 2010-03-31 10:17:32 PST -------
(In reply to comment #6)
> (From update of attachment 52160 [details] [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 From 2010-03-31 10:20:01 PST -------
(From update of attachment 52182 [details])
r=me
------- Comment #11 From 2010-03-31 10:47:41 PST -------
(From update of attachment 52182 [details])
Clearing flags on attachment: 52182

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