Bug 76014

Created attachment 121951 [details] Patch

Please wait for approval from fishd@chromium.org before submitting because this patch contains changes to the Chromium public API.

Comment on attachment 121951 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=121951&action=review > Source/WebKit/chromium/public/WebIntent.h:66 > + void postResult(const WebString&); > + void postFailure(const WebString&); Virtual?

Created attachment 122056 [details] Patch

Comment on attachment 121951 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=121951&action=review >> Source/WebKit/chromium/public/WebIntent.h:66 >> + void postFailure(const WebString&); > > Virtual? Yes. Botched this just before leaving last night.

Comment on attachment 122056 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=122056&action=review I wonder if it would be better to use two separate objects in the interface, a WebIntent object and a WebIntentRequest object. Each of them could wrap a RefPtr to the WebCore object, like we do for WebCore::Node. That seems less error prone w.r.t. memory management. (I'm also happy to defer to fishd on these matters.) > Source/WebKit/chromium/public/WebIntent.h:65 > + ~WebIntentReplyBroker() { } Should the destructor be virtual too? > Source/WebKit/chromium/public/WebIntent.h:76 > + explicit WebIntent(const WTF::PassRefPtr<WebCore::IntentRequest>&); Should we rename this interface to WebIntentRequest to match the WebCore object? Maybe we should have a WebIntentRequest that has a WebIntent as a member to mirror the structure of the WebCore classes. > Source/WebKit/chromium/src/WebIntent.cpp:88 > + delete m_broker; Do we not have an OwnPtr variant for the API? > Source/WebKit/chromium/src/WebIntent.cpp:133 > - m_identifier = identifier; > + WebIntentReplyBroker* broker = m_broker; > + m_broker = 0; > + return broker; I see. This transfers ownership to the embedder. I don't remember how we typically manage memory in the API. I'll let fishd comment on that aspect. From this design, it seems clear that WebIntentReplyBroker needs a virtual destructor.

Comment on attachment 122056 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=122056&action=review >> Source/WebKit/chromium/public/WebIntent.h:65 >> + ~WebIntentReplyBroker() { } > > Should the destructor be virtual too? yes >> Source/WebKit/chromium/public/WebIntent.h:76 >> + explicit WebIntent(const WTF::PassRefPtr<WebCore::IntentRequest>&); > > Should we rename this interface to WebIntentRequest to match the WebCore object? Maybe we should have a WebIntentRequest that has a WebIntent as a member to mirror the structure of the WebCore classes. Yeah, I re-used the WebIntent object because I'd already created it while writing the chromium-side code. I agree it isn't the best name. Shall I make a new one now, or put it off to later? >> Source/WebKit/chromium/src/WebIntent.cpp:88 >> + delete m_broker; > > Do we not have an OwnPtr variant for the API? I looked at WebPrivatePtr and WebPrivateOwnPtr before doing this manually, but those are not really appropriate. WebPrivateOwnPtr is close, but it has to be manually cleared before exit anyway, and the semantics aren't right. "WebOwnPtr" is probably what we need. Is it worth it to make such a thing? I looked around, but didn't find a pattern elsewhere where we transfer object ownership up like this. There's a lot of classes, though. Is there one I should copy? >> Source/WebKit/chromium/src/WebIntent.cpp:133 >> + return broker; > > I see. This transfers ownership to the embedder. I don't remember how we typically manage memory in the API. I'll let fishd comment on that aspect. From this design, it seems clear that WebIntentReplyBroker needs a virtual destructor. Yes.

Comment on attachment 122056 [details] Patch As discussed over IM, please just use WebPrivatePtr to wrap WebCore::IntentRequest in a simple fashion. See WebNode for example.

Created attachment 122099 [details] Patch

(In reply to comment #8) > (From update of attachment 122056 [details]) > As discussed over IM, please just use WebPrivatePtr to wrap WebCore::IntentRequest in a simple fashion. See WebNode for example. Yeah, this comes out nicely. I'm just transferring ownership of the WebIntentRequest directly, not using any pass-through object wrapper as before. Does that look OK? Should I add code to delete this param in the empty impl of WebFrameClient?

Comment on attachment 122099 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=122099&action=review > Source/WebKit/chromium/public/WebIntentRequest.h:57 > + WEBKIT_EXPORT WebString action() const; > + > + WEBKIT_EXPORT WebString type() const; > + > + WEBKIT_EXPORT WebString data() const; Do you want to expose these accessors on WebIntentRequest, or do you just want to expose an access for the WebIntent itself? > Source/WebKit/chromium/src/FrameLoaderClientImpl.cpp:1636 > + m_webFrame->client()->dispatchIntent(webFrame(), new WebIntentRequest(intentRequest)); Do you want to use "new" here, or have it stack allocated? > Source/WebKit/chromium/src/WebIntentRequest.cpp:59 > + if (m_intentRequest.get() && m_intentRequest->intent()) is m_intentRequest->intent() ever null? It seems meaningless to have an intent request without an intent.

Comment on attachment 122099 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=122099&action=review >> Source/WebKit/chromium/public/WebIntentRequest.h:57 >> + WEBKIT_EXPORT WebString data() const; > > Do you want to expose these accessors on WebIntentRequest, or do you just want to expose an access for the WebIntent itself? I think it is easier to just use these. See http://codereview.chromium.org/9186021/ for the chromium-side code that uses this. This way I can remove the WebIntent object in a follow-on. >> Source/WebKit/chromium/src/FrameLoaderClientImpl.cpp:1636 >> + m_webFrame->client()->dispatchIntent(webFrame(), new WebIntentRequest(intentRequest)); > > Do you want to use "new" here, or have it stack allocated? I'm passing ownership to the client. I think if I stack-allocate it I'd have to make a copy in the client anyway. Is that preferable? It makes the stub WebFrameClient default code simpler, but to me it obscures that the callee is expected to take ownership of the object. I see WebNode ownership transfer happening in, i.e. WebDOMEvent, but it is using a mechanism similar to the one I had before, where there's an intermediary object you grab the reference from (but that's just because of the larger interface there, it seems). >> Source/WebKit/chromium/src/WebIntentRequest.cpp:59 >> + if (m_intentRequest.get() && m_intentRequest->intent()) > > is m_intentRequest->intent() ever null? It seems meaningless to have an intent request without an intent. We discussed before that we may want to clear it out, since it might be heavy. That kind of makes this API state-dependent, which is awkward, but it may be preferable to carrying a large Intent object payload for a long time.

Comment on attachment 122099 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=122099&action=review > Source/WebKit/chromium/public/WebIntentRequest.h:51 > + ~WebIntentRequest(); remember, if you do not implement a method inline or behind a WEBKIT_IMPLEMENTATION guard, then it needs to be decorated with WEBKIT_EXPORT. else the DLL build will fail to link. >>> Source/WebKit/chromium/src/WebIntentRequest.cpp:59 >>> + if (m_intentRequest.get() && m_intentRequest->intent()) >> >> is m_intentRequest->intent() ever null? It seems meaningless to have an intent request without an intent. > > We discussed before that we may want to clear it out, since it might be heavy. That kind of makes this API state-dependent, which is awkward, but it may be preferable to carrying a large Intent object payload for a long time. Sorry to create more work for you, but we really try to make the WebKit API just be a thin wrapper for WebCore. You might as well expose WebIntent since it can easily wrap WebCore::Intent as that is also ref-counted. This way as these objects grow, you don't have to have the WebKit API concepts differ from the WebCore concepts. Past experience suggests that keeping these aligned helps. > Source/WebKit/chromium/src/WebIntentRequest.cpp:86 > +void WebIntentRequest::postResult(const WebKit::WebString& data) nit: postResult and postFailure should take WebSerializedScriptValue. this will help when you want to post more than just a string, which i'm sure we will want to do. it also will reduce the code for the WebKit layer, which is always a good thing. > Source/WebKit/chromium/src/WebIntentRequest.cpp:99 > + if (m_intentRequest.get()) it does not look possible to create a WebIntentRequest that has a null m_intentRequest. why are you null checking? just ASSERT or let it crash.

Comment on attachment 122099 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=122099&action=review >> Source/WebKit/chromium/public/WebIntentRequest.h:51 >> + ~WebIntentRequest(); > > remember, if you do not implement a method inline or behind a WEBKIT_IMPLEMENTATION guard, then it needs to be decorated with WEBKIT_EXPORT. else the DLL build will fail to link. Moved the empty impl here inline >>> Source/WebKit/chromium/public/WebIntentRequest.h:57 >>> + WEBKIT_EXPORT WebString data() const; >> >> Do you want to expose these accessors on WebIntentRequest, or do you just want to expose an access for the WebIntent itself? > > I think it is easier to just use these. See http://codereview.chromium.org/9186021/ for the chromium-side code that uses this. This way I can remove the WebIntent object in a follow-on. Followed Darin's advice and just put in the WebIntent object. >>> Source/WebKit/chromium/src/FrameLoaderClientImpl.cpp:1636 >>> + m_webFrame->client()->dispatchIntent(webFrame(), new WebIntentRequest(intentRequest)); >> >> Do you want to use "new" here, or have it stack allocated? > > I'm passing ownership to the client. I think if I stack-allocate it I'd have to make a copy in the client anyway. Is that preferable? It makes the stub WebFrameClient default code simpler, but to me it obscures that the callee is expected to take ownership of the object. I see WebNode ownership transfer happening in, i.e. WebDOMEvent, but it is using a mechanism similar to the one I had before, where there's an intermediary object you grab the reference from (but that's just because of the larger interface there, it seems). Any view on this, Darin? Pass pointer or use a copy constructor? >>>> Source/WebKit/chromium/src/WebIntentRequest.cpp:59 >>>> + if (m_intentRequest.get() && m_intentRequest->intent()) >>> >>> is m_intentRequest->intent() ever null? It seems meaningless to have an intent request without an intent. >> >> We discussed before that we may want to clear it out, since it might be heavy. That kind of makes this API state-dependent, which is awkward, but it may be preferable to carrying a large Intent object payload for a long time. > > Sorry to create more work for you, but we really try to make the WebKit API just be a thin wrapper for WebCore. You might as well expose WebIntent since it can easily wrap WebCore::Intent as that is also ref-counted. This way as these objects grow, you don't have to have the WebKit API concepts differ from the WebCore concepts. Past experience suggests that keeping these aligned helps. OK, switched to that. >> Source/WebKit/chromium/src/WebIntentRequest.cpp:86 >> +void WebIntentRequest::postResult(const WebKit::WebString& data) > > nit: postResult and postFailure should take WebSerializedScriptValue. this will help when you want to post more than just a string, which i'm sure we will want to do. it also will reduce the code for the WebKit layer, which is always a good thing. Done. >> Source/WebKit/chromium/src/WebIntentRequest.cpp:99 >> + if (m_intentRequest.get()) > > it does not look possible to create a WebIntentRequest that has a null m_intentRequest. why are you null checking? just ASSERT or let it crash. Done.

Created attachment 122309 [details] Patch

Comment on attachment 122309 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=122309&action=review > Source/WebKit/chromium/public/WebIntent.h:70 > +#if WEBKIT_IMPLEMENTATION Presumably we'll need this storage in this class even when WEBKIT_IMPLEMENTATION isn't defined.

Comment on attachment 122309 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=122309&action=review >> Source/WebKit/chromium/public/WebIntent.h:70 >> +#if WEBKIT_IMPLEMENTATION > > Presumably we'll need this storage in this class even when WEBKIT_IMPLEMENTATION isn't defined. I've changed the impl to just directly use the fields in the wrapped type. Is this going to be impossible? That is, I can't use ENABLE(WEB_INTENTS) outside the WEBKIT_IMPLEMENTATION ifdef, since Chromium code doesn't know about it. But does that mean the linker is confused about what the methods will be doing internally? My mental image is that the webkit compile will create the object file, and the defines are to not let client code know about these implementation details.

Created attachment 122445 [details] Patch

Comment on attachment 122445 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=122445&action=review > Source/WebKit/chromium/public/WebFrameClient.h:388 > + virtual void dispatchIntent(WebFrame*, WebIntentRequest*) { } WebIntentRequest should be passed using |const Foo&|. the chromium code should retain a WebIntentRequest by using the copy-constructor. the only oddity about this is that it strips the const, but that's the pattern we've been following. treat it like a RefPtr. > Source/WebKit/chromium/public/WebIntent.h:57 > + // FIXME(gbillock): delete this. nit: WebKit style does not put names on FIXME comments like this. > Source/WebKit/chromium/public/WebIntent.h:62 > + WebIntent(const WebIntent&); why don't you want to allow the embedder to copy a WebIntent? i think you will have trouble using WebIntentRequest::intent() if you don't make the copy-constructor available. There's a pattern for writing classes like this. For an example that is simpler than WebNode, see WebUserMediaRequest. You should have the following methods generally speaking on classes that just wrap a ref-counted WebCore type: default constructor copy constructor destructor operator= reset isNull assign you can optionally add equals and operator== if needed. treat this as boilerplate. maybe we should have macros to make this easier to replicate, but we've so far favored the readability of not using macros. same deal for WebIntentRequest.h

Comment on attachment 122445 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=122445&action=review >> Source/WebKit/chromium/public/WebFrameClient.h:388 >> + virtual void dispatchIntent(WebFrame*, WebIntentRequest*) { } > > WebIntentRequest should be passed using |const Foo&|. the chromium code should > retain a WebIntentRequest by using the copy-constructor. the only oddity about > this is that it strips the const, but that's the pattern we've been following. > treat it like a RefPtr. Done. >> Source/WebKit/chromium/public/WebIntent.h:57 >> + // FIXME(gbillock): delete this. > > nit: WebKit style does not put names on FIXME comments like this. Done >> Source/WebKit/chromium/public/WebIntent.h:62 >> + WebIntent(const WebIntent&); > > why don't you want to allow the embedder to copy a WebIntent? i think > you will have trouble using WebIntentRequest::intent() if you don't > make the copy-constructor available. > > There's a pattern for writing classes like this. For an example that > is simpler than WebNode, see WebUserMediaRequest. You should have > the following methods generally speaking on classes that just wrap > a ref-counted WebCore type: > > default constructor > copy constructor > destructor > operator= > reset > isNull > assign > > you can optionally add equals and operator== if needed. > > treat this as boilerplate. maybe we should have macros to make this > easier to replicate, but we've so far favored the readability of not > using macros. > > same deal for WebIntentRequest.h OK. I was trying to only implement the ones I needed, but if there's a macro-like consistency I'll do that.

Created attachment 122474 [details] Patch

Created attachment 122527 [details] Patch

Created attachment 122797 [details] Patch

Created attachment 122827 [details] Patch

Comment on attachment 122827 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=122827&action=review > Source/WebKit/chromium/public/WebFrameClient.h:385 > + // the WebFrame starting the activity. The callee uses this object to This comment is incorrect now, right? "Replies to this request should be sent to __the request object__." > Source/WebKit/chromium/public/WebFrameClient.h:387 > + // soon as possible when it is no longer needed.) This parenthetical comment is perhaps a bit confusing since there is no way to release a WebIntentRequest. > Source/WebKit/chromium/public/WebIntent.h:47 > + WebIntent(); constructors should be implemented inline. > Source/WebKit/chromium/public/WebIntent.h:48 > + WebIntent(const WebIntent&); this should just be an inline call to assign. > Source/WebKit/chromium/public/WebIntentRequest.h:50 > + WebIntentRequest(); implement constructors inline > Source/WebKit/chromium/public/WebIntentRequest.h:70 > +#if ENABLE(WEB_INTENTS) since you've unconditionally declared WebCore::IntentRequest, it seems like this function does not need to be protected by ENABLE(WEB_INTENTS). in generaly, it'd be nice to avoid using ENABLE macros in public header files, even though this one is protected by WEBKIT_IMPLEMENTATION. > Source/WebKit/chromium/src/FrameLoaderClientImpl.cpp:1637 > + WebIntentRequest request(intentRequest); nit: you should be able to delete this line and just pass intentRequest directly to dispatchIntent. it should be implicitly converted to WebIntentRequest. > Source/WebKit/chromium/src/WebIntent.cpp:66 > +#endif nit: use #else here and elsewhere instead of relying on first return statement wins. > Source/WebKit/chromium/src/WebIntentRequest.cpp:109 > + RefPtr<WebCore::SerializedScriptValue> serializedValue = nit: we'd normally write this like so: m_private->postFailure(PassRefPtr<WebCore::SerializedScriptValue>(data).get());

Comment on attachment 122827 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=122827&action=review >> Source/WebKit/chromium/public/WebFrameClient.h:385 >> + // the WebFrame starting the activity. The callee uses this object to > > This comment is incorrect now, right? "Replies to this request should be sent to __the request object__." Fixed up now. Took out the bad information and condensed. >> Source/WebKit/chromium/public/WebFrameClient.h:387 >> + // soon as possible when it is no longer needed.) > > This parenthetical comment is perhaps a bit confusing since there is no way > to release a WebIntentRequest. Removed. >> Source/WebKit/chromium/public/WebIntent.h:47 >> + WebIntent(); > > constructors should be implemented inline. Done. I moved these to the .cc file while I had the ifdefs around the m_private member to make sure they got created in the webkit code. That whole thing was a bad idea, though, now fixed up. >> Source/WebKit/chromium/public/WebIntent.h:48 >> + WebIntent(const WebIntent&); > > this should just be an inline call to assign. Done. >> Source/WebKit/chromium/public/WebIntentRequest.h:50 >> + WebIntentRequest(); > > implement constructors inline Done (ditto). >> Source/WebKit/chromium/public/WebIntentRequest.h:70 >> +#if ENABLE(WEB_INTENTS) > > since you've unconditionally declared WebCore::IntentRequest, it seems like this > function does not need to be protected by ENABLE(WEB_INTENTS). in generaly, it'd > be nice to avoid using ENABLE macros in public header files, even though this one > is protected by WEBKIT_IMPLEMENTATION. Agreed. Removed this. >> Source/WebKit/chromium/src/FrameLoaderClientImpl.cpp:1637 >> + WebIntentRequest request(intentRequest); > > nit: you should be able to delete this line and just pass intentRequest directly to dispatchIntent. it should be implicitly converted to WebIntentRequest. That constructor is marked explicit, so I need to make the temporary. It looks to me like those constructors are typically not marked explicit in the chromium API. Shall I change them? >> Source/WebKit/chromium/src/WebIntent.cpp:66 >> +#endif > > nit: use #else here and elsewhere instead of relying on first return statement wins. Done. >> Source/WebKit/chromium/src/WebIntentRequest.cpp:109 >> + RefPtr<WebCore::SerializedScriptValue> serializedValue = > > nit: we'd normally write this like so: > > m_private->postFailure(PassRefPtr<WebCore::SerializedScriptValue>(data).get()); Done here and above.

Created attachment 123038 [details] Patch

(In reply to comment #26) > >> Source/WebKit/chromium/src/FrameLoaderClientImpl.cpp:1637 > >> + WebIntentRequest request(intentRequest); > > > > nit: you should be able to delete this line and just pass intentRequest directly to dispatchIntent. it should be implicitly converted to WebIntentRequest. > > That constructor is marked explicit, so I need to make the temporary. It looks to me like those constructors are typically not marked explicit in the chromium API. Shall I change them? Ah, you should not mark that WebIntentRequest constructor explicit. The point is to benefit from implicit conversion where you are just converting between like types. A WebIntentRequest is really just a RefPtr<IntentRequest>, and implicit conversion between those should not be something a developer really needs to think about. We've preferred less verbosity when crossing the API boundary in other words!

(In reply to comment #28) > (In reply to comment #26) > > >> Source/WebKit/chromium/src/FrameLoaderClientImpl.cpp:1637 > > >> + WebIntentRequest request(intentRequest); > > > > > > nit: you should be able to delete this line and just pass intentRequest directly to dispatchIntent. it should be implicitly converted to WebIntentRequest. > > > > That constructor is marked explicit, so I need to make the temporary. It looks to me like those constructors are typically not marked explicit in the chromium API. Shall I change them? > > Ah, you should not mark that WebIntentRequest constructor explicit. The point is to benefit from implicit conversion where you are just converting between like types. A WebIntentRequest is really just a RefPtr<IntentRequest>, and implicit conversion between those should not be something a developer really needs to think about. We've preferred less verbosity when crossing the API boundary in other words! Sounds good. Done.

Created attachment 123127 [details] Patch

Comment on attachment 123127 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=123127&action=review > Source/WebKit/chromium/public/WebIntent.h:58 > + WEBKIT_EXPORT bool equals(const WebIntent&); nit: I think equals should be a const method. > Source/WebKit/chromium/public/WebIntentRequest.h:61 > + WEBKIT_EXPORT bool equals(const WebIntentRequest&); nit: equals should be const > Source/WebKit/chromium/src/WebIntentRequest.cpp:93 > +// RefPtr<WebCore::SerializedScriptValue> serializedValue = nit: delete commented out code > Source/WebKit/chromium/src/WebIntentRequest.cpp:104 > +// (PassRefPtr<WebCore::SerializedScriptValue>)data; nit: delete commented out code

Comment on attachment 123127 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=123127&action=review >> Source/WebKit/chromium/public/WebIntent.h:58 >> + WEBKIT_EXPORT bool equals(const WebIntent&); > > nit: I think equals should be a const method. Done >> Source/WebKit/chromium/public/WebIntentRequest.h:61 >> + WEBKIT_EXPORT bool equals(const WebIntentRequest&); > > nit: equals should be const Done >> Source/WebKit/chromium/src/WebIntentRequest.cpp:93 >> +// RefPtr<WebCore::SerializedScriptValue> serializedValue = > > nit: delete commented out code Done >> Source/WebKit/chromium/src/WebIntentRequest.cpp:104 >> +// (PassRefPtr<WebCore::SerializedScriptValue>)data; > > nit: delete commented out code Done

Created attachment 123196 [details] Patch

Comment on attachment 123196 [details] Patch Clearing flags on attachment: 123196 Committed r105469: <http://trac.webkit.org/changeset/105469>

All reviewed patches have been landed. Closing bug.