Bug 73791 - Upstream platform/network/blackberry/DeferredData.{h, cpp}, NetworkJob.{h, cpp} and NetworkManager.{h, cpp}
Summary: Upstream platform/network/blackberry/DeferredData.{h, cpp}, NetworkJob.{h, cp...
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 73869
  Show dependency treegraph
 
Reported: 2011-12-04 18:38 PST by Leo Yang
Modified: 2011-12-08 22:44 PST (History)
5 users (show)

See Also:


Attachments
Patch (67.74 KB, patch)
2011-12-04 18:50 PST, Leo Yang
rwlbuis: review-
Details | Formatted Diff | Diff
Patch v2 (61.18 KB, patch)
2011-12-08 19:15 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-12-04 18:38:37 PST
This is the main part of the BlackBerry networking porting.
Comment 1 Leo Yang 2011-12-04 18:50:10 PST
Created attachment 117823 [details]
Patch
Comment 2 Rob Buis 2011-12-08 07:48:01 PST
Comment on attachment 117823 [details]
Patch

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

Still some stuff to fix.

> Source/WebCore/platform/network/blackberry/DeferredData.cpp:40
> +void DeferredData::deferOpen(int status, const WTF::String& message)

Do we need the WTF:: prefix?

> Source/WebCore/platform/network/blackberry/NetworkJob.cpp:69
> +}

I wonder if we can use toASCIIHexValue from wtf/ASCIICType.h?

> Source/WebCore/platform/network/blackberry/NetworkManager.cpp:117
> +#if OS(QNX)

Can be removed
Comment 3 Leo Yang 2011-12-08 19:15:52 PST
Created attachment 118515 [details]
Patch v2

Following Rob's comments. Also fixing many style issues of code comments.
Comment 4 Rob Buis 2011-12-08 19:34:04 PST
Comment on attachment 118515 [details]
Patch v2

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

Assuming toASCIIHexValue usage was tested, r+.

> Source/WebCore/platform/network/blackberry/DeferredData.h:34
> +    RecursionGuard(bool &guard)

bool& . Please run check-webkit-style :)

> Source/WebCore/platform/network/blackberry/NetworkJob.h:144
> +    }

Should be moved above the private: to join the other private methods.

> Source/WebCore/platform/network/blackberry/NetworkJob.h:179
> +    bool m_needsRetryAsFTPDirectory;

Note: you can pack the bools together if you want to save some memory.
Comment 5 Leo Yang 2011-12-08 19:41:06 PST
(In reply to comment #4)
> (From update of attachment 118515 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=118515&action=review
> 
> Assuming toASCIIHexValue usage was tested, r+.
> 
> > Source/WebCore/platform/network/blackberry/DeferredData.h:34
> > +    RecursionGuard(bool &guard)
> 
> bool& . Please run check-webkit-style :)
Good catch. I did run, but check-webkit-style didn't catch this. Will fix before landing.

> 
> > Source/WebCore/platform/network/blackberry/NetworkJob.h:144
> > +    }
> 
> Should be moved above the private: to join the other private methods.
Good catch.

> 
> > Source/WebCore/platform/network/blackberry/NetworkJob.h:179
> > +    bool m_needsRetryAsFTPDirectory;
> 
> Note: you can pack the bools together if you want to save some memory.
Hmmm, yes. But it hurts perf a bit. I wanted to do the same thing in other place, seems GS didn't like packing.

Thanks for review.
Comment 6 Leo Yang 2011-12-08 19:42:32 PST
(In reply to comment #5)
> (In reply to comment #4)
> > (From update of attachment 118515 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=118515&action=review
> > 
> > Assuming toASCIIHexValue usage was tested, r+.
> > 
> > > Source/WebCore/platform/network/blackberry/DeferredData.h:34
> > > +    RecursionGuard(bool &guard)
> > 
> > bool& . Please run check-webkit-style :)
> Good catch. I did run, but check-webkit-style didn't catch this. Will fix before landing.
> 
> > 
> > > Source/WebCore/platform/network/blackberry/NetworkJob.h:144
> > > +    }
> > 
> > Should be moved above the private: to join the other private methods.
> Good catch.
> 
> > 
> > > Source/WebCore/platform/network/blackberry/NetworkJob.h:179
> > > +    bool m_needsRetryAsFTPDirectory;
> > 
> > Note: you can pack the bools together if you want to save some memory.
> Hmmm, yes. But it hurts perf a bit. I wanted to do the same thing in other place, seems GS didn't like packing.
Oh, sorry. I misunderstood what you meant. Yes, I will move them above.
> 
> Thanks for review.
Comment 7 Leo Yang 2011-12-08 22:44:03 PST
Committed r102433: <http://trac.webkit.org/changeset/102433>