Bug 73791 – Upstream platform/network/blackberry/DeferredData.{h, cpp}, NetworkJob.{h, cpp} and NetworkManager.{h, cpp}

• Status: RESOLVED
• Resolution: FIXED
• Component: Platform

[Leo Yang]

This is the main part of the BlackBerry networking porting.

[Leo Yang]

Created attachment 117823 [details]
Patch

[Rob Buis]

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

[Leo Yang]

Created attachment 118515 [details]
Patch v2

Following Rob's comments. Also fixing many style issues of code comments.

[Rob Buis]

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.

[Leo Yang]

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

[Leo Yang]

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

[Leo Yang]

Committed r102433: <http://trac.webkit.org/changeset/102433>