Bug 17115 - [GTK] Provide asynchonous response mechanism
Summary: [GTK] Provide asynchonous response mechanism
Status: RESOLVED INVALID
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKitGTK (show other bugs)
Version: 528+ (Nightly build)
Hardware: PC Linux
: P2 Enhancement
Assignee: Nobody
URL:
Keywords: Gtk
Depends on:
Blocks: 16562 16947
  Show dependency treegraph
 
Reported: 2008-01-31 08:09 PST by Pierre-Luc Beaudoin
Modified: 2008-02-13 04:32 PST (History)
4 users (show)

See Also:


Attachments
WebKitResponseListener (7.94 KB, patch)
2008-01-31 08:11 PST, Pierre-Luc Beaudoin
no flags Details | Formatted Diff | Diff
Name changes (7.60 KB, patch)
2008-02-04 08:15 PST, Pierre-Luc Beaudoin
no flags Details | Formatted Diff | Diff
Applied comments (9.03 KB, patch)
2008-02-04 16:04 PST, Pierre-Luc Beaudoin
mrowe: review-
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Pierre-Luc Beaudoin 2008-01-31 08:09:33 PST
We need an asynchronous response system so we don't lock the application while waiting for a response from the user.
Comment 1 Pierre-Luc Beaudoin 2008-01-31 08:11:49 PST
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.
Comment 2 Christian Dywan 2008-01-31 09:01:57 PST
(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.
Comment 3 Luca Bruno 2008-01-31 10:02:55 PST
> > +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.
Comment 4 Pierre-Luc Beaudoin 2008-02-04 08:15:32 PST
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.
Comment 5 Mark Rowe (bdash) 2008-02-04 15:02:39 PST
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.
Comment 6 Pierre-Luc Beaudoin 2008-02-04 16:04:49 PST
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
Comment 7 Mark Rowe (bdash) 2008-02-04 23:53:46 PST
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.
Comment 8 Mark Rowe (bdash) 2008-02-04 23:54:36 PST
Comment on attachment 18922 [details]
Applied comments

Going to say r- so the issues raised can be considered further.
Comment 9 Christian Dywan 2008-02-05 05:53:26 PST
(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.
Comment 10 Mark Rowe (bdash) 2008-02-05 06:31:18 PST
(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.
Comment 11 Pierre-Luc Beaudoin 2008-02-06 15:24:52 PST
This approach has been deprecated in favour of per usage specific listener ie. WekitWebPolicyDecision that will require no enums to ease binding writing.
Comment 12 Alp Toker 2008-02-13 04:32:43 PST
See bug #16562 for new ideas on async API