Empty API for web intents
Created attachment 110579 [details] Patch
Please wait for approval from fishd@chromium.org before submitting because this patch contains changes to the Chromium public API.
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!
Created attachment 114381 [details] Patch
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?
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.
Created attachment 115257 [details] Patch
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.
Created attachment 115418 [details] Patch
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.
(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.
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
Created attachment 115877 [details] Patch
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.
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?
Created attachment 116124 [details] Patch
Comment on attachment 116124 [details] Patch Attachment 116124 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/10554003
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?
(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.
Created attachment 116141 [details] Patch
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
Created attachment 116146 [details] Patch
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.
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
Comment on attachment 116146 [details] Patch Clearing flags on attachment: 116146 Committed r101021: <http://trac.webkit.org/changeset/101021>
All reviewed patches have been landed. Closing bug.