[webkit-reviews] review denied: [Bug 73388] Upstream the BlackBerry porting of network Resource : [Attachment 118006] Patch v2

[bugzilla-daemon at webkit.org]

Rob Buis <rwlbuis at gmail.com> has denied Leo Yang
<leo.yang at torchmobile.com.cn>'s request for review:
Bug 73388: Upstream the BlackBerry porting of network Resource
https://bugs.webkit.org/show_bug.cgi?id=73388

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

------- Additional Comments from Rob Buis <rwlbuis at gmail.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=118006&action=review


Looking quite good, can be improved a bit, so r- for now.

> Source/WebCore/platform/network/blackberry/ResourceHandleBlackBerry.cpp:47
> +    virtual void didFinishLoading(ResourceHandle*, double /*finishTime*/);

Why put finishTime in a comment?

> Source/WebCore/platform/network/blackberry/ResourceHandleBlackBerry.cpp:148
> +	   ASSERT(false && "loadResourceSynchronously called with invalid
networking context");

Is this correct? Seems uncommon.

> Source/WebCore/platform/network/blackberry/ResourceHandleBlackBerry.cpp:155
> +	   ASSERT(false && "loadResourceSynchronously called without a frame or
frame client");

Ditto.

> Source/WebCore/platform/network/blackberry/ResourceRequest.h:89
> +    // marks requests which must be handled by webkit, even if
LinksHandledExternally is set

Make this a sentence.

> Source/WebCore/platform/network/blackberry/ResourceRequestBlackBerry.cpp:88
> +    // if this is the initial load, skip the request body and headers

Ditto.

> Source/WebCore/platform/network/blackberry/ResourceRequestBlackBerry.cpp:100
> +	       // use setData for simple forms because it is slightly more
efficient

Ditto.

> Source/WebCore/platform/network/blackberry/ResourceRequestBlackBerry.cpp:123
> +	   {

Brace should go on same line as the for loop.