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

[Mario Sanchez Prada]

On Mon, 2012-04-02 at 10:39 +0200, Carlos Garcia Campos wrote:
> [...]El lun, 02-04-2012 a las 09:11 +0200, Carlos Garcia Campos escribió:
> > I simpler approach would be:
> > 
> >  - Rename use()/ignore() to accept()/reject() or allow()/deny().
> >  - Add WKGeolocationPermissionRequestRef attribute to
> > WebKitPolicyDecision and
> > webkitPolicyDecisionSetGeolocationPermissionRequest().
> >  - Add WebKitGeolocationPolicyDecision that calls
> > webkitPolicyDecisionSetGeolocationPermissionRequest() when created.
> >  - That way WebKitPolicyDecision will always have either a listener or a
> > geolocation request, and it will use whatever it has, doing nothing in
> > download() if listener is NULL.
> >  - Document that download() doesn't make sense for geolocation
> > decisions, if it's not obvious enough.
> 
> Btw, I'm proposing this a simpler solution, because the download problem
> is present in the current API too. But conceptually, I agree with the
> option1, with a minor change. Since geolocation requests are very
> different than frame policy decisions, maybe it would be better to use
> an interface for WebKitPolicyDecision instead of an abstract class. That
> way concrete objects will implement the interface instead of inheriting
> from a common parent. The idea and the result is indeed the same. 

I'm fine with that idea too. After all the WebKitPolicyDecision abstract
class I have in my current patch (see my last mail here [1]) has no
state and all its methods are pure virtual, so it's already a de-facto
interface.

I agree in the conceptual thing too, though. I'm attaching a new UML
diagram reflecting your proposal to make sure we all are aware what
we're talking about.

> WebKitPolicyDecision
> 
> webkit_policy_decision_accept();
> webkit_policy_decision_reject();

As I said, this is exactly what I have now, but being an abstract class
(see [1]), so I agree.

> WebKitFramePolicyDecision (I can't think of a better name either)
                            ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
Ha! You ended up being as much original as me it seems :)

> Implements accept() and reject() and adds download()

Exactly.

> WebKitResponsePolicyDecision and WebKitNavigationPolicyDecision would
> simply inherit from WebKitFramePolicyDecision.
> 
> WebKitGeolocationPolicyDecision implements accept() and reject()

Yep.

> Both solutions look good to me, the first one is simpler (the code and
> the API exposed), 

I was discussing on Friday with Martin via IRC, and it seems he also
prefers an approach similar to that one, that is "Option 2", where the
hierarchy is not changed as much as in "Option 1".

Main difference between your initial proposal (your previous mail) and
"Option 2" is that you propose to deal with instances of
WKFrameListenerRef and WKGeolocationPermissionRequestRef in a simpler
way (just by handling them internally in current implementation of
WebKitPolicyDecision) and in Option 2 I was proposing to do it through a
typical Strategy pattern, where I would define a PolicyListener
interface/abstract class with use(), ignore() and download() methods to
be implemented by a FramePolicyListener and GeolocationPolicyListener. 

Anyway, as those new "listener" classes would be private I don't think
it would make much a difference in practical terms and perhaps it would
be better and less error prone to go ahead with your proposal of just
doing everything in WebKitPolicyDecision, if we finally decide to do it
that way.

> but if you think the download() is an important
> problem, this second one would be fine with me too.

I think it is, because your sending the apps using the API an strange
message: that there are these "Policy decision" generic objects that
have this download() operation always available, even if it only makes
sense for one out of three specific kind of decisions.

Also, it seems to me that having this download() operation available in
PolicyDecision objects comes from the fact of, so far, that API mimics
what we have in WebKit's Policy Client API, which doesn't make sense
IMHO if we now are trying to integrate a new kind of policy decision
that has nothing to do with that POlicy Client API (geolocation
permission request).

Perhaps the thing is that "Geolocation permissions requests" should not
be treated as "policy decisions" after all, and so we should have our
own "geolocation-permission-request" signal with its own associated
WebKitGeolocationPermissionRequest object providing allow() / deny()
methods, and that's all. Not sure, something I'm considering, though.

For the sake of the discussions, in order of preference I'd do:

  1. Implement "Option 1", with Carlos's suggestion of making 
     WebKitPolicyDecision an interface (see attachment)

  2. Implement Geolocation permission requests completely out of the 
     policy decisions framework (see paragraph above), as in WK1.

  3. Implement "Option 2", either by doing it "the overengineered
     way" (see previous mail) or following Carlos's suggestion in his
     previous mail to do it simpler.

Problem is that, as far as I know, Martin's preferred order is exactly
the opposite (3 -> 2 -> 1) so we have a conflict here that must be
resolved somehow :-)

Perhaps it's just another clue telling us that (2) is the right one?

Opinions?
Mario
-------------- next part --------------
A non-text attachment was scrubbed...
Name: geolocationWK2-option1.png
Type: image/png
Size: 23810 bytes
Desc: not available
URL: <http://lists.webkit.org/pipermail/webkit-gtk/attachments/20120402/0e66c810/attachment-0001.png>