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

[bugzilla-daemon at webkit.org]

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


Darin Fisher (:fishd, Google) <fishd at chromium.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
 Attachment #115257|review?                     |review-
               Flag|                            |




--- Comment #8 from Darin Fisher (:fishd, Google) <fishd at chromium.org>  2011-11-15 21:14:58 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.

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

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

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

> Source/WebKit/chromium/public/WebIntentData.h:13
> +class WebIntentData {

since this corresponds to a "Javascript Intent object", why not just call this class "WebIntent"?

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

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