Bug 117988 – Merge FrameWinCE into FrameWin

• Component: Platform
• Status: RESOLVED FIXED

[Patrick R. Gansterer]

Merge FrameWinCE into FrameWin

[Patrick R. Gansterer]

Created attachment 205395 [details]
Patch

[WebKit Commit Bot]

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

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCore/PlatformWinCE.cmake', u'Source/WebCore/page/win/FrameGdiWin.cpp', u'Source/WebCore/page/wince/FrameWinCE.cpp']" exit_code: 1
Source/WebCore/page/win/FrameGdiWin.cpp:28:  You should add a blank line after implementation file's own header.  [build/include_order] [4]
Source/WebCore/page/win/FrameGdiWin.cpp:30:  Alphabetical sorting problem.  [build/include_order] [4]
Source/WebCore/page/win/FrameGdiWin.cpp:37:  g_screenDC is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
Total errors found: 3 in 3 files


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

[Brent Fulgham]

Comment on attachment 205395 [details]
Patch

Please correct the style warnings.  Otherwise, this looks great!

[Patrick R. Gansterer]

Comment on attachment 205395 [details]
Patch

(In reply to comment #3)
> (From update of attachment 205395 [details])
> Please correct the style warnings.  Otherwise, this looks great!

The sorting of the header isn't wrong IMHO and the name of the variable is defined at an other place already. So what should I fix exactly?

[Brent Fulgham]

Comment on attachment 205395 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=205395&action=review

>> Source/WebCore/page/win/FrameGdiWin.cpp:37
>> +extern HDC g_screenDC;
> 
> g_screenDC is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]

This should be named something like GlobalScreenDC or SharedScreenDC.  WebKit style is to not use the "g_BlahBlah" naming convention.

[Brent Fulgham]

(In reply to comment #4)
> (From update of attachment 205395 [details])
> (In reply to comment #3)
> > (From update of attachment 205395 [details] [details])
> > Please correct the style warnings.  Otherwise, this looks great!
> 
> The sorting of the header isn't wrong IMHO and the name of the variable is defined at an other place already. So what should I fix exactly?

You currently have:

#include "config.h"
#include "Frame.h"
#include "FrameWin.h"
#include "FrameView.h"

It should probably be something like:

#include "config.h"
#include "FrameWin.h"

#include "FrameView.h"

... or, if Frame.h is what is actually needed to compile (it seems like it should be included by FrameWin.h):

#include "config.h"
#include "Frame.h"

#include "FrameView.h"
#include "FrameWin.h"

[Patrick R. Gansterer]

Created attachment 207925 [details]
Patch

[WebKit Commit Bot]

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

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCore/PlatformWinCE.cmake', u'Source/WebCore/page/win/FrameGdiWin.cpp', u'Source/WebCore/page/wince/FrameWinCE.cpp']" exit_code: 1
Source/WebCore/page/win/FrameGdiWin.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]
Source/WebCore/page/win/FrameGdiWin.cpp:37:  g_screenDC is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
Total errors found: 2 in 3 files


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

[Brent Fulgham]

(In reply to comment #8)
> Attachment 207925 [details] did not pass style-queue:
> 
> Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCore/PlatformWinCE.cmake', u'Source/WebCore/page/win/FrameGdiWin.cpp', u'Source/WebCore/page/wince/FrameWinCE.cpp']" exit_code: 1
> Source/WebCore/page/win/FrameGdiWin.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]
> Source/WebCore/page/win/FrameGdiWin.cpp:37:  g_screenDC is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
> Total errors found: 2 in 3 files

It doesn't seem like you resolved the two style issues I mentioned earlier in this bug report.

[Patrick R. Gansterer]

Comment on attachment 207925 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=207925&action=review

>> Source/WebCore/page/win/FrameGdiWin.cpp:30
>> +#include "Frame.h"
> 
> Found header this file implements after other header. Should be: config.h, primary header, blank line, and then alphabetically sorted.  [build/include_order] [4]

there is no FrameGdiWin.h, so FrameWin.h is the correct header.

>> Source/WebCore/page/win/FrameGdiWin.cpp:37
>> +extern HDC g_screenDC;
> 
> g_screenDC is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]

g_screenDC is an _extern_ variable and already used across the GDI files. IMHO changing the name is out of scope of this patch.

[Brent Fulgham]

(In reply to comment #10)
> (From update of attachment 207925 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=207925&action=review
> 
> >> Source/WebCore/page/win/FrameGdiWin.cpp:30
> >> +#include "Frame.h"
> > 
> > Found header this file implements after other header. Should be: config.h, primary header, blank line, and then alphabetically sorted.  [build/include_order] [4]
> 
> there is no FrameGdiWin.h, so FrameWin.h is the correct header.

Does Frame.h need to be included separately?  It seems like it should be:

#include "config.h"
#include "FrameWin.h"
<space>
... other stuff ...

 > >> Source/WebCore/page/win/FrameGdiWin.cpp:37
> >> +extern HDC g_screenDC;
> g_screenDC is an _extern_ variable and already used across the GDI files. IMHO changing the name is out of scope of this patch.

Good point! Leave it!

[Patrick R. Gansterer]

(In reply to comment #11)
> (In reply to comment #10)
> > (From update of attachment 207925 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=207925&action=review
> > 
> > >> Source/WebCore/page/win/FrameGdiWin.cpp:30
> > >> +#include "Frame.h"
> > > 
> > > Found header this file implements after other header. Should be: config.h, primary header, blank line, and then alphabetically sorted.  [build/include_order] [4]
> > 
> > there is no FrameGdiWin.h, so FrameWin.h is the correct header.
> 
> Does Frame.h need to be included separately?  It seems like it should be:
> 
> #include "config.h"
> #include "FrameWin.h"
> <space>
> ... other stuff ...

Frame.h is required. Please see http://trac.webkit.org/browser/trunk/Source/WebCore/page/win/FrameCGWin.cpp

[Brent Fulgham]

Comment on attachment 207925 [details]
Patch

r=me

[WebKit Commit Bot]

Comment on attachment 207925 [details]
Patch

Clearing flags on attachment: 207925

Committed r153589: <http://trac.webkit.org/changeset/153589>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.