Bug 73791

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>