Bug 73388 – Upstream the BlackBerry porting of network Resource

• Component: Platform
• Status: RESOLVED FIXED

[Leo Yang]

This is to upstream the BlackBerry implement of network resource.

[Leo Yang]

Created attachment 117102 [details]
Patch

[Simon Hausmann]

Comment on attachment 117102 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=117102&action=review

> Source/WebCore/platform/network/blackberry/ResourceErrorBlackBerry.cpp:25
> +const char* ResourceError::platformErrorDomain = "BlackBerryPlatform";

These should probably be const char* const to make the actual pointer const, too, and avoid unnecessary relocations.

[Leo Yang]

Created attachment 117308 [details]
Patch

Following Simon's comment. Thanks Simon.

[Leo Yang]

Created attachment 118006 [details]
Patch v2

Updated from the internal fix.

[Rob Buis]

Comment on attachment 118006 [details]
Patch v2

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.

[Leo Yang]

(In reply to comment #5)
> (From update of attachment 118006 [details])
> 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?
> 
Will remove the comment.

> > Source/WebCore/platform/network/blackberry/ResourceHandleBlackBerry.cpp:148
> > +        ASSERT(false && "loadResourceSynchronously called with invalid networking context");
> 
> Is this correct? Seems uncommon.
Yes. Actually it's same as ASSERT(false) except that it will print the useful information when the ASSERT reaches.

> 
> > Source/WebCore/platform/network/blackberry/ResourceHandleBlackBerry.cpp:155
> > +        ASSERT(false && "loadResourceSynchronously called without a frame or frame client");
> 
> Ditto.
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.
OK.

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

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

> 
> > Source/WebCore/platform/network/blackberry/ResourceRequestBlackBerry.cpp:123
> > +        {
> 
> Brace should go on same line as the for loop.
OK.

[Leo Yang]

Created attachment 118302 [details]
Patch v3

[Rob Buis]

Comment on attachment 118302 [details]
Patch v3

View in context: https://bugs.webkit.org/attachment.cgi?id=118302&action=review

Some nits.... Mainly empty space related.

> Source/WebCore/platform/network/blackberry/ResourceError.h:28
> +

Why empty line?

> Source/WebCore/platform/network/blackberry/ResourceHandleBlackBerry.cpp:190
> +WTF::PassRefPtr<SharedBuffer> ResourceHandle::bufferedData()

Is WTF needed?

> Source/WebCore/platform/network/blackberry/ResourceRequest.h:113
> +

Why empty line?

> Source/WebCore/platform/network/blackberry/ResourceResponse.h:47
> +    bool isMultipartPayload() const { return m_isMultipartPayload; }

Why not an empty line here?

> Source/WebCore/platform/network/blackberry/ResourceResponse.h:51
> +

Why empty line?

[Leo Yang]

Created attachment 118306 [details]
Patch v4

[Rob Buis]

Comment on attachment 118306 [details]
Patch v4

View in context: https://bugs.webkit.org/attachment.cgi?id=118306&action=review

Some nits, but you cant fix them before landing. r+

> Source/WebCore/platform/network/blackberry/ResourceHandleBlackBerry.cpp:46
> +    virtual void didReceiveData(ResourceHandle*, const char*, int, int lengthReceived);

Didn't see this before, but lengthReceived is not needed.

> Source/WebCore/platform/network/blackberry/ResourceHandleBlackBerry.cpp:172
> +    static const double s_syncLoadTimeOut = 60.0; // seconds

60.0 -> 60.
Also s_syncLoadTimeOut is a bit weird. It looks like a global constant but is just used in this function. Should it use DEFINE_STATIC_LOCAL? Maybe look at similar code like this in WebCore/, but no blocker.

[Rob Buis]

Typo, I meant of course "you *can* fix them before landing".

[Leo Yang]

(In reply to comment #10)
> (From update of attachment 118306 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=118306&action=review
> 
> Some nits, but you cant fix them before landing. r+
> 
> > Source/WebCore/platform/network/blackberry/ResourceHandleBlackBerry.cpp:46
> > +    virtual void didReceiveData(ResourceHandle*, const char*, int, int lengthReceived);
> 
> Didn't see this before, but lengthReceived is not needed.
Will fix before landing.

> 
> > Source/WebCore/platform/network/blackberry/ResourceHandleBlackBerry.cpp:172
> > +    static const double s_syncLoadTimeOut = 60.0; // seconds
> 
> 60.0 -> 60.
> Also s_syncLoadTimeOut is a bit weird. It looks like a global constant but is just used in this function. Should it use DEFINE_STATIC_LOCAL? Maybe look at similar code like this in WebCore/, but no blocker.
Actually static is not needed, will remove static and change 60.0 to 60 before landing. 

Thanks for your review.

[Leo Yang]

Committed r102303: <http://trac.webkit.org/changeset/102303>

[Nikolas Zimmermann]

(In reply to comment #12)
> (In reply to comment #10)
> > (From update of attachment 118306 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=118306&action=review
> > 
> > Some nits, but you cant fix them before landing. r+
> > 
> > > Source/WebCore/platform/network/blackberry/ResourceHandleBlackBerry.cpp:46
> > > +    virtual void didReceiveData(ResourceHandle*, const char*, int, int lengthReceived);
> > 
> > Didn't see this before, but lengthReceived is not needed.
> Will fix before landing.

Just for the record: this is a place where names are actually helpful for _both_ integers. Can you tell what they would mean w/o looking at the cpp file? Same for the const char*.

We only leave it out, if it doesn't help. eg. void setFontStyle(FontStyle);

[Rob Buis]

Hi Niko,

(In reply to comment #14)
> (In reply to comment #12)
> > (In reply to comment #10)
> > > (From update of attachment 118306 [details] [details] [details])
> > > View in context: https://bugs.webkit.org/attachment.cgi?id=118306&action=review
> > > 
> > > Some nits, but you cant fix them before landing. r+
> > > 
> > > > Source/WebCore/platform/network/blackberry/ResourceHandleBlackBerry.cpp:46
> > > > +    virtual void didReceiveData(ResourceHandle*, const char*, int, int lengthReceived);
> > > 
> > > Didn't see this before, but lengthReceived is not needed.
> > Will fix before landing.
> 
> Just for the record: this is a place where names are actually helpful for _both_ integers. Can you tell what they would mean w/o looking at the cpp file? Same for the const char*.
> 
> We only leave it out, if it doesn't help. eg. void setFontStyle(FontStyle);

Good point! Will be more careful in future.
Cheers,

Rob.