[webkit-reviews] review denied: [Bug 69870] [Chromium] Empty API for web intents : [Attachment 115257] Patch
bugzilla-daemon at webkit.org
bugzilla-daemon at webkit.org
Tue Nov 15 21:14:58 PST 2011
Darin Fisher (:fishd, Google) <fishd at chromium.org> has denied Greg Billock
<gbillock at google.com>'s request for review:
Bug 69870: [Chromium] Empty API for web intents
https://bugs.webkit.org/show_bug.cgi?id=69870
Attachment 115257: Patch
https://bugs.webkit.org/attachment.cgi?id=115257&action=review
------- Additional Comments from Darin Fisher (:fishd, Google)
<fishd at chromium.org>
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.
More information about the webkit-reviews
mailing list