Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
120773007:
content: Copy layout test helpers from Blink to Chromium. (Closed)
|
Created:
6 years, 11 months ago by tfarina Modified:
6 years, 11 months ago Reviewers:
jochen (gone - plz use gerrit) CC:
chromium-reviews, joi+watch-content_chromium.org, darin-cc_chromium.org, jam, jochen+watch_chromium.org Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
Descriptioncontent: Copy layout test helpers from Blink to Chromium.
It just copies the source files first, the gyp changes will come next.
BUG=331304
TEST=None, no functional changes yet.
R=jochen@chromium.org
NOTRY=true
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=242982
Patch Set 1 #Patch Set 2 : #
Total comments: 4
Patch Set 3 : rename #Patch Set 4 : format #
Total comments: 44
Patch Set 5 : fix style nits #Patch Set 6 : revert some unintended changes #
Messages
Total messages: 7 (0 generated)
since the files are so small, can you rename them to chromium_style.cc and reformat them according to chromium style? https://codereview.chromium.org/120773007/diff/20001/content/shell/renderer/t... File content/shell/renderer/test_runner/helper/LayoutTestHelperMac.mm (right): https://codereview.chromium.org/120773007/diff/20001/content/shell/renderer/t... content/shell/renderer/test_runner/helper/LayoutTestHelperMac.mm:1: /* can you put a chromium style copyright header on top of this one? We'll need to keep the old one around because of the Apple line. https://codereview.chromium.org/120773007/diff/20001/content/shell/renderer/t... File content/shell/renderer/test_runner/helper/LayoutTestHelperWin.cpp (right): https://codereview.chromium.org/120773007/diff/20001/content/shell/renderer/t... content/shell/renderer/test_runner/helper/LayoutTestHelperWin.cpp:1: /* please replace this header with a chromium-style copyright header.
On 2014/01/03 13:16:52, jochen wrote: > since the files are so small, can you rename them to chromium_style.cc and > reformat them according to chromium style? > Done. https://codereview.chromium.org/120773007/diff/20001/content/shell/renderer/t... File content/shell/renderer/test_runner/helper/LayoutTestHelperMac.mm (right): https://codereview.chromium.org/120773007/diff/20001/content/shell/renderer/t... content/shell/renderer/test_runner/helper/LayoutTestHelperMac.mm:1: /* On 2014/01/03 13:16:52, jochen wrote: > can you put a chromium style copyright header on top of this one? > > We'll need to keep the old one around because of the Apple line. Done. https://codereview.chromium.org/120773007/diff/20001/content/shell/renderer/t... File content/shell/renderer/test_runner/helper/LayoutTestHelperWin.cpp (right): https://codereview.chromium.org/120773007/diff/20001/content/shell/renderer/t... content/shell/renderer/test_runner/helper/LayoutTestHelperWin.cpp:1: /* On 2014/01/03 13:16:52, jochen wrote: > please replace this header with a chromium-style copyright header. Done.
lgtm with a bunch of nits https://codereview.chromium.org/120773007/diff/120001/content/shell/renderer/... File content/shell/renderer/test_runner/helper/layout_test_helper_mac.mm (right): https://codereview.chromium.org/120773007/diff/120001/content/shell/renderer/... content/shell/renderer/test_runner/helper/layout_test_helper_mac.mm:49: #if defined(MAC_OS_X_VERSION_10_7) && MAC_OS_X_VERSION_MAX_ALLOWED >= MAC_OS_X_VERSION_10_7 nit. wrap at 80c https://codereview.chromium.org/120773007/diff/120001/content/shell/renderer/... content/shell/renderer/test_runner/helper/layout_test_helper_mac.mm:51: CFURLRef sUserColorProfileURL; nit. user_color_profile_url https://codereview.chromium.org/120773007/diff/120001/content/shell/renderer/... content/shell/renderer/test_runner/helper/layout_test_helper_mac.mm:53: void installLayoutTestColorProfile() { InstallLayout... (with a capital I) https://codereview.chromium.org/120773007/diff/120001/content/shell/renderer/... content/shell/renderer/test_runner/helper/layout_test_helper_mac.mm:58: CFUUIDRef mainDisplayID = CGDisplayCreateUUIDFromDisplayID(CGMainDisplayID()); main_display_id https://codereview.chromium.org/120773007/diff/120001/content/shell/renderer/... content/shell/renderer/test_runner/helper/layout_test_helper_mac.mm:61: CFDictionaryRef deviceInfo = ColorSyncDeviceCopyDeviceInfo( device_info https://codereview.chromium.org/120773007/diff/120001/content/shell/renderer/... content/shell/renderer/test_runner/helper/layout_test_helper_mac.mm:71: CFDictionaryRef profileInfo = (CFDictionaryRef)CFDictionaryGetValue( profile_info https://codereview.chromium.org/120773007/diff/120001/content/shell/renderer/... content/shell/renderer/test_runner/helper/layout_test_helper_mac.mm:80: CFDictionaryRef factoryProfile = factory_profile https://codereview.chromium.org/120773007/diff/120001/content/shell/renderer/... content/shell/renderer/test_runner/helper/layout_test_helper_mac.mm:90: ColorSyncProfileRef genericRGBProfile = generic_rgb_profile https://codereview.chromium.org/120773007/diff/120001/content/shell/renderer/... content/shell/renderer/test_runner/helper/layout_test_helper_mac.mm:93: CFURLRef profileURL = ColorSyncProfileGetURL(genericRGBProfile, &error); profile_url https://codereview.chromium.org/120773007/diff/120001/content/shell/renderer/... content/shell/renderer/test_runner/helper/layout_test_helper_mac.mm:109: CFMutableDictionaryRef profileInfo = profile_info https://codereview.chromium.org/120773007/diff/120001/content/shell/renderer/... content/shell/renderer/test_runner/helper/layout_test_helper_mac.mm:133: void restoreUserColorProfile(void) { Restore... and only () instead of (void) https://codereview.chromium.org/120773007/diff/120001/content/shell/renderer/... content/shell/renderer/test_runner/helper/layout_test_helper_mac.mm:142: CFUUIDRef mainDisplayID = CGDisplayCreateUUIDFromDisplayID(CGMainDisplayID()); main_display_id https://codereview.chromium.org/120773007/diff/120001/content/shell/renderer/... content/shell/renderer/test_runner/helper/layout_test_helper_mac.mm:143: CFMutableDictionaryRef profileInfo = profile_info https://codereview.chromium.org/120773007/diff/120001/content/shell/renderer/... content/shell/renderer/test_runner/helper/layout_test_helper_mac.mm:158: const char colorProfilePath[] = color_profile_path https://codereview.chromium.org/120773007/diff/120001/content/shell/renderer/... content/shell/renderer/test_runner/helper/layout_test_helper_mac.mm:161: CMProfileLocation initialColorProfileLocation; // The locType field is initial_color_profile_location. Can you put the comment above this line? https://codereview.chromium.org/120773007/diff/120001/content/shell/renderer/... content/shell/renderer/test_runner/helper/layout_test_helper_mac.mm:165: void installLayoutTestColorProfile() { Install.. https://codereview.chromium.org/120773007/diff/120001/content/shell/renderer/... content/shell/renderer/test_runner/helper/layout_test_helper_mac.mm:206: void restoreUserColorProfile(void) { Restore and no (void) https://codereview.chromium.org/120773007/diff/120001/content/shell/renderer/... content/shell/renderer/test_runner/helper/layout_test_helper_mac.mm:230: void simpleSignalHandler(int sig) { SimpleSignal.. https://codereview.chromium.org/120773007/diff/120001/content/shell/renderer/... File content/shell/renderer/test_runner/helper/layout_test_helper_win.cc (right): https://codereview.chromium.org/120773007/diff/120001/content/shell/renderer/... content/shell/renderer/test_runner/helper/layout_test_helper_win.cc:10: static BOOL fontSmoothingEnabled = FALSE; font_smoothing_enabled https://codereview.chromium.org/120773007/diff/120001/content/shell/renderer/... content/shell/renderer/test_runner/helper/layout_test_helper_win.cc:12: static void saveInitialSettings(void) { SaveInitial... and no (void) https://codereview.chromium.org/120773007/diff/120001/content/shell/renderer/... content/shell/renderer/test_runner/helper/layout_test_helper_win.cc:20: static void installLayoutTestSettings(void) { Install... and no (void) https://codereview.chromium.org/120773007/diff/120001/content/shell/renderer/... content/shell/renderer/test_runner/helper/layout_test_helper_win.cc:24: static void restoreInitialSettings(void) { Restore... and no (void) https://codereview.chromium.org/120773007/diff/120001/content/shell/renderer/... content/shell/renderer/test_runner/helper/layout_test_helper_win.cc:29: static void simpleSignalHandler(int signalNumber) { Simple...
https://codereview.chromium.org/120773007/diff/120001/content/shell/renderer/... File content/shell/renderer/test_runner/helper/layout_test_helper_mac.mm (right): https://codereview.chromium.org/120773007/diff/120001/content/shell/renderer/... content/shell/renderer/test_runner/helper/layout_test_helper_mac.mm:49: #if defined(MAC_OS_X_VERSION_10_7) && MAC_OS_X_VERSION_MAX_ALLOWED >= MAC_OS_X_VERSION_10_7 On 2014/01/03 14:08:58, jochen wrote: > nit. wrap at 80c Done. https://codereview.chromium.org/120773007/diff/120001/content/shell/renderer/... content/shell/renderer/test_runner/helper/layout_test_helper_mac.mm:51: CFURLRef sUserColorProfileURL; On 2014/01/03 14:08:58, jochen wrote: > nit. user_color_profile_url Done. https://codereview.chromium.org/120773007/diff/120001/content/shell/renderer/... content/shell/renderer/test_runner/helper/layout_test_helper_mac.mm:53: void installLayoutTestColorProfile() { On 2014/01/03 14:08:58, jochen wrote: > InstallLayout... (with a capital I) Done. https://codereview.chromium.org/120773007/diff/120001/content/shell/renderer/... content/shell/renderer/test_runner/helper/layout_test_helper_mac.mm:58: CFUUIDRef mainDisplayID = CGDisplayCreateUUIDFromDisplayID(CGMainDisplayID()); On 2014/01/03 14:08:58, jochen wrote: > main_display_id Done. https://codereview.chromium.org/120773007/diff/120001/content/shell/renderer/... content/shell/renderer/test_runner/helper/layout_test_helper_mac.mm:61: CFDictionaryRef deviceInfo = ColorSyncDeviceCopyDeviceInfo( On 2014/01/03 14:08:58, jochen wrote: > device_info Done. https://codereview.chromium.org/120773007/diff/120001/content/shell/renderer/... content/shell/renderer/test_runner/helper/layout_test_helper_mac.mm:71: CFDictionaryRef profileInfo = (CFDictionaryRef)CFDictionaryGetValue( On 2014/01/03 14:08:58, jochen wrote: > profile_info Done. https://codereview.chromium.org/120773007/diff/120001/content/shell/renderer/... content/shell/renderer/test_runner/helper/layout_test_helper_mac.mm:80: CFDictionaryRef factoryProfile = On 2014/01/03 14:08:58, jochen wrote: > factory_profile Done. https://codereview.chromium.org/120773007/diff/120001/content/shell/renderer/... content/shell/renderer/test_runner/helper/layout_test_helper_mac.mm:90: ColorSyncProfileRef genericRGBProfile = On 2014/01/03 14:08:58, jochen wrote: > generic_rgb_profile Done. https://codereview.chromium.org/120773007/diff/120001/content/shell/renderer/... content/shell/renderer/test_runner/helper/layout_test_helper_mac.mm:93: CFURLRef profileURL = ColorSyncProfileGetURL(genericRGBProfile, &error); On 2014/01/03 14:08:58, jochen wrote: > profile_url Done. https://codereview.chromium.org/120773007/diff/120001/content/shell/renderer/... content/shell/renderer/test_runner/helper/layout_test_helper_mac.mm:109: CFMutableDictionaryRef profileInfo = On 2014/01/03 14:08:58, jochen wrote: > profile_info Done. https://codereview.chromium.org/120773007/diff/120001/content/shell/renderer/... content/shell/renderer/test_runner/helper/layout_test_helper_mac.mm:133: void restoreUserColorProfile(void) { On 2014/01/03 14:08:58, jochen wrote: > Restore... and only () instead of (void) Done. https://codereview.chromium.org/120773007/diff/120001/content/shell/renderer/... content/shell/renderer/test_runner/helper/layout_test_helper_mac.mm:142: CFUUIDRef mainDisplayID = CGDisplayCreateUUIDFromDisplayID(CGMainDisplayID()); On 2014/01/03 14:08:58, jochen wrote: > main_display_id Done. https://codereview.chromium.org/120773007/diff/120001/content/shell/renderer/... content/shell/renderer/test_runner/helper/layout_test_helper_mac.mm:143: CFMutableDictionaryRef profileInfo = On 2014/01/03 14:08:58, jochen wrote: > profile_info Done. https://codereview.chromium.org/120773007/diff/120001/content/shell/renderer/... content/shell/renderer/test_runner/helper/layout_test_helper_mac.mm:158: const char colorProfilePath[] = On 2014/01/03 14:08:58, jochen wrote: > color_profile_path Done. https://codereview.chromium.org/120773007/diff/120001/content/shell/renderer/... content/shell/renderer/test_runner/helper/layout_test_helper_mac.mm:161: CMProfileLocation initialColorProfileLocation; // The locType field is On 2014/01/03 14:08:58, jochen wrote: > initial_color_profile_location. Can you put the comment above this line? Done. https://codereview.chromium.org/120773007/diff/120001/content/shell/renderer/... content/shell/renderer/test_runner/helper/layout_test_helper_mac.mm:165: void installLayoutTestColorProfile() { On 2014/01/03 14:08:58, jochen wrote: > Install.. Done. https://codereview.chromium.org/120773007/diff/120001/content/shell/renderer/... File content/shell/renderer/test_runner/helper/layout_test_helper_win.cc (right): https://codereview.chromium.org/120773007/diff/120001/content/shell/renderer/... content/shell/renderer/test_runner/helper/layout_test_helper_win.cc:10: static BOOL fontSmoothingEnabled = FALSE; On 2014/01/03 14:08:58, jochen wrote: > font_smoothing_enabled Done. https://codereview.chromium.org/120773007/diff/120001/content/shell/renderer/... content/shell/renderer/test_runner/helper/layout_test_helper_win.cc:12: static void saveInitialSettings(void) { On 2014/01/03 14:08:58, jochen wrote: > SaveInitial... and no (void) Done. https://codereview.chromium.org/120773007/diff/120001/content/shell/renderer/... content/shell/renderer/test_runner/helper/layout_test_helper_win.cc:20: static void installLayoutTestSettings(void) { On 2014/01/03 14:08:58, jochen wrote: > Install... and no (void) Done. https://codereview.chromium.org/120773007/diff/120001/content/shell/renderer/... content/shell/renderer/test_runner/helper/layout_test_helper_win.cc:24: static void restoreInitialSettings(void) { On 2014/01/03 14:08:58, jochen wrote: > Restore... and no (void) Done. https://codereview.chromium.org/120773007/diff/120001/content/shell/renderer/... content/shell/renderer/test_runner/helper/layout_test_helper_win.cc:29: static void simpleSignalHandler(int signalNumber) { On 2014/01/03 14:08:58, jochen wrote: > Simple... Done.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/tfarina@chromium.org/120773007/220001
Message was sent while issue was closed.
Change committed as 242982 |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||
