Bug 134209 – [Win] Clean up and refactor WinLauncher

• Status: RESOLVED
• Resolution: FIXED
• Component: Tools / Tests

[Brent Fulgham]

WinLauncher is a bit of a mess, with tons of global variables and potentially buggy memory handling. Clean this up to take advantage of our various smart pointers, and refactor into a class that can be subclassed for use in other tools.

[Brent Fulgham]

Created attachment 233657 [details]
Patch

[WebKit Commit Bot]

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


ERROR: Tools/WinLauncher/WinLauncherWebHost.h:34:  This { should be at the end of the previous line  [whitespace/braces] [4]
ERROR: Tools/WinLauncher/WinLauncherWebHost.cpp:29:  Found other header before WebCore config.h. Should be: config.h, primary header, blank line, and then alphabetically sorted.  [build/include_order] [4]
ERROR: Tools/WinLauncher/WinLauncherWebHost.cpp:30:  Found header this file implements after other header. Should be: config.h, primary header, blank line, and then alphabetically sorted.  [build/include_order] [4]
ERROR: Tools/WinLauncher/Common.cpp:425:  Multi line control clauses should use braces.  [whitespace/braces] [4]
ERROR: Tools/WinLauncher/WinMain.cpp:29:  Alphabetical sorting problem.  [build/include_order] [4]
ERROR: Tools/WinLauncher/DOMDefaultImpl.cpp:28:  Found other header before WebCore config.h. Should be: config.h, primary header, blank line, and then alphabetically sorted.  [build/include_order] [4]
ERROR: Tools/WinLauncher/DOMDefaultImpl.cpp:29:  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: 7 in 10 files


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

[Brent Fulgham]

Created attachment 233659 [details]
Patch

[WebKit Commit Bot]

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


ERROR: Tools/WinLauncher/WinLauncherWebHost.cpp:29:  Found other header before WebCore config.h. Should be: config.h, primary header, blank line, and then alphabetically sorted.  [build/include_order] [4]
ERROR: Tools/WinLauncher/WinLauncherWebHost.cpp:30:  Found header this file implements after other header. Should be: config.h, primary header, blank line, and then alphabetically sorted.  [build/include_order] [4]
ERROR: Tools/WinLauncher/Common.cpp:425:  Multi line control clauses should use braces.  [whitespace/braces] [4]
ERROR: Tools/WinLauncher/WinMain.cpp:29:  Alphabetical sorting problem.  [build/include_order] [4]
ERROR: Tools/WinLauncher/DOMDefaultImpl.cpp:28:  Found other header before WebCore config.h. Should be: config.h, primary header, blank line, and then alphabetically sorted.  [build/include_order] [4]
ERROR: Tools/WinLauncher/DOMDefaultImpl.cpp:29:  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: 6 in 10 files


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

[Brent Fulgham]

(In reply to comment #4)
> Attachment 233659 [details] did not pass style-queue:
> 
> 
> ERROR: Tools/WinLauncher/WinLauncherWebHost.cpp:29:  Found other header before WebCore config.h. Should be: config.h, primary header, blank line, and then alphabetically sorted.  [build/include_order] [4]
> ERROR: Tools/WinLauncher/WinLauncherWebHost.cpp:30:  Found header this file implements after other header. Should be: config.h, primary header, blank line, and then alphabetically sorted.  [build/include_order] [4]

These are correct. check-webkit-style should handle "stdafx.h" properly.

> ERROR: Tools/WinLauncher/Common.cpp:425:  Multi line control clauses should use braces.  [whitespace/braces] [4]

It does. This code seems to confuse the style checker. It's been this way for years (I just copied to a new file).

> ERROR: Tools/WinLauncher/WinMain.cpp:29:  Alphabetical sorting problem.  [build/include_order] [4]
This is correct. check-webkit-style should handle "stdafx.h" properly.

> ERROR: Tools/WinLauncher/DOMDefaultImpl.cpp:28:  Found other header before WebCore config.h. Should be: config.h, primary header, blank line, and then alphabetically sorted.  [build/include_order] [4]
> ERROR: Tools/WinLauncher/DOMDefaultImpl.cpp:29:  Found header this file implements after other header. Should be: config.h, primary header, blank line, and then alphabetically sorted.  [build/include_order] [4]

These are correct. check-webkit-style should handle "stdafx.h" properly.

[Tim Horton]

Comment on attachment 233659 [details]
Patch

rs=me

[Brent Fulgham]

Committed r170339: <http://trac.webkit.org/changeset/170339>

[Daniel Bates]

For completeness, Brent Fulgham attempted to fix the build in <http://trac.webkit.org/changeset/170340>.

[Daniel Bates]

(In reply to comment #7)
> Committed r170339: <http://trac.webkit.org/changeset/170339>

This broke the Apple Windows Debug and Release builds.

Apple Windows Debug (r170340):
<http://build.webkit.org/builders/Apple%20Win%20Debug%20%28Build%29/builds/80161/steps/compile-webkit/logs/stdio>

Apple Windows Release (r170340):
<http://build.webkit.org/builders/Apple%20Win%20Release%20%28Build%29/builds/62067/steps/compile-webkit/logs/stdio>

Apple Window Debug (r170339):
<http://build.webkit.org/builders/Apple%20Win%20Debug%20%28Build%29/builds/80160/steps/compile-webkit/logs/stdio>

Apple Windows Release (r170339):
<http://build.webkit.org/builders/Apple%20Win%20Release%20%28Build%29/builds/62066/steps/compile-webkit/logs/stdio>

[Daniel Bates]

(In reply to comment #9)
> (In reply to comment #7)
> > Committed r170339: <http://trac.webkit.org/changeset/170339>
> 
> This broke the Apple Windows Debug and Release builds.
> 

I'm going to rollout both <http://trac.webkit.org/changeset/170340> and <http://trac.webkit.org/changeset/170339>.

Looking at the standard output from the bots, the bots are complaining that we're missing file WinLauncher.h, which I assume will have the class definition for WinLauncher. While I could attempt to write out a file WinLauncher.h with a class definition based on WinLauncher.cpp, I suspect Brent Fulgham has such a file and that it would be straightforward to land it when landing the changes again.

[Daniel Bates]

Rolled out <http://trac.webkit.org/changeset/170340> and <http://trac.webkit.org/changeset/170339> in <http://trac.webkit.org/changeset/170346>.

[Brent Fulgham]

(In reply to comment #10)
> (In reply to comment #9)
> > (In reply to comment #7)
> > > Committed r170339: <http://trac.webkit.org/changeset/170339>
> > 
> > This broke the Apple Windows Debug and Release builds.
> > 
> I'm going to rollout both <http://trac.webkit.org/changeset/170340> and <http://trac.webkit.org/changeset/170339>.
> 
> Looking at the standard output from the bots, the bots are complaining that we're missing file WinLauncher.h, which I assume will have the class definition for WinLauncher.

This was caused by some svn mistakes on my part. WinLauncher.h was moved to WinLauncherWebHost.h, and then a new WinLauncher.h was added. This got munged up during my svn manipulations, which caused the archive version of the file to be wrong.

This should correct that.

Landed in <http://trac.webkit.org/changeset/170365>