Bug 17115 – [GTK] Provide asynchonous response mechanism

• Status: RESOLVED
• Resolution: INVALID
• Component: WebKitGTK

[Pierre-Luc Beaudoin]

We need an asynchronous response system so we don't lock the application while waiting for a response from the user.

[Pierre-Luc Beaudoin]

Created attachment 18815 [details]
WebKitResponseListener

This implements the requested mechanism.  Original works by Luca Bruno.  
Each responses enum (to pass as a WebKitListenerResponse) shall be defined in the header file of the object that needs a response.

[Christian Dywan]

(In reply to comment #1)
> Created an attachment (id=18815) [edit]
> WebKitResponseListener

> +typedef int64_t WebKitListenerResponse;

I find this name confusing. If the intent is to prevent redundancy, it really should be 'WebKitResponseFoo'. Perhaps 'WebKitResponseAction'?

Otherwise I appreciate that you are bringing this forward.

[Luca Bruno]

> > +typedef int64_t WebKitListenerResponse;
> 
> I find this name confusing. If the intent is to prevent redundancy, it really
> should be 'WebKitResponseFoo'. Perhaps 'WebKitResponseAction'?
> 
> Otherwise I appreciate that you are bringing this forward.
> 

I do agree, it's confusing.

[Pierre-Luc Beaudoin]

Created attachment 18907 [details]
Name changes

Complete name changes.  Alp Toker suggested WebKitWebDecision instead of WebKitResponseListener.  His argument was that Response could be mistaken for a HTTP or DocumentLoader response.  Decision is used in WebPolicyDecisionListener on the Mac, making it a good replacement choice.

I changed _reply to _send since we don't reply to a decision but send it to the requester.  I also renamed WebKitListenerResponse to WebKitWebDecisionValue.

[Mark Rowe (bdash)]

Comment on attachment 18907 [details]
Name changes

This look generally ok to me.  There are some minor issues with naming and the documentation that were mentioned on IRC, so a further revision of the patch with tweaks to those aspects would be great before landing this.

[Pierre-Luc Beaudoin]

Created attachment 18922 [details]
Applied comments

Addressed some final comments:
* Apply correct CamelCase in the cpp file
* Use decision->priv instead of the macro
* improve documentation
* use g_int64
* make it build

[Mark Rowe (bdash)]

A few things that came up when discussing this patch with Alp:
* The definition + use of webkit_web_decision_finalize appears to be unnecessary.
* webkit_web_decision_new should not be part of the API as it would only be used by WebKit.
* "typedef struct _WebKitWebDecision WebKitWebDecision" and friends should live on webkitdefines.h alongside other similar typedefs.

Alp also raised the issue of how well a enum-based approach would map to some language bindings.  One possible alternative could be to have a WebDecision class with subclasses for each of the types of decisions that would be requested (eg, WebPolicyDecision) which exposes methods which should be called  when the decision has been made.

[Mark Rowe (bdash)]

Comment on attachment 18922 [details]
Applied comments

Going to say r- so the issues raised can be considered further.

[Christian Dywan]

(In reply to comment #7)
> * webkit_web_decision_new should not be part of the API as it would only be
> used by WebKit.

I imagine the intent of making that public was to allow the client to add its own custom decisions. However the idea of using subclasses sounds interesting and would elegantly cover this.

[Mark Rowe (bdash)]

(In reply to comment #9)
> (In reply to comment #7)
> > * webkit_web_decision_new should not be part of the API as it would only be
> > used by WebKit.
> 
> I imagine the intent of making that public was to allow the client to add its
> own custom decisions.

I don't think is something we need to consider.  Providing a completely generic asynchronous response API is beyond the scope of the functionality WebKit should provide.

[Pierre-Luc Beaudoin]

This approach has been deprecated in favour of per usage specific listener ie. WekitWebPolicyDecision that will require no enums to ease binding writing.

[Alp Toker]

See bug #16562 for new ideas on async API