Summary: | [GTK] Provide asynchonous response mechanism | ||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Pierre-Luc Beaudoin <pierre-luc.beaudoin> | ||||||||
Component: | WebKitGTK | Assignee: | Nobody <webkit-unassigned> | ||||||||
Status: | RESOLVED INVALID | ||||||||||
Severity: | Enhancement | CC: | alp, christian, lethalman88, tofu.linden | ||||||||
Priority: | P2 | Keywords: | Gtk | ||||||||
Version: | 528+ (Nightly build) | ||||||||||
Hardware: | PC | ||||||||||
OS: | Linux | ||||||||||
Bug Depends on: | |||||||||||
Bug Blocks: | 16562, 16947 | ||||||||||
Attachments: |
|
Description
Pierre-Luc Beaudoin
2008-01-31 08:09:33 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.
(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. > > +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.
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 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.
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
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 on attachment 18922 [details]
Applied comments
Going to say r- so the issues raised can be considered further.
(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. (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. This approach has been deprecated in favour of per usage specific listener ie. WekitWebPolicyDecision that will require no enums to ease binding writing. See bug #16562 for new ideas on async API |