Bug 73388 - Upstream the BlackBerry porting of network Resource
Summary: Upstream the BlackBerry porting of network Resource
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Platform (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Leo Yang
URL:
Keywords:
Depends on:
Blocks: 73144
  Show dependency treegraph
 
Reported: 2011-11-29 19:48 PST by Leo Yang
Modified: 2011-12-08 18:57 PST (History)
5 users (show)

See Also:


Attachments
Patch (28.18 KB, patch)
2011-11-29 20:15 PST, Leo Yang
no flags Details | Formatted Diff | Diff
Patch (28.21 KB, patch)
2011-11-30 18:47 PST, Leo Yang
no flags Details | Formatted Diff | Diff
Patch v2 (28.41 KB, patch)
2011-12-06 01:21 PST, Leo Yang
rwlbuis: review-
Details | Formatted Diff | Diff
Patch v3 (28.32 KB, patch)
2011-12-07 18:26 PST, Leo Yang
rwlbuis: review-
Details | Formatted Diff | Diff
Patch v4 (28.31 KB, patch)
2011-12-07 18:48 PST, Leo Yang
rwlbuis: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Leo Yang 2011-11-29 19:48:00 PST
This is to upstream the BlackBerry implement of network resource.
Comment 1 Leo Yang 2011-11-29 20:15:24 PST
Created attachment 117102 [details]
Patch
Comment 2 Simon Hausmann 2011-11-30 13:27:56 PST
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.
Comment 3 Leo Yang 2011-11-30 18:47:14 PST
Created attachment 117308 [details]
Patch

Following Simon's comment. Thanks Simon.
Comment 4 Leo Yang 2011-12-06 01:21:03 PST
Created attachment 118006 [details]
Patch v2

Updated from the internal fix.
Comment 5 Rob Buis 2011-12-07 15:25:19 PST
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.
Comment 6 Leo Yang 2011-12-07 17:45:59 PST
(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.
Comment 7 Leo Yang 2011-12-07 18:26:24 PST
Created attachment 118302 [details]
Patch v3
Comment 8 Rob Buis 2011-12-07 18:35:24 PST
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?
Comment 9 Leo Yang 2011-12-07 18:48:36 PST
Created attachment 118306 [details]
Patch v4
Comment 10 Rob Buis 2011-12-07 18:58:54 PST
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.
Comment 11 Rob Buis 2011-12-07 19:02:51 PST
Typo, I meant of course "you *can* fix them before landing".
Comment 12 Leo Yang 2011-12-07 19:09:00 PST
(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.
Comment 13 Leo Yang 2011-12-07 19:38:47 PST
Committed r102303: <http://trac.webkit.org/changeset/102303>
Comment 14 Nikolas Zimmermann 2011-12-08 00:36:35 PST
(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);
Comment 15 Rob Buis 2011-12-08 18:57:36 PST
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.