Bug 73388

Created attachment 117102 [details] Patch

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.

Created attachment 117308 [details] Patch Following Simon's comment. Thanks Simon.

Created attachment 118006 [details] Patch v2 Updated from the internal fix.

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.

(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.

Created attachment 118302 [details] Patch v3

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?

Created attachment 118306 [details] Patch v4

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.

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

(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.

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

(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);

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.