Bug 87143

Created attachment 143322 [details] Patch

Please wait for approval from abarth@webkit.org, dglazkov@chromium.org, fishd@chromium.org, jamesr@chromium.org or tkent@chromium.org before submitting, as this patch contains changes to the Chromium public API. See also https://trac.webkit.org/wiki/ChromiumWebKitAPI.

Comment on attachment 143322 [details] Patch Attachment 143322 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/12744714 New failing tests: webintents/web-intents-delivery.html webintents/web-intents-reload.html webintents/web-intents-invoke-port.html webintents/web-intents-invoke.html webintents/web-intents-failure.html webintents/web-intents-obj-constructor.html webintents/web-intents-reply.html

Created attachment 143349 [details] Archive of layout-test-results from ec2-cr-linux-02 The attached test failures were seen while running run-webkit-tests on the chromium-ews. Bot: ec2-cr-linux-02 Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'> Platform: Linux-2.6.35-28-virtual-x86_64-with-Ubuntu-10.10-maverick

Created attachment 143583 [details] Patch

Comment on attachment 143583 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=143583&action=review > Source/WebKit/chromium/ChangeLog:8 > + When delivering an intent to webkit, the caller needs to be able to You apparently missed a verb here. > Source/WebKit/chromium/public/WebIntent.h:60 > + Why aren't the extrasValues passed by reference?

Comment on attachment 143583 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=143583&action=review >> Source/WebKit/chromium/ChangeLog:8 >> + When delivering an intent to webkit, the caller needs to be able to > > You apparently missed a verb here. Oops! Fixed. >> Source/WebKit/chromium/public/WebIntent.h:60 >> + > > Why aren't the extrasValues passed by reference? Good catch. Fixed.

Created attachment 143858 [details] Patch

