Merge FrameWinCE into FrameWin
Created attachment 205395 [details] Patch
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 on attachment 205395 [details] Patch Please correct the style warnings. Otherwise, this looks great!
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 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.
(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"
Created attachment 207925 [details] Patch
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.
(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 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.
(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!
(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 on attachment 207925 [details] Patch r=me
Comment on attachment 207925 [details] Patch Clearing flags on attachment: 207925 Committed r153589: <http://trac.webkit.org/changeset/153589>
All reviewed patches have been landed. Closing bug.