Bug 46200 - Cleanup network code in ResourceHandleWin
Summary: Cleanup network code in ResourceHandleWin
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Platform (show other bugs)
Version: 528+ (Nightly build)
Hardware: PC Windows XP
: P2 Normal
Assignee: Patrick R. Gansterer
URL:
Keywords:
Depends on:
Blocks: 43712
  Show dependency treegraph
 
Reported: 2010-09-21 10:32 PDT by Patrick R. Gansterer
Modified: 2010-09-24 10:26 PDT (History)
2 users (show)

See Also:


Attachments
Patch (13.33 KB, patch)
2010-09-21 10:52 PDT, Patrick R. Gansterer
no flags Details | Formatted Diff | Diff
Patch (subset of first patch) (8.64 KB, patch)
2010-09-21 13:42 PDT, Patrick R. Gansterer
aroben: review+
aroben: commit-queue-
Details | Formatted Diff | Diff
Patch (subset of first patch) (8.01 KB, patch)
2010-09-23 07:42 PDT, Patrick R. Gansterer
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Patrick R. Gansterer 2010-09-21 10:32:57 PDT
see patch
Comment 1 Patrick R. Gansterer 2010-09-21 10:52:07 PDT
Created attachment 68260 [details]
Patch
Comment 2 Adam Roben (:aroben) 2010-09-21 12:39:17 PDT
Comment on attachment 68260 [details]
Patch

I'm finding this patch hard to review, which I think is a sign that this patch is trying to do too much at once. Is there a way to break it up into smaller pieces?
Comment 3 Patrick R. Gansterer 2010-09-21 13:42:39 PDT
Created attachment 68285 [details]
Patch (subset of first patch)
Comment 4 Adam Roben (:aroben) 2010-09-23 06:53:39 PDT
Comment on attachment 68285 [details]
Patch (subset of first patch)

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

> WebCore/platform/network/win/ResourceHandleWin.cpp:405
> +    DWORD flags =
> +        INTERNET_FLAG_KEEP_CONNECTION |
> +        INTERNET_FLAG_IGNORE_REDIRECT_TO_HTTPS |
> +        INTERNET_FLAG_IGNORE_REDIRECT_TO_HTTP;

Operators should be at the start of each line, not the end.

> WebCore/platform/network/win/ResourceHandleWin.cpp:414
> +    d->m_connectHandle = InternetConnectW(d->m_internetHandle,
> +                                          firstRequest().url().host().charactersWithNullTermination(),
> +                                          firstRequest().url().port(),
> +                                          0, // no username
> +                                          0, // no password
> +                                          INTERNET_SERVICE_HTTP,
> +                                          flags,
> +                                          reinterpret_cast<DWORD_PTR>(this));

We don't normally wrap and line up parameters like this. (Same comment applies elsewhere in this patch.)

> WebCore/platform/network/win/ResourceHandleWin.cpp:444
> +    if (!d->m_requestHandle) {
> +        InternetCloseHandle(d->m_connectHandle);
> +        return false;
> +    }

Do we need to null out d->m_connectHandle here, too?

> WebCore/platform/network/win/ResourceHandleWin.cpp:448
> +    INTERNET_BUFFERSW internetBuffers;
> +    ZeroMemory(&internetBuffers, sizeof(INTERNET_BUFFERSW));
> +    internetBuffers.dwStructSize = sizeof(INTERNET_BUFFERSW);

If you use sizeof(internetBuffers) instead, you won't have to change three lines if the type changes in the future.
Comment 5 Patrick R. Gansterer 2010-09-23 07:42:52 PDT
Created attachment 68516 [details]
Patch (subset of first patch)

> > WebCore/platform/network/win/ResourceHandleWin.cpp:444
> > +    if (!d->m_requestHandle) {
> > +        InternetCloseHandle(d->m_connectHandle);
> > +        return false;
> > +    }
> 
> Do we need to null out d->m_connectHandle here, too?
No, no need, because nothing will happen after the "return false".
Comment 6 WebKit Commit Bot 2010-09-23 09:13:27 PDT
Comment on attachment 68516 [details]
Patch (subset of first patch)

Clearing flags on attachment: 68516

Committed r68147: <http://trac.webkit.org/changeset/68147>
Comment 7 Patrick R. Gansterer 2010-09-24 10:26:37 PDT
All reviewed patches have been landed.  Closing bug.