[webkit-reviews] review denied: [Bug 65209] Make MessagePort Transferable. : [Attachment 104212] Address comments; add a ScriptExecutionContext* argument to transferOut.

bugzilla-daemon at webkit.org bugzilla-daemon at webkit.org
Wed Aug 17 16:37:45 PDT 2011


Darin Fisher (:fishd, Google) <fishd at chromium.org> has denied Luke Zarko
<lukezarko+bugzilla at gmail.com>'s request for review:
Bug 65209: Make MessagePort Transferable.
https://bugs.webkit.org/show_bug.cgi?id=65209

Attachment 104212: Address comments; add a ScriptExecutionContext* argument to
transferOut.
https://bugs.webkit.org/attachment.cgi?id=104212&action=review

------- Additional Comments from Darin Fisher (:fishd, Google)
<fishd at chromium.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=104212&action=review


> Source/WebKit/chromium/public/WebMessagePortChannel.h:52
> +    virtual void postMessage(const WebString&, WebTransferableReceiptArray*)
= 0;

why is the second parameter non-const?	it seems like it should be passed as
|const Foo&| since you don't expect postMessage to mutate the array.

> Source/WebKit/chromium/public/WebMessagePortChannel.h:54
> +    virtual WebMessagePortChannel* asWebMessagePortChannel() { return this;
}

you may need to export this function since you are implementing it.  it may
need to be tagged with WEBKIT_EXPORT.  please make sure that the
component=shared_library build does not break.

> Source/WebKit/chromium/public/WebSerializedScriptValue.h:72
> +    WEBKIT_EXPORT static bool isFlattenable(WebTransferableReceipt*);

Why isn't this a member function of WebTransferableReceipt?  It seems like
something that should be a property.

> Source/WebKit/chromium/public/WebSerializedScriptValue.h:77
> +    WEBKIT_EXPORT static WebString flattenReceiptList(const WebString&,

this function is really quite confusing to me... reading the comment, it is
not so obvious what this function will do.

i thought after we chatted the other day that the plan was to do this final
flattening at the chromium level.  maybe i misunderstood something.

> Source/WebKit/chromium/public/WebTransferableReceipt.h:57
> +    virtual WebMessagePortChannel* asWebMessagePortChannel() { return 0; }

perhaps you should use the approach that we use for casting WebElement to
WebInputElement?
define a global function in WebMessagePortChannel.h:

  WEBKIT_EXPORT WebMessagePortChannel*
toWebMessagePortChannel(WebTransferableReceipt*);

we don't use the asFoo approach to RTTI anywhere else in the WebKit API, but we
do use
the toFoo approach.  seems like we should be consistent.

> Source/WebKit/chromium/public/WebTransferableReceipt.h:64
> +    static PassOwnPtr<WebCore::TransferableReceipt>
fromWeb(WebTransferableReceipt*);

Perhaps these would be better as global functions:

  PassOwnPtr<WebCore::TransferableReceipt>
toTransferableReceipt(WebTransferableReceipt*);

> Source/WebKit/chromium/src/WebSerializedScriptValue.cpp:78
> +    for (size_t i = 0; i < receipts.size(); ++i) {

it seems like this workload of determining the unflattenable count could be
done by the embedder too.

it is interesting that SerializedScriptValue::flattenReceiptList does not
similarly determine the unflattenable count.  this seems to indicate that
it should not be done by the WebKit API either.  complexity should either
be in WebCore or in Chromium, not in the WebKit "API glue" layer.

> Source/WebKit/chromium/src/WebSerializedScriptValue.cpp:93
> +    return WebCore::SerializedScriptValue::flattenReceiptList(message,
*coreReceipts);

no need for the WebCore:: prefix.


More information about the webkit-reviews mailing list