Bug 32963

Summary: buildfix for ResourceHandleWin.cpp
Product: WebKit Reporter: Patrick R. Gansterer <paroga>
Component: WebCore Misc.Assignee: Nobody <webkit-unassigned>
Status: RESOLVED FIXED    
Severity: Normal CC: abarth, aroben, beidson, commit-queue, eric, fishd, mjs, webkit.review.bot
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: PC   
OS: All   
Attachments:
Description Flags
Buildfix for ResourceHandleWin.cpp
none
Buildfix for ResourceHandleWin.cpp
none
The patch (only the buildfix for Win32)
none
The patch (buildfix for Win32 and r55542) none

Description Patrick R. Gansterer 2009-12-27 10:16:34 PST
see patch
Comment 1 Patrick R. Gansterer 2009-12-27 10:36:01 PST
Created attachment 45525 [details]
Buildfix for ResourceHandleWin.cpp

transferJobStatusCallback and ResourceHandleWndProc are static.
Is there a reason for the declaration in the header file?
Comment 2 WebKit Review Bot 2009-12-27 10:37:59 PST
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
Comment 3 Patrick R. Gansterer 2009-12-27 10:44:59 PST
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?
Comment 4 Patrick R. Gansterer 2009-12-27 10:48:12 PST
(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.
Comment 5 Patrick R. Gansterer 2009-12-27 11:02:05 PST
Created attachment 45527 [details]
Buildfix for ResourceHandleWin.cpp

corrected bug url
Comment 6 WebKit Review Bot 2009-12-27 11:03:57 PST
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
Comment 7 Adam Barth 2009-12-27 18:10:35 PST
> 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
Comment 8 Eric Seidel (no email) 2009-12-29 00:14:54 PST
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
Comment 9 Patrick R. Gansterer 2009-12-29 08:43:38 PST
(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.
Comment 10 Eric Seidel (no email) 2009-12-29 09:17:45 PST
(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.
Comment 11 Patrick R. Gansterer 2009-12-29 23:21:05 PST
(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().
Comment 12 Patrick R. Gansterer 2010-04-08 10:15:12 PDT
Comment on attachment 45527 [details]
Buildfix for ResourceHandleWin.cpp

Setting back to r? because i didn't get a answer since last year.
Comment 13 Darin Adler 2010-04-09 09:18:59 PDT
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.
Comment 14 Patrick R. Gansterer 2010-04-09 09:54:36 PDT
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.
Comment 15 WebKit Review Bot 2010-04-09 09:59:36 PDT
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.
Comment 16 Patrick R. Gansterer 2010-04-09 10:29:14 PDT
Created attachment 52965 [details]
The patch (buildfix for Win32 and r55542)

This patch also fixes the build after http://trac.webkit.org/changeset/55542.
Comment 17 Patrick R. Gansterer 2010-04-09 10:29:56 PDT
Comment on attachment 52961 [details]
The patch (only the buildfix for Win32)

The second patch is better.
Comment 18 WebKit Review Bot 2010-04-09 10:33:16 PDT
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.
Comment 19 Eric Seidel (no email) 2010-04-09 13:26:08 PDT
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.
Comment 20 Patrick R. Gansterer 2010-04-10 01:19:43 PDT
(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.
Comment 21 David Levin 2010-06-14 11:44:19 PDT
Comment on attachment 52965 [details]
The patch (buildfix for Win32 and r55542)

Surprising how little this file is maintained.

But the patch looks fine.
Comment 22 WebKit Commit Bot 2010-06-15 06:09:04 PDT
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>
Comment 23 WebKit Commit Bot 2010-06-15 06:09:14 PDT
All reviewed patches have been landed.  Closing bug.