Bug 87143 - New constructor for WebIntent to be used for delivery
Summary: New constructor for WebIntent to be used for delivery
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Greg Billock
URL:
Keywords:
Depends on:
Blocks: 75123
  Show dependency treegraph
 
Reported: 2012-05-22 10:14 PDT by Greg Billock
Modified: 2012-06-05 11:40 PDT (History)
8 users (show)

See Also:


Attachments
Patch (8.20 KB, patch)
2012-05-22 10:15 PDT, Greg Billock
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ec2-cr-linux-02 (459.38 KB, application/zip)
2012-05-22 12:13 PDT, WebKit Review Bot
no flags Details
Patch (11.16 KB, patch)
2012-05-23 09:42 PDT, Greg Billock
no flags Details | Formatted Diff | Diff
Patch (11.50 KB, patch)
2012-05-24 11:17 PDT, Greg Billock
no flags Details | Formatted Diff | Diff
Patch (10.10 KB, patch)
2012-05-30 12:37 PDT, Greg Billock
no flags Details | Formatted Diff | Diff
Patch (10.81 KB, patch)
2012-06-04 22:17 PDT, Greg Billock
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Greg Billock 2012-05-22 10:14:13 PDT
New constructor for WebIntent to be used for delivery
Comment 1 Greg Billock 2012-05-22 10:15:59 PDT
Created attachment 143322 [details]
Patch
Comment 2 WebKit Review Bot 2012-05-22 10:18:04 PDT
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 3 WebKit Review Bot 2012-05-22 12:13:47 PDT
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
Comment 4 WebKit Review Bot 2012-05-22 12:13:52 PDT
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
Comment 5 Greg Billock 2012-05-23 09:42:05 PDT
Created attachment 143583 [details]
Patch
Comment 6 Chris Dumez 2012-05-23 10:08:44 PDT
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 7 Greg Billock 2012-05-24 09:23:03 PDT
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.
Comment 8 Greg Billock 2012-05-24 11:17:03 PDT
Created attachment 143858 [details]
Patch
Comment 9 Darin Fisher (:fishd, Google) 2012-05-24 13:45:08 PDT
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 10 Greg Billock 2012-05-25 08:14:10 PDT
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 11 Darin Fisher (:fishd, Google) 2012-05-29 15:17:15 PDT
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 12 Greg Billock 2012-05-30 12:35:49 PDT
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.
Comment 13 Greg Billock 2012-05-30 12:37:32 PDT
Created attachment 144898 [details]
Patch
Comment 14 Greg Billock 2012-06-01 12:03:56 PDT
Any more feedback on this, Darin?

(In reply to comment #13)
> Created an attachment (id=144898) [details]
> Patch
Comment 15 Darin Fisher (:fishd, Google) 2012-06-04 16:18:13 PDT
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 16 Greg Billock 2012-06-04 22:16:40 PDT
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.
Comment 17 Greg Billock 2012-06-04 22:17:26 PDT
Created attachment 145696 [details]
Patch
Comment 18 WebKit Review Bot 2012-06-05 11:39:54 PDT
Comment on attachment 145696 [details]
Patch

Clearing flags on attachment: 145696

Committed r119508: <http://trac.webkit.org/changeset/119508>
Comment 19 WebKit Review Bot 2012-06-05 11:40:03 PDT
All reviewed patches have been landed.  Closing bug.