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

[bugzilla-daemon at webkit.org]

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





--- Comment #11 from Darin Fisher (:fishd, Google) <fishd at chromium.org>  2011-11-18 11:08:40 PST ---
(In reply to comment #10)
> (From update of attachment 115257 [details])
> 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.

Perhaps the intentId could just be a property of WebIntent?  WebIntent could have an identifier member.
Since you are calling this parameter intentId, you are saying "intent identifier", and that could just
be WebIntent::identifier, right?


> >> 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.

We have to change that.  It won't be possible to test the WebKit bits using LayoutTests without that, right?


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

I think this might be better named "handleIntent{Reply,Result}"...

I'm thinking of dispatchEvent and handleEvent.


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

Oh... a serialized object.  Should this be using WebSerializedScriptValue?  Should WebIntent::data be a WebSerializedScriptValue too?

I would probably go with WebIntent{Reply,Result} and give it a data member and an identifier member.


> >> 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.

OK


> >> 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.

A reference to the proposed spec is helpful.

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