Bug 69870 – [Chromium] Empty API for web intents

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Greg Billock]

Empty API for web intents

[Greg Billock]

Created attachment 110579 [details]
Patch

[WebKit Review Bot]

Please wait for approval from [e-mail] before submitting because this patch contains changes to the Chromium public API.

[Greg Billock]

To provide some context: this is a dummy WebKit/chromium API which would let me land some Chromium-side code implementing the return pathway of our prototype webintents work.

If the CC'ed are interested in where we are with that work, let me know and I can provide more detail. Thanks!

[Greg Billock]

Created attachment 114381 [details]
Patch

[Darin Fisher (:fishd, Google)]

Comment on attachment 114381 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=114381&action=review

> Source/WebKit/chromium/public/WebIntentsController.h:42
> +    WEBKIT_EXPORT void webIntentReply(ReplyType, const WebString&, int);

this looks like it should just be a method on WebFrame.  it should probably have a name that mirrors startActivity, like didCompleteActivity.  For bonus points, the name should really say something about intents.

> Source/WebKit/chromium/public/WebViewClient.h:323
> +    virtual void registerIntentHandler(const WebString& action,

it seems like the request to register an intent handler should be relative to the WebFrame so that the embedder can tell what origin is requesting the intent handler.

it would probably be good to bundle up these request parameters into a separate structure.

> Source/WebKit/chromium/public/WebViewClient.h:330
> +    virtual void startActivity(const WebString& action,

it seems like activities are started by script in the context of a document.  therefore this should also be a WebFrame-relative method (i.e., it should be declared on WebFrameClient).  perhaps the parameters should be bottled up into a WebIntent structure (or some other structure)?

what is the intentId?  is that supposed to be passed back to WebKit as the third parameter to webIntentReply?  is there really any need for an intent identifier?  can there be multiple overlapping startActivity calls made by a single script context?

[Greg Billock]

Comment on attachment 114381 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=114381&action=review

>> Source/WebKit/chromium/public/WebIntentsController.h:42
>> +    WEBKIT_EXPORT void webIntentReply(ReplyType, const WebString&, int);
> 
> this looks like it should just be a method on WebFrame.  it should probably have a name that mirrors startActivity, like didCompleteActivity.  For bonus points, the name should really say something about intents.

My goal was to put the dispatch code into the WebCore controller object. Do you think it is preferable to dispatch in the embedder code? I was copying at the way the other notifications processes work (i.e. geolocation notices), which are passed through to controller objects in WebCore.

>> Source/WebKit/chromium/public/WebViewClient.h:323
>> +    virtual void registerIntentHandler(const WebString& action,
> 
> it seems like the request to register an intent handler should be relative to the WebFrame so that the embedder can tell what origin is requesting the intent handler.
> 
> it would probably be good to bundle up these request parameters into a separate structure.

That makes sense. Something to consider alongside this, though: the registerProtocolHandler call is on WebViewClient. Do you think that's misplaced? One comes from a tag and one from a navigator call. But startActivity is on the navigator object as well. I agree knowing the origin is important, but we could pass this along instead of using the FrameLoaderClient->WebFrameClient pathway. Do you think that's preferable? There are a few WebFrameClient-style "method(WebFrame*, args) methods in WebViewClient, so this wouldn't be the first...

>> Source/WebKit/chromium/public/WebViewClient.h:330
>> +    virtual void startActivity(const WebString& action,
> 
> it seems like activities are started by script in the context of a document.  therefore this should also be a WebFrame-relative method (i.e., it should be declared on WebFrameClient).  perhaps the parameters should be bottled up into a WebIntent structure (or some other structure)?
> 
> what is the intentId?  is that supposed to be passed back to WebKit as the third parameter to webIntentReply?  is there really any need for an intent identifier?  can there be multiple overlapping startActivity calls made by a single script context?

I currently have the intent ID as a way to make a positive connection between the invocation and the registered javascript callback. If the policy is that we'll only allow one intent in process, then there's no need for it; we'll just have one registered callback. I didn't want to hardcode that policy at this point, though.

If we have a structure for the intent data, it'll be moot, as the intent ID will ride along there. In my webkit implementation patch I do currently have a WebCore Intent object that would be natural wrapped analog for this.

[Greg Billock]

Created attachment 115257 [details]
Patch

[Darin Fisher (:fishd, Google)]

Comment on attachment 115257 [details]
Patch

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.

[Greg Billock]

Created attachment 115418 [details]
Patch

[Greg Billock]

Comment on attachment 115257 [details]
Patch

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.

[Darin Fisher (:fishd, Google)]

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

[Darin Fisher (:fishd, Google)]

Comment on attachment 115418 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=115418&action=review

> Source/WebKit/chromium/public/WebFrame.h:556
> +    virtual void replyFromIntentService(WebIntentsReplyType, const WebString&, int intentId) = 0;

How about calling this "handleIntentResult" or "handleIntentReply"?

> Source/WebKit/chromium/public/WebIntentServiceData.h:48
> +class WebIntentServiceData {

nit: "Info" might be a better suffix for this class.  See for example WebPopupMenuInfo.

> Source/WebKit/chromium/public/WebIntentServiceData.h:53
> +    WEBKIT_EXPORT void setUrl(const WebURL&);

nit: setUrl should be setURL per the style guide

[Greg Billock]

Created attachment 115877 [details]
Patch

[Greg Billock]

Comment on attachment 115418 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=115418&action=review

>> Source/WebKit/chromium/public/WebFrame.h:556
>> +    virtual void replyFromIntentService(WebIntentsReplyType, const WebString&, int intentId) = 0;
> 
> How about calling this "handleIntentResult" or "handleIntentReply"?

As we discussed, changed to handleIntentResult and handleIntentFailure. The signatures can be evolved separately now if needed.

>> Source/WebKit/chromium/public/WebIntentServiceData.h:48
>> +class WebIntentServiceData {
> 
> nit: "Info" might be a better suffix for this class.  See for example WebPopupMenuInfo.

Done.

>> Source/WebKit/chromium/public/WebIntentServiceData.h:53
>> +    WEBKIT_EXPORT void setUrl(const WebURL&);
> 
> nit: setUrl should be setURL per the style guide

Done.

[Darin Fisher (:fishd, Google)]

Comment on attachment 115877 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=115877&action=review

> Source/WebKit/chromium/public/WebFrame.h:554
> +    virtual void handleIntentFailure(const WebString&, int intentId) = 0;

nit: I think it would be better to list the intentIdentifier first since that
identifies the intent.  the WebString is just payload, so it seems secondary.

nit: spell out intentIdentifier since this corresponds to WebIntent::identifier().
(it helps to keep these as similar as possible to strengthen their relationship.)

> Source/WebKit/chromium/public/WebFrameClient.h:392
> +    // the WebFrame starting the activity. The |intentId| is an arbitrary

nit: fix this comment.  it refers to "intentId" which doesn't exist as a direct parameter to this method.

> Source/WebKit/chromium/public/WebIntentServiceInfo.h:40
> +// The |url| is the location of the handler page registered by the service.

nit: Move these comments to be above their respective method groups.  e.g.:

  // The location of the handler page registered by the service.
  WEBKIT_EXPORT WebURL url() const;
  WEBKIT_EXPORT void setURL(const WebURL&);

^^^ This way you don't have to repeat the name of the property.

> Source/WebKit/chromium/public/WebIntentServiceInfo.h:53
> +    WEBKIT_EXPORT WebString url() const;

I think you meant url() to return WebURL?

[Greg Billock]

Created attachment 116124 [details]
Patch

[WebKit Review Bot]

Comment on attachment 116124 [details]
Patch

Attachment 116124 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/10554003

[Greg Billock]

Comment on attachment 115877 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=115877&action=review

>> Source/WebKit/chromium/public/WebFrame.h:554
>> +    virtual void handleIntentFailure(const WebString&, int intentId) = 0;
> 
> nit: I think it would be better to list the intentIdentifier first since that
> identifies the intent.  the WebString is just payload, so it seems secondary.
> 
> nit: spell out intentIdentifier since this corresponds to WebIntent::identifier().
> (it helps to keep these as similar as possible to strengthen their relationship.)

Done.

>> Source/WebKit/chromium/public/WebFrameClient.h:392
>> +    // the WebFrame starting the activity. The |intentId| is an arbitrary
> 
> nit: fix this comment.  it refers to "intentId" which doesn't exist as a direct parameter to this method.

Done.

>> Source/WebKit/chromium/public/WebIntentServiceInfo.h:40
>> +// The |url| is the location of the handler page registered by the service.
> 
> nit: Move these comments to be above their respective method groups.  e.g.:
> 
>   // The location of the handler page registered by the service.
>   WEBKIT_EXPORT WebURL url() const;
>   WEBKIT_EXPORT void setURL(const WebURL&);
> 
> ^^^ This way you don't have to repeat the name of the property.

Done.

>> Source/WebKit/chromium/public/WebIntentServiceInfo.h:53
>> +    WEBKIT_EXPORT WebString url() const;
> 
> I think you meant url() to return WebURL?

This is representing the "href" attribute of the intent tag. Will we get sufficient error handling by returning WebURL?

[Darin Fisher (:fishd, Google)]

(In reply to comment #18)
> >> Source/WebKit/chromium/public/WebIntentServiceInfo.h:53
> >> +    WEBKIT_EXPORT WebString url() const;
> > 
> > I think you meant url() to return WebURL?
> 
> This is representing the "href" attribute of the intent tag. Will we get sufficient error handling by returning WebURL?

I don't think you should be calling registerIntentHandler with a bogus URL.

[Greg Billock]

Created attachment 116141 [details]
Patch

[Darin Fisher (:fishd, Google)]

Comment on attachment 116141 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=116141&action=review

> Source/WebKit/chromium/public/WebFrame.h:550
> +    // appropriate registered Javascript callback. The |intentId| is the

nit: intentId -> intentIdentifier

> Source/WebKit/chromium/public/WebFrame.h:556
> +    // Javascript callback. The |intentId| is the parameter received from

nit: intentId -> intentIdentifier

[Greg Billock]

Created attachment 116146 [details]
Patch

[Greg Billock]

Comment on attachment 116141 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=116141&action=review

>> Source/WebKit/chromium/public/WebFrame.h:550
>> +    // appropriate registered Javascript callback. The |intentId| is the
> 
> nit: intentId -> intentIdentifier

Done.

>> Source/WebKit/chromium/public/WebFrame.h:556
>> +    // Javascript callback. The |intentId| is the parameter received from
> 
> nit: intentId -> intentIdentifier

Done.

[WebKit Review Bot]

Comment on attachment 116146 [details]
Patch

Rejecting attachment 116146 [details] from commit-queue.

New failing tests:
fast/repaint/fixed-scale.html
fast/repaint/scroll-fixed-reflected-layer.html
fast/repaint/fixed-table-overflow-zindex.html
fast/repaint/fixed-tranformed.html
Full output: http://queues.webkit.org/results/10559029

[WebKit Review Bot]

Comment on attachment 116146 [details]
Patch

Clearing flags on attachment: 116146

Committed r101021: <http://trac.webkit.org/changeset/101021>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.