Bug 117988 - Merge FrameWinCE into FrameWin
Summary: Merge FrameWinCE into FrameWin
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Platform (show other bugs)
Version: 528+ (Nightly build)
Hardware: Other Other
: P2 Normal
Assignee: Patrick R. Gansterer
URL:
Keywords:
Depends on: 117984
Blocks:
  Show dependency treegraph
 
Reported: 2013-06-25 06:07 PDT by Patrick R. Gansterer
Modified: 2013-08-01 10:05 PDT (History)
4 users (show)

See Also:


Attachments
Patch (9.27 KB, patch)
2013-06-25 06:16 PDT, Patrick R. Gansterer
no flags Details | Formatted Diff | Diff
Patch (9.57 KB, patch)
2013-08-01 07:59 PDT, Patrick R. Gansterer
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Patrick R. Gansterer 2013-06-25 06:07:36 PDT
Merge FrameWinCE into FrameWin
Comment 1 Patrick R. Gansterer 2013-06-25 06:16:06 PDT
Created attachment 205395 [details]
Patch
Comment 2 WebKit Commit Bot 2013-06-25 06:18:27 PDT
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.
Comment 3 Brent Fulgham 2013-07-30 09:30:27 PDT
Comment on attachment 205395 [details]
Patch

Please correct the style warnings.  Otherwise, this looks great!
Comment 4 Patrick R. Gansterer 2013-07-30 09:34:44 PDT
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?
Comment 5 Brent Fulgham 2013-07-30 10:21:33 PDT
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.
Comment 6 Brent Fulgham 2013-07-30 10:25:38 PDT
(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"
Comment 7 Patrick R. Gansterer 2013-08-01 07:59:19 PDT
Created attachment 207925 [details]
Patch
Comment 8 WebKit Commit Bot 2013-08-01 08:00:42 PDT
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.
Comment 9 Brent Fulgham 2013-08-01 09:05:42 PDT
(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.
Comment 10 Patrick R. Gansterer 2013-08-01 09:07:20 PDT
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.
Comment 11 Brent Fulgham 2013-08-01 09:26:01 PDT
(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!
Comment 12 Patrick R. Gansterer 2013-08-01 09:31:50 PDT
(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
Comment 13 Brent Fulgham 2013-08-01 09:42:53 PDT
Comment on attachment 207925 [details]
Patch

r=me
Comment 14 WebKit Commit Bot 2013-08-01 10:05:49 PDT
Comment on attachment 207925 [details]
Patch

Clearing flags on attachment: 207925

Committed r153589: <http://trac.webkit.org/changeset/153589>
Comment 15 WebKit Commit Bot 2013-08-01 10:05:53 PDT
All reviewed patches have been landed.  Closing bug.