[webkit-gtk] WK2: Adapting the Policy Decision Framework for Geolocation

[Mario Sanchez Prada]

On Mon, 2012-04-02 at 15:42 -0700, Martin Robinson wrote:
> [...]
> > It would be great to hear more opinions then.
> 
> I like the approach in option 1,

Just a nitpick: what you're describing below in this mail is basically
Option 2, as you're talking about defining a PolicyDecisionListener
abstract class to relay on from WebKitPolicyDecision.

Option 1 is what I'm proposing, without much success so far :-)

> but I think that we can scrap the
> GObject interface with an abstract C++ class in the private data
> portion of the WebKitPolicyDecision object. Since this issue is only
> an implementation detail, I think we should avoid completely exposing
> it in the API. The creation of the WebKitFramePolicyDecision class is
> an artificial result of this implementation detail. I'm not sure if
> makes sense either, since geolocation requests are also associated
> with frames. Sometimes naming problems point to real design problems.

I agree current naming is perhaps not the best one.

> Here's a more concrete explanation of what I'm suggesting with the
> caveat that the names I'm choosing are terrible. :)
> 
> class PolicyDecisionDecisionListener {

You mean PolicyDecisionListener, I guess.

> public:
>     void accept() = 0;
>     void reject() = 0;
>     void download() = 0;
> }
>
> class CAPIPolicyDecisionListener {

I would still name this FramePolicyDecisionListener, or perhaps
PolicyClientPolicyDecisionListener. If it's going to be private, I don't
see much trouble on that.

> private:
>    WKRetainPtr<WKFramePolicyListenerRef> m_listener;
> 
> public:
>     void accept() {
>         WKFramePolicyListenerUse(m_listener.get());
>     }
>   ...etc...
> }
> 
> class GeolocationPolicyDecisionListener {
> private:
>     WKRetainPtr<WKGeolocationPermissionRequestRef> m_request;
> public:
>     void accept() {
>        WKGeolocationPermissionRequestAllow(m_request.get());
>     }
>   ...etc...
> }
> 
> In WebKitPolicyDecision.cpp:
> struct _WebKitPolicyDecisionPrivate {
>     OwnPtr<PolicyDecisionDecisionListener> listener;
>     bool madePolicyDecision;
> };
> 
> Here are the benefits I see to this approach:
> 1. It's less code.

I agree

> 2. The API does not bleed details of the implementation.

I don't agree. You're exposing a download() method for any
implementation of PolicyDecision, even if it does not make sense for 2
out of 3 specific decision types ('navigation' and 'geolocation'), just
because it's one of the methods present in the internal C API for the
Policy Client (where it actually comes from). Hence this design is
actually bleeding details of the internal implementation, which is my
biggest concern here.

My proposal in Option 1, however, is precisely about avoiding that
problem, because you would just expose accept() and reject() methods in
WebKitPolicyDecision, regardless of the specific implementation. 

Hence, in this specific regard I think my proposal is better.

Perhaps I'm missing something, though, so if I'm the only one thinking
this way I'd be fine going ahead with your proposal and forgetting about
that weird download() method in WebKitPolicyDecision.

Mario