Bug 32963 – buildfix for ResourceHandleWin.cpp

• Status: RESOLVED
• Resolution: FIXED
• Component: WebCore Misc.

[Patrick R. Gansterer]

see patch

[Patrick R. Gansterer]

Created attachment 45525 [details]
Buildfix for ResourceHandleWin.cpp

transferJobStatusCallback and ResourceHandleWndProc are static.
Is there a reason for the declaration in the header file?

[WebKit Review Bot]

Attachment 45525 [details] did not pass style-queue:

Failed to run "WebKitTools/Scripts/check-webkit-style" exit_code: 1
WebCore/platform/network/win/ResourceHandleWin.cpp:38:  Found header this file implements after other header. Should be: config.h, primary header, blank line, and then alphabetically sorted.  [build/include_order] [4]
Total errors found: 1

[Patrick R. Gansterer]

In line 188 of ResourceHandleWin.cpp:
> const CString& headersLatin1 = headers.latin1();
> String formData = request().httpBody()->flattenToString();
> INTERNET_BUFFERSA buffers;
> memset(&buffers, 0, sizeof(buffers));
> buffers.dwStructSize = sizeof(INTERNET_BUFFERSA);
> buffers.lpcszHeader = headersLatin1.data();
> buffers.dwHeadersLength = headers.length();
> buffers.dwBufferTotal = formData.length();

- headers.latin1() returns a value and not a reference.
- buffers uses headersLatin1.data() but not headersLatin1.length().
- same problems across the file
Isn't that wrong?

[Patrick R. Gansterer]

