Bug 134209

Summary:

[Win] Clean up and refactor WinLauncher

Product:

WebKit

Reporter:

Brent Fulgham <bfulgham>

Component:

Tools / Tests

Assignee:

Brent Fulgham <bfulgham>

Status:

RESOLVED FIXED

Severity:

Normal

CC:

bfulgham, commit-queue, dbates, dino, thorton

Priority:

Version:

528+ (Nightly build)

Hardware:

Unspecified

OS:

Unspecified

Attachments:

Description	Flags
Patch	none
Patch	thorton: review+

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 9 Daniel Bates 2014-06-23 22:34:30 PDT

(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>

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 11 Daniel Bates 2014-06-23 22:47:37 PDT

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

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>