Bug 134209 - [Win] Clean up and refactor WinLauncher
Summary: [Win] Clean up and refactor WinLauncher
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Tools / Tests (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Brent Fulgham
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2014-06-23 12:41 PDT by Brent Fulgham
Modified: 2014-06-24 09:32 PDT (History)
5 users (show)

See Also:


Attachments
Patch (104.53 KB, patch)
2014-06-23 17:15 PDT, Brent Fulgham
no flags Details | Formatted Diff | Diff
Patch (105.00 KB, patch)
2014-06-23 17:21 PDT, Brent Fulgham
thorton: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Brent Fulgham 2014-06-23 12:41:47 PDT
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.
Comment 1 Brent Fulgham 2014-06-23 17:15:03 PDT
Created attachment 233657 [details]
Patch
Comment 2 WebKit Commit Bot 2014-06-23 17:16:15 PDT
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.
Comment 3 Brent Fulgham 2014-06-23 17:21:35 PDT
Created attachment 233659 [details]
Patch
Comment 4 WebKit Commit Bot 2014-06-23 17:23:44 PDT
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.
Comment 5 Brent Fulgham 2014-06-23 17:26:09 PDT
(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.
Comment 6 Tim Horton 2014-06-23 17:30:38 PDT
Comment on attachment 233659 [details]
Patch

rs=me
Comment 7 Brent Fulgham 2014-06-23 17:52:24 PDT
Committed r170339: <http://trac.webkit.org/changeset/170339>
Comment 8 Daniel Bates 2014-06-23 22:28:13 PDT
For completeness, Brent Fulgham attempted to fix the build in <http://trac.webkit.org/changeset/170340>.
Comment 10 Daniel Bates 2014-06-23 22:44:51 PDT
(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.
Comment 12 Brent Fulgham 2014-06-24 09:20:40 PDT
(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>