Comment on attachment 143858 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=143858&action=review > Source/WebKit/chromium/public/WebIntent.h:64 > + WEBKIT_EXPORT WebIntent(const WebString& action, const WebString& type, WebBlob*, You could also add a method like: addExtra(name, value) See WebHTTPBody for example. Maybe you want a WebIntentBuilder class?

Comment on attachment 143858 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=143858&action=review >> Source/WebKit/chromium/public/WebIntent.h:64 >> + WEBKIT_EXPORT WebIntent(const WebString& action, const WebString& type, WebBlob*, > > You could also add a method like: addExtra(name, value) > > See WebHTTPBody for example. Maybe you want a WebIntentBuilder class? You mean like WebHTTPBody::append*? A Builder is an option, but it looks like create* or from* static methods are fairly common conventions in the API. Shall I rework these delivery constructors to that form?

Comment on attachment 143858 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=143858&action=review >>> Source/WebKit/chromium/public/WebIntent.h:64 >>> + WEBKIT_EXPORT WebIntent(const WebString& action, const WebString& type, WebBlob*, >> >> You could also add a method like: addExtra(name, value) >> >> See WebHTTPBody for example. Maybe you want a WebIntentBuilder class? > > You mean like WebHTTPBody::append*? > > A Builder is an option, but it looks like create* or from* static methods are fairly common conventions in the API. Shall I rework these delivery constructors to that form? The reason for a builder interface is that it would permit you to avoid having parameters of type WebVector. You could just have appendFoo methods. If that is not appealing, then what you have here is fine. Also note that we don't generally export constructors: http://trac.webkit.org/wiki/ChromiumWebKitAPI#Methods > Source/WebKit/chromium/public/WebIntent.h:106 > + mutable WebMessagePortChannelArray* m_ownedChannels; it looks like m_ownedChannels does not survive a copy. consider code like this: WebIntent a; WebIntent b(..., ports, ...); a = b; The "a" object would no longer retain ownership of ports. If b is destroyed, then the ports will be deleted too. Is this what you intended? Keep in mind that the point of using WebPrivatePtr<T> is to build data types that can be easily copied around. Normally, the copy is not a lossy operation. > Source/WebKit/chromium/src/WebIntent.cpp:96 > + extras.add(extrasNames[i], extrasValues[i]); indentation > Source/WebKit/chromium/src/WebIntent.cpp:110 > + WebSerializedScriptValue serializedData = WebSerializedScriptValue::serialize(blob->toV8Value()); please avoid so much code duplication. use a helper function.

Comment on attachment 143858 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=143858&action=review >>>> Source/WebKit/chromium/public/WebIntent.h:64 >>>> + WEBKIT_EXPORT WebIntent(const WebString& action, const WebString& type, WebBlob*, >>> >>> You could also add a method like: addExtra(name, value) >>> >>> See WebHTTPBody for example. Maybe you want a WebIntentBuilder class? >> >> You mean like WebHTTPBody::append*? >> >> A Builder is an option, but it looks like create* or from* static methods are fairly common conventions in the API. Shall I rework these delivery constructors to that form? > > The reason for a builder interface is that it would permit you to avoid having > parameters of type WebVector. You could just have appendFoo methods. If that > is not appealing, then what you have here is fine. > > Also note that we don't generally export constructors: > http://trac.webkit.org/wiki/ChromiumWebKitAPI#Methods I slimmed this down and put initialization elsewhere. Just one delivery constructor now. I don't like appendFoo because I'd like to keep WebIntent immutable. (Though the semantics of ports makes that a trial.) >> Source/WebKit/chromium/public/WebIntent.h:106 >> + mutable WebMessagePortChannelArray* m_ownedChannels; > > it looks like m_ownedChannels does not survive a copy. consider > code like this: > > WebIntent a; > WebIntent b(..., ports, ...); > a = b; > > The "a" object would no longer retain ownership of ports. If b is > destroyed, then the ports will be deleted too. > > Is this what you intended? Keep in mind that the point of using > WebPrivatePtr<T> is to build data types that can be easily copied > around. Normally, the copy is not a lossy operation. Right. I have notes there to that effect, but dealing with ports is a pain. I could also just pass them separately to the deliverIntent call. Is that preferable than agonizing over them here? >> Source/WebKit/chromium/src/WebIntent.cpp:96 >> + extras.add(extrasNames[i], extrasValues[i]); > > indentation Fixed >> Source/WebKit/chromium/src/WebIntent.cpp:110 >> + WebSerializedScriptValue serializedData = WebSerializedScriptValue::serialize(blob->toV8Value()); > > please avoid so much code duplication. use a helper function. This is now consolidated.

Created attachment 144898 [details] Patch

Any more feedback on this, Darin? (In reply to comment #13) > Created an attachment (id=144898) [details] > Patch

Comment on attachment 144898 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=144898&action=review > Source/WebKit/chromium/public/WebIntent.h:54 > + WEBKIT_EXPORT WebIntent(const WebString& action, const WebString& type, const WebString& data, nit: Instead of exporting constructors, please add an exported initialize method. This is in the style guide. > Source/WebKit/chromium/public/WebIntent.h:95 > + mutable WebMessagePortChannelArray* m_ownedChannels; I don't think you should add this member variable. If you have a way to avoid it, that would be worth considering.

Comment on attachment 144898 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=144898&action=review >> Source/WebKit/chromium/public/WebIntent.h:54 >> + WEBKIT_EXPORT WebIntent(const WebString& action, const WebString& type, const WebString& data, > > nit: Instead of exporting constructors, please add an exported initialize method. This is in the style guide. Done. >> Source/WebKit/chromium/public/WebIntent.h:95 >> + mutable WebMessagePortChannelArray* m_ownedChannels; > > I don't think you should add this member variable. If you have a way to avoid it, that would be worth considering. Done. Moved ports to deliverIntent. This is a bit sad, but much better overall. I ended up aborting the approach we talked about separately. DeliveredIntent needs a Frame, and so can't really be wrapped, and writing a new struct seemed pointless. I think this API is fine.

Created attachment 145696 [details] Patch

Comment on attachment 145696 [details] Patch Clearing flags on attachment: 145696 Committed r119508: <http://trac.webkit.org/changeset/119508>

All reviewed patches have been landed. Closing bug.