[webkit-reviews] review denied: [Bug 87143] New constructor for WebIntent to be used for delivery : [Attachment 143858] Patch

[bugzilla-daemon at webkit.org]

Darin Fisher (:fishd, Google) <fishd at chromium.org> has denied Greg Billock
<gbillock at google.com>'s request for review:
Bug 87143: New constructor for WebIntent to be used for delivery
https://bugs.webkit.org/show_bug.cgi?id=87143

Attachment 143858: Patch
https://bugs.webkit.org/attachment.cgi?id=143858&action=review

------- Additional Comments from Darin Fisher (:fishd, Google)
<fishd at chromium.org>
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.