Summary: | Upstream platform/network/blackberry/DeferredData.{h, cpp}, NetworkJob.{h, cpp} and NetworkManager.{h, cpp} | ||||||||
---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Leo Yang <leo.yang> | ||||||
Component: | Platform | Assignee: | Leo Yang <leo.yang> | ||||||
Status: | RESOLVED FIXED | ||||||||
Severity: | Normal | CC: | charles.wei, dbates, rwlbuis, staikos, zimmermann | ||||||
Priority: | P2 | ||||||||
Version: | 528+ (Nightly build) | ||||||||
Hardware: | Unspecified | ||||||||
OS: | Unspecified | ||||||||
Bug Depends on: | |||||||||
Bug Blocks: | 73144, 73869 | ||||||||
Attachments: |
|
Description
Leo Yang
2011-12-04 18:38:37 PST
Created attachment 117823 [details]
Patch
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 Created attachment 118515 [details]
Patch v2
Following Rob's comments. Also fixing many style issues of code comments.
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. (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. (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. Committed r102433: <http://trac.webkit.org/changeset/102433> |