(In reply to comment #2)
> WebCore/platform/network/win/ResourceHandleWin.cpp:38:  Found header this file
> implements after other header. Should be: config.h, primary header, blank line,
> and then alphabetically sorted.  [build/include_order] [4]
The primary header header is ResourceHandle.h. ResourceHandleWin.h is only an ugly helperfile.

[Patrick R. Gansterer]

Created attachment 45527 [details]
Buildfix for ResourceHandleWin.cpp

corrected bug url

[WebKit Review Bot]

Attachment 45527 [details] did not pass style-queue:

Failed to run "WebKitTools/Scripts/check-webkit-style" exit_code: 1
WebCore/platform/network/win/ResourceHandleWin.cpp:38:  Found header this file implements after other header. Should be: config.h, primary header, blank line, and then alphabetically sorted.  [build/include_order] [4]
Total errors found: 1

[Adam Barth]

> The primary header header is ResourceHandle.h. ResourceHandleWin.h is only an
> ugly helperfile.

False positive filed: https://bugs.webkit.org/show_bug.cgi?id=32975

[Eric Seidel (no email)]

Comment on attachment 45527 [details]
Buildfix for ResourceHandleWin.cpp

Please explain your changes in the ChangeLog.  There are too many, too complicated changes for this to be reviewed with such an empty ChangeLog.

For example, why this?
-        if (method() == "POST") {
+        if (request().httpMethod() == "POST") {

And this?
+#ifdef RESOURCE_LOADER_DEBUG
+        char buf[64];
+        _snprintf(buf, sizeof(buf), "Load error: %i\n", error);
+        OutputDebugStringA(buf);
+#endif

[Patrick R. Gansterer]

(In reply to comment #8)
> (From update of attachment 45527 [details])
> Please explain your changes in the ChangeLog.  There are too many, too
> complicated changes for this to be reviewed with such an empty ChangeLog.
This is _only_ a buildfix!

> For example, why this?
> -        if (method() == "POST") {
> +        if (request().httpMethod() == "POST") {
because http://trac.webkit.org/changeset/24202 removed the url(), postData() and method() meberfunctions ;-)

> And this?
> +#ifdef RESOURCE_LOADER_DEBUG
> +        char buf[64];
> +        _snprintf(buf, sizeof(buf), "Load error: %i\n", error);
> +        OutputDebugStringA(buf);
> +#endif
_RPTF1 also outputs a debug string and isn't supported on WinCE

Maybe i can split it into a a windows and a wince part, but i don't think that it make sense, because it doesn't work since r24204 and seams to be unuse. Real enhancements will follow in a separate bug.

I think that the points mentioned in comment #3 are also reasons for r-, but i like to get some howto feedback before i change it.

[Eric Seidel (no email)]

(In reply to comment #9)
> (In reply to comment #8)
> > (From update of attachment 45527 [details] [details])
> > Please explain your changes in the ChangeLog.  There are too many, too
> > complicated changes for this to be reviewed with such an empty ChangeLog.
> This is _only_ a buildfix!

I believe you. :)  I just had trouble understanding the patch.  It's difficult to tell if this change is only inside WINCE blocks.  I take it this code is not used by the AppleWin port.  Is it used by the ChromiumWin port, or just the WinCE port?  What about the CairoWin port?

See http://trac.webkit.org/changeset/43259 as an example of explaining file-level changes in ChangeLogs.  The goal of the ChangeLog (in my mind) is to document what the change is doing to make the review easier, and to make any later "svn blame" investigation easier.
http://webkit.org/coding/contributing.html#changelogs has more thoughts on ChangeLogs and their purpose.

In this case, I just don't have enough context to understand the change, but it's possible that another reviewer would.

[Patrick R. Gansterer]

(In reply to comment #10)
> I believe you. :)  I just had trouble understanding the patch.  It's difficult
> to tell if this change is only inside WINCE blocks.  I take it this code is not
> used by the AppleWin port.  Is it used by the ChromiumWin port, or just the
> WinCE port?  What about the CairoWin port?
As already mentioned http://trac.webkit.org/changeset/24202 made it fail! Everywhere!

@initializeOffScreenResourceH: as in attachment 45502 [details]
StringImpl constructor is now private
_RPTF1 had been already mentioned
in the remaining part in start() only the intention changed (+ .data() at strings like above)

What about the following points of comment #3?
- headers.latin1() returns a value and not a reference.
- buffers uses headersLatin1.data() but not headersLatin1.length().

[Patrick R. Gansterer]

Comment on attachment 45527 [details]
Buildfix for ResourceHandleWin.cpp

Setting back to r? because i didn't get a answer since last year.

[Darin Adler]

Comment on attachment 45527 [details]
Buildfix for ResourceHandleWin.cpp

I am completely confused by this patch. This claims to "fix the build everywhere" that was broken three years ago. And it's a big change.

Could you clarify what build is broken? And can we keep the build fix change as small as possible and leave out other types of style changes?

It may well be that all these changes are justified, but mixing the style cleanup, refactoring, and build fix all in one patch leads to a patch that probably won't get reviewed.

[Patrick R. Gansterer]

Created attachment 52961 [details]
The patch (only the buildfix for Win32)

(In reply to comment #13)
> Could you clarify what build is broken?
It seams nobody is using this file in the moment, but i will be used in the WinCE port.

I will post a second patch for the style issues and a third for the WinCE specific part.

[WebKit Review Bot]

Attachment 52961 [details] did not pass style-queue:

Failed to run "WebKitTools/Scripts/check-webkit-style" exit_code: 1
WebCore/platform/network/win/ResourceHandleWin.cpp:334:  An else statement can be removed when the prior "if" concludes with a return, break, continue or goto statement.  [readability/control_flow] [4]
Total errors found: 1 in 2 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Patrick R. Gansterer]

Created attachment 52965 [details]
The patch (buildfix for Win32 and r55542)

This patch also fixes the build after http://trac.webkit.org/changeset/55542.

[Patrick R. Gansterer]

Comment on attachment 52961 [details]
The patch (only the buildfix for Win32)

The second patch is better.

[WebKit Review Bot]

Attachment 52965 [details] did not pass style-queue:

Failed to run "WebKitTools/Scripts/check-webkit-style" exit_code: 1
WebCore/platform/network/win/ResourceHandleWin.cpp:335:  An else statement can be removed when the prior "if" concludes with a return, break, continue or goto statement.  [readability/control_flow] [4]
Total errors found: 1 in 2 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Eric Seidel (no email)]

Comment on attachment 52965 [details]
The patch (buildfix for Win32 and r55542)

if this is only used by WinCE, seems we should rename the file.

[Patrick R. Gansterer]

(In reply to comment #19)
> (From update of attachment 52965 [details])
> if this is only used by WinCE, seems we should rename the file.
I don't think so. This file is not dedicaed to the WinCE platform. It is also possible to use it on Win32.
WinCE port uses many other files too, which end with "Win.*".

My vision is that the WinCE port will be only some #ifdefs in a GDI port.

[David Levin]

Comment on attachment 52965 [details]
The patch (buildfix for Win32 and r55542)

Surprising how little this file is maintained.

But the patch looks fine.

[WebKit Commit Bot]

Comment on attachment 52965 [details]
The patch (buildfix for Win32 and r55542)

Clearing flags on attachment: 52965

Committed r61183: <http://trac.webkit.org/changeset/61183>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.