[Webkit-unassigned] [Bug 69870] [Chromium] Empty API for web intents

bugzilla-daemon at webkit.org bugzilla-daemon at webkit.org
Wed Nov 16 11:47:18 PST 2011


https://bugs.webkit.org/show_bug.cgi?id=69870


Greg Billock <gbillock at google.com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
 Attachment #115257|1                           |0
        is obsolete|                            |




--- Comment #10 from Greg Billock <gbillock at google.com>  2011-11-16 11:47:18 PST ---
(From update of attachment 115257)
View in context: https://bugs.webkit.org/attachment.cgi?id=115257&action=review

>> Source/WebKit/chromium/public/WebFrame.h:555
>> +    // for managing the return of the reply data to the correct (source)
> 
> I'm confused by the last sentence.  Isn't the "correct (source) WebFrame object" the
> WebFrame object on which you call this method?
> 
> What is the 'int' parameter?  Please give it a name.

I wanted to document that this method is what you use to return a value to the WebFrame included in WebFrameClient::startWebIntentActivity. I moved the documentation there, though.

Added "intentId" name for the int parameter and documentation about where it comes from and what to do with it.

>> Source/WebKit/chromium/public/WebFrame.h:557
>> +    virtual void replyFromWebIntentActivity(WebIntentsReplyType, const WebString&, int) = 0;
> 
> it occurred to me that you must also require a way to pass an Intent to the page for processing.
> is that going to happen through a separate patch?
> 
> it'd be nice to pick a name for this method here that somehow ties back to the method
> on WebFrameClient used to dispatch the intent.
> 
> perhaps the reply parameters should also be bottled up into a class?  WebIntentResult?

Right now I'm doing that through the NPAPI and putting it on window.intent. If we change that, it'll go in a future patch.

I tried to match up startWebIntentActivity with replyFromWebIntentActivity. Based on the rename to dispatchIntent below, I've renamed to "replyFromIntentService".

The reply type is just a serialized object. Do you think a class wrapper is a good idea there even so?

>> Source/WebKit/chromium/public/WebFrameClient.h:389
>> +    virtual void didRegisterWebIntentHandler(WebFrame*, const WebIntentServiceData&) { }
> 
> nit: since this is a request for the embedder to take an action, i'd drop the "did" prefix.
> the registration actually hasn't happened yet, right?  you rely on the embedder to actually
> perform the registration.  also, we normally avoid the "Web" prefix in method names.
> 
>   registerIntentHandler(WebFrame*, const WebIntentHandlerInfo&);
> 
> it seems like it might be nice to either use the term "service" or the term "handler",
> but using both terms for the same thing is confusing.  how about renaming WebIntentServiceData
> to something like WebIntentHandler or WebIntentHandlerInfo?

Good point. Renamed to registerIntentService. The name was chosen to parallel registerProtocolHandler, but registerIntentService is better.

>> Source/WebKit/chromium/public/WebFrameClient.h:392
>> +    virtual void startWebIntentActivity(WebFrame*, const WebIntentData&, int) { }
> 
> I know the API is web API is called startActivity, and so it may make sense to reuse that term,
> but again, it seems a bit like we are unnecessarily using multiple terms to refer to the same
> thing.  we call it both an "intent" as well as an "activity"... naming this method something
> like dispatchIntent seems like it might be better.  It makes it very clear that the purpose
> of this method is to take the given intent and dispatch it.
> 
> nit: the 'int' parameter should have a name.

The "intent" is the data payload passed to the activity. The API name ("startActivity") comes from the parallel to Android. I think "dispatchIntent" sounds good. What should the reply method be then? I renamed to "replyFromIntentService"... WDYT?

>> Source/WebKit/chromium/public/WebIntentData.h:13
>> +class WebIntentData {
> 
> since this corresponds to a "Javascript Intent object", why not just call this class "WebIntent"?

Done.

>> Source/WebKit/chromium/public/WebIntentServiceData.h:17
>> +    WEBKIT_EXPORT WebString serviceUrl() const;
> 
> nit: the type name says "service" in it, so probably don't need to say service here.  this
> field can just be called "url" and it will be apparent that it is the "url" of the "service"
> given the scoping of the method.
> 
> it seems like it would be helpful to add a comment to the top of this file that points to
> the section in the spec where the service data is described.  that way it is possible to
> learn what title, action, type and disposition fields are supposed to mean.  or, you could
> document them here.

We don't have a stable URL for the spec yet. Shall I include one to our proposal? Added inline docs anyway, since they're helpful.

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.



More information about the webkit-unassigned mailing list