WebCore refactoring of device motion/orientation and fixing of webkit ports
Created attachment 195536 [details] Patch
Please wait for approval from abarth@webkit.org, dglazkov@chromium.org, fishd@chromium.org, jamesr@chromium.org or tkent@chromium.org before submitting, as this patch contains changes to the Chromium public API. See also https://trac.webkit.org/wiki/ChromiumWebKitAPI.
Attachment 195536 [details] did not pass style-queue: Source/WebCore/page/DeviceController.cpp:54: Should have a space between // and comment [whitespace/comments] [4] Source/WebCore/page/DeviceController.cpp:64: Should have a space between // and comment [whitespace/comments] [4] Source/WebCore/page/DeviceController.cpp:74: Should have a space between // and comment [whitespace/comments] [4] Source/WebCore/platform/chromium/DeviceOrientationChromium.cpp:27: Alphabetical sorting problem. [build/include_order] [4] Source/WebCore/platform/chromium/DeviceOrientationChromium.cpp:40: Place brace on its own line for function definitions. [whitespace/braces] [4] Source/WebCore/platform/chromium/DeviceOrientationChromium.cpp:41: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] Source/WebCore/platform/chromium/DeviceOrientationChromium.cpp:42: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] Source/WebCore/platform/chromium/DeviceOrientationChromium.cpp:43: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] Source/WebCore/platform/chromium/DeviceOrientationChromium.cpp:44: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] Source/WebCore/platform/chromium/DeviceOrientationChromium.cpp:47: Place brace on its own line for function definitions. [whitespace/braces] [4] Source/WebCore/platform/chromium/DeviceOrientationChromium.cpp:48: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] Source/WebCore/platform/chromium/DeviceOrientationChromium.cpp:49: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] Source/WebCore/platform/chromium/DeviceOrientationChromium.cpp:50: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] Source/WebCore/platform/chromium/DeviceOrientationChromium.cpp:51: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] Source/WebCore/platform/qt/DeviceMotionProviderQt.cpp:39: Place brace on its own line for function definitions. [whitespace/braces] [4] Source/WebCore/platform/qt/DeviceMotionProviderQt.cpp:42: One line control clauses should not use braces. [whitespace/braces] [4] Source/WebCore/platform/qt/DeviceOrientationProviderQt.cpp:34: Place brace on its own line for function definitions. [whitespace/braces] [4] Source/WebCore/platform/qt/DeviceOrientationProviderQt.cpp:37: One line control clauses should not use braces. [whitespace/braces] [4] Source/WebCore/dom/DeviceMotionController.cpp:43: Place brace on its own line for function definitions. [whitespace/braces] [4] Source/WebCore/dom/DeviceMotionController.cpp:44: Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons. [readability/comparison_to_zero] [5] Source/WebCore/dom/DeviceMotionController.cpp:46: One line control clauses should not use braces. [whitespace/braces] [4] Source/WebCore/dom/DeviceMotionController.cpp:59: Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons. [readability/comparison_to_zero] [5] Source/WebCore/dom/DeviceMotionController.cpp:67: Place brace on its own line for function definitions. [whitespace/braces] [4] Source/WebCore/dom/DeviceMotionController.cpp:71: Place brace on its own line for function definitions. [whitespace/braces] [4] Source/WebCore/platform/qt/DeviceMotionQt.cpp:27: Alphabetical sorting problem. [build/include_order] [4] Source/WebCore/platform/qt/DeviceMotionQt.cpp:35: Place brace on its own line for function definitions. [whitespace/braces] [4] Source/WebCore/platform/qt/DeviceMotionQt.cpp:36: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] Source/WebCore/platform/qt/DeviceMotionQt.cpp:37: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] Source/WebCore/platform/qt/DeviceMotionQt.cpp:38: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] Source/WebCore/platform/qt/DeviceMotionQt.cpp:39: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] Source/WebCore/platform/qt/DeviceMotionQt.cpp:40: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] Source/WebCore/platform/qt/DeviceMotionQt.cpp:43: Place brace on its own line for function definitions. [whitespace/braces] [4] Source/WebCore/platform/qt/DeviceMotionQt.cpp:44: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] Source/WebCore/platform/qt/DeviceMotionQt.cpp:45: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] Source/WebCore/platform/qt/DeviceMotionQt.cpp:46: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] Source/WebCore/platform/qt/DeviceMotionQt.cpp:47: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] Source/WebCore/platform/qt/DeviceOrientationQt.cpp:27: Alphabetical sorting problem. [build/include_order] [4] Source/WebCore/platform/qt/DeviceOrientationQt.cpp:35: Place brace on its own line for function definitions. [whitespace/braces] [4] Source/WebCore/platform/qt/DeviceOrientationQt.cpp:36: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] Source/WebCore/platform/qt/DeviceOrientationQt.cpp:37: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] Source/WebCore/platform/qt/DeviceOrientationQt.cpp:38: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] Source/WebCore/platform/qt/DeviceOrientationQt.cpp:39: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] Source/WebCore/platform/qt/DeviceOrientationQt.cpp:42: Place brace on its own line for function definitions. [whitespace/braces] [4] Source/WebCore/platform/qt/DeviceOrientationQt.cpp:43: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] Source/WebCore/platform/qt/DeviceOrientationQt.cpp:44: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] Source/WebCore/platform/qt/DeviceOrientationQt.cpp:45: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] Source/WebCore/platform/qt/DeviceOrientationQt.cpp:46: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] Source/WebCore/platform/chromium/DeviceMotionChromium.cpp:35: Place brace on its own line for function definitions. [whitespace/braces] [4] Source/WebCore/platform/chromium/DeviceMotionChromium.cpp:36: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] Source/WebCore/platform/chromium/DeviceMotionChromium.cpp:39: Place brace on its own line for function definitions. [whitespace/braces] [4] Source/WebCore/platform/chromium/DeviceMotionChromium.cpp:40: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] Source/WebCore/dom/DeviceOrientationController.cpp:47: Place brace on its own line for function definitions. [whitespace/braces] [4] Source/WebCore/dom/DeviceOrientationController.cpp:48: Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons. [readability/comparison_to_zero] [5] Source/WebCore/dom/DeviceOrientationController.cpp:50: One line control clauses should not use braces. [whitespace/braces] [4] Source/WebCore/dom/DeviceOrientationController.cpp:59: Should have a space between // and comment [whitespace/comments] [4] Source/WebCore/dom/DeviceOrientationController.cpp:65: One space before end of line comments [whitespace/comments] [5] Source/WebCore/dom/DeviceOrientationController.cpp:65: ShouldFailed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/Platform/ChangeLog', u'Source/Platform/chromium/public/Platform.h', u'Source/WebCore/ChangeLog', u'Source/WebCore/Target.pri', u'Source/WebCore/WebCore.gypi', u'Source/WebCore/dom/DeviceMotionController.cpp', u'Source/WebCore/dom/DeviceMotionController.h', u'Source/WebCore/dom/DeviceOrientationController.cpp', u'Source/WebCore/dom/DeviceOrientationController.h', u'Source/WebCore/history/PageCache.cpp', u'Source/WebCore/inspector/InspectorPageAgent.cpp', u'Source/WebCore/page/DOMWindow.cpp', u'Source/WebCore/page/DOMWindow.h', u'Source/WebCore/page/DeviceController.cpp', u'Source/WebCore/page/DeviceController.h', u'Source/WebCore/platform/PlatformDeviceMotion.h', u'Source/WebCore/platform/PlatformDeviceOrientation.h', u'Source/WebCore/platform/chromium/DeviceMotionChromium.cpp', u'Source/WebCore/platform/chromium/DeviceOrientationChromium.cpp', u'Source/WebCore/platform/qt/DeviceMotionProviderQt.cpp', u'Source/WebCore/platform/qt/DeviceMotionProviderQt.h', u'Source/WebCore/platform/qt/DeviceMotionQt.cpp', u'Source/WebCore/platform/qt/DeviceOrientationProviderQt.cpp', u'Source/WebCore/platform/qt/DeviceOrientationProviderQt.h', u'Source/WebCore/platform/qt/DeviceOrientationQt.cpp', u'Source/WebKit/chromium/ChangeLog', u'Source/WebKit/chromium/src/WebDeviceOrientationController.cpp', u'Source/WebKit/chromium/src/WebViewImpl.cpp']" exit_code: 1 have a space between // and comment [whitespace/comments] [4] Source/WebCore/dom/DeviceOrientationController.cpp:74: Place brace on its own line for function definitions. [whitespace/braces] [4] Source/WebCore/dom/DeviceOrientationController.cpp:79: Place brace on its own line for function definitions. [whitespace/braces] [4] Total errors found: 59 in 28 files If any of these errors are false positives, please file a bug against check-webkit-style.
Comment on attachment 195536 [details] Patch Attachment 195536 [details] did not pass efl-ews (efl): Output: http://webkit-commit-queue.appspot.com/results/17322223
Comment on attachment 195536 [details] Patch Attachment 195536 [details] did not pass qt-ews (qt): Output: http://webkit-commit-queue.appspot.com/results/17257513
Comment on attachment 195536 [details] Patch Attachment 195536 [details] did not pass qt-wk2-ews (qt): Output: http://webkit-commit-queue.appspot.com/results/17257514
Comment on attachment 195536 [details] Patch Attachment 195536 [details] did not pass cr-linux-debug-ews (chromium-xvfb): Output: http://webkit-commit-queue.appspot.com/results/17320314
Comment on attachment 195536 [details] Patch Attachment 195536 [details] did not pass win-ews (win): Output: http://webkit-commit-queue.appspot.com/results/17358006
Comment on attachment 195536 [details] Patch Attachment 195536 [details] did not pass gtk-ews (gtk): Output: http://webkit-commit-queue.appspot.com/results/17257524
Comment on attachment 195536 [details] Patch Attachment 195536 [details] did not pass chromium-ews (chromium-xvfb): Output: http://webkit-commit-queue.appspot.com/results/17322226 New failing tests: accessibility/aria-checkbox-checked.html accessibility/aria-labelledby-on-input.html animations/3d/matrix-transform-type-animation.html http/tests/appcache/cyrillic-uri.html animations/animation-add-events-in-handler.html http/tests/appcache/deferred-events-delete-while-raising.html canvas/philip/tests/2d.clearRect.globalalpha.html accessibility/aria-describedby-on-input.html http/tests/appcache/crash-when-navigating-away-then-back.html animations/animation-direction-alternate-reverse.html accessibility/aria-labelledby-overrides-label.html http/tests/appcache/credential-url.html http/tests/appcache/access-via-redirect.php http/tests/appcache/different-scheme.html accessibility/aria-hidden.html http/tests/appcache/destroyed-frame.html http/tests/appcache/deferred-events-delete-while-raising-timer.html animations/animation-controller-drt-api.html accessibility/aria-hidden-hides-all-elements.html animations/3d/transform-perspective.html accessibility/aria-hidden-updates-alldescendants.html accessibility/accessibility-node-reparent.html canvas/philip/tests/2d.canvas.reference.html animations/3d/state-at-end-event-transform.html canvas/philip/tests/2d.clearRect.basic.html accessibility/aria-fallback-roles.html animations/animation-direction-reverse-fill-mode-hardware.html accessibility/adjacent-continuations-cause-assertion-failure.html http/tests/appcache/detached-iframe.html canvas/philip/tests/2d.clearRect+fillRect.alpha0.html
Created attachment 195541 [details] Archive of layout-test-results from gce-cr-linux-06 for chromium-linux-x86_64 The attached test failures were seen while running run-webkit-tests on the chromium-ews. Bot: gce-cr-linux-06 Port: chromium-linux-x86_64 Platform: Linux-3.3.8-gcg-201212281604-x86_64-with-GCEL-10.04-gcel_10.04
Created attachment 195542 [details] Patch
Attachment 195542 [details] did not pass style-queue: Source/WebCore/page/DeviceController.cpp:54: Should have a space between // and comment [whitespace/comments] [4] Source/WebCore/page/DeviceController.cpp:64: Should have a space between // and comment [whitespace/comments] [4] Source/WebCore/page/DeviceController.cpp:74: Should have a space between // and comment [whitespace/comments] [4] Source/WebCore/platform/chromium/DeviceOrientationChromium.cpp:27: Alphabetical sorting problem. [build/include_order] [4] Source/WebCore/platform/chromium/DeviceOrientationChromium.cpp:40: Place brace on its own line for function definitions. [whitespace/braces] [4] Source/WebCore/platform/chromium/DeviceOrientationChromium.cpp:41: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] Source/WebCore/platform/chromium/DeviceOrientationChromium.cpp:42: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] Source/WebCore/platform/chromium/DeviceOrientationChromium.cpp:43: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] Source/WebCore/platform/chromium/DeviceOrientationChromium.cpp:44: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] Source/WebCore/platform/chromium/DeviceOrientationChromium.cpp:47: Place brace on its own line for function definitions. [whitespace/braces] [4] Source/WebCore/platform/chromium/DeviceOrientationChromium.cpp:48: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] Source/WebCore/platform/chromium/DeviceOrientationChromium.cpp:49: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] Source/WebCore/platform/chromium/DeviceOrientationChromium.cpp:50: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] Source/WebCore/platform/chromium/DeviceOrientationChromium.cpp:51: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] Source/WebCore/platform/qt/DeviceMotionProviderQt.cpp:39: Place brace on its own line for function definitions. [whitespace/braces] [4] Source/WebCore/platform/qt/DeviceMotionProviderQt.cpp:42: One line control clauses should not use braces. [whitespace/braces] [4] Source/WebCore/platform/qt/DeviceOrientationProviderQt.cpp:34: Place brace on its own line for function definitions. [whitespace/braces] [4] Source/WebCore/platform/qt/DeviceOrientationProviderQt.cpp:37: One line control clauses should not use braces. [whitespace/braces] [4] Source/WebCore/dom/DeviceMotionController.cpp:43: Place brace on its own line for function definitions. [whitespace/braces] [4] Source/WebCore/dom/DeviceMotionController.cpp:44: Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons. [readability/comparison_to_zero] [5] Source/WebCore/dom/DeviceMotionController.cpp:46: One line control clauses should not use braces. [whitespace/braces] [4] Source/WebCore/dom/DeviceMotionController.cpp:59: Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons. [readability/comparison_to_zero] [5] Source/WebCore/dom/DeviceMotionController.cpp:67: Place brace on its own line for function definitions. [whitespace/braces] [4] Source/WebCore/dom/DeviceMotionController.cpp:71: Place brace on its own line for function definitions. [whitespace/braces] [4] Source/WebCore/platform/qt/DeviceMotionQt.cpp:27: Alphabetical sorting problem. [build/include_order] [4] Source/WebCore/platform/qt/DeviceMotionQt.cpp:35: Place brace on its own line for function definitions. [whitespace/braces] [4] Source/WebCore/platform/qt/DeviceMotionQt.cpp:36: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] Source/WebCore/platform/qt/DeviceMotionQt.cpp:37: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] Source/WebCore/platform/qt/DeviceMotionQt.cpp:38: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] Source/WebCore/platform/qt/DeviceMotionQt.cpp:39: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] Source/WebCore/platform/qt/DeviceMotionQt.cpp:40: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] Source/WebCore/platform/qt/DeviceMotionQt.cpp:43: Place brace on its own line for function definitions. [whitespace/braces] [4] Source/WebCore/platform/qt/DeviceMotionQt.cpp:44: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] Source/WebCore/platform/qt/DeviceMotionQt.cpp:45: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] Source/WebCore/platform/qt/DeviceMotionQt.cpp:46: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] Source/WebCore/platform/qt/DeviceMotionQt.cpp:47: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] Source/WebCore/platform/qt/DeviceOrientationQt.cpp:27: Alphabetical sorting problem. [build/include_order] [4] Source/WebCore/platform/qt/DeviceOrientationQt.cpp:35: Place brace on its own line for function definitions. [whitespace/braces] [4] Source/WebCore/platform/qt/DeviceOrientationQt.cpp:36: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] Source/WebCore/platform/qt/DeviceOrientationQt.cpp:37: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] Source/WebCore/platform/qt/DeviceOrientationQt.cpp:38: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] Source/WebCore/platform/qt/DeviceOrientationQt.cpp:39: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] Source/WebCore/platform/qt/DeviceOrientationQt.cpp:42: Place brace on its own line for function definitions. [whitespace/braces] [4] Source/WebCore/platform/qt/DeviceOrientationQt.cpp:43: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] Source/WebCore/platform/qt/DeviceOrientationQt.cpp:44: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] Source/WebCore/platform/qt/DeviceOrientationQt.cpp:45: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] Source/WebCore/platform/qt/DeviceOrientationQt.cpp:46: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] Source/WebCore/platform/chromium/DeviceMotionChromium.cpp:35: Place brace on its own line for function definitions. [whitespace/braces] [4] Source/WebCore/platform/chromium/DeviceMotionChromium.cpp:36: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] Source/WebCore/platform/chromium/DeviceMotionChromium.cpp:39: Place brace on its own line for function definitions. [whitespace/braces] [4] Source/WebCore/platform/chromium/DeviceMotionChromium.cpp:40: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] Source/WebCore/dom/DeviceOrientationController.cpp:47: Place brace on its own line for function definitions. [whitespace/braces] [4] Source/WebCore/dom/DeviceOrientationController.cpp:48: Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons. [readability/comparison_to_zero] [5] Source/WebCore/dom/DeviceOrientationController.cpp:50: One line control clauses should not use braces. [whitespace/braces] [4] Source/WebCore/dom/DeviceOrientationController.cpp:59: Should have a space between // and comment [whitespace/comments] [4] Source/WebCore/dom/DeviceOrientationController.cpp:65: One space before end of line comments [whitespace/comments] [5] Source/WebCore/dom/DeviceOrientationController.cpp:65: ShouldFailed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/Platform/ChangeLog', u'Source/Platform/chromium/public/Platform.h', u'Source/WebCore/ChangeLog', u'Source/WebCore/Target.pri', u'Source/WebCore/WebCore.gypi', u'Source/WebCore/dom/DeviceMotionController.cpp', u'Source/WebCore/dom/DeviceMotionController.h', u'Source/WebCore/dom/DeviceOrientationController.cpp', u'Source/WebCore/dom/DeviceOrientationController.h', u'Source/WebCore/history/PageCache.cpp', u'Source/WebCore/inspector/InspectorPageAgent.cpp', u'Source/WebCore/page/DOMWindow.cpp', u'Source/WebCore/page/DOMWindow.h', u'Source/WebCore/page/DeviceController.cpp', u'Source/WebCore/page/DeviceController.h', u'Source/WebCore/platform/PlatformDeviceMotion.h', u'Source/WebCore/platform/PlatformDeviceOrientation.h', u'Source/WebCore/platform/chromium/DeviceMotionChromium.cpp', u'Source/WebCore/platform/chromium/DeviceOrientationChromium.cpp', u'Source/WebCore/platform/qt/DeviceMotionProviderQt.cpp', u'Source/WebCore/platform/qt/DeviceMotionProviderQt.h', u'Source/WebCore/platform/qt/DeviceMotionQt.cpp', u'Source/WebCore/platform/qt/DeviceOrientationProviderQt.cpp', u'Source/WebCore/platform/qt/DeviceOrientationProviderQt.h', u'Source/WebCore/platform/qt/DeviceOrientationQt.cpp', u'Source/WebKit/chromium/ChangeLog', u'Source/WebKit/chromium/src/WebDeviceOrientationController.cpp', u'Source/WebKit/chromium/src/WebViewImpl.cpp']" exit_code: 1 have a space between // and comment [whitespace/comments] [4] Source/WebCore/dom/DeviceOrientationController.cpp:74: Place brace on its own line for function definitions. [whitespace/braces] [4] Source/WebCore/dom/DeviceOrientationController.cpp:79: Place brace on its own line for function definitions. [whitespace/braces] [4] Total errors found: 59 in 28 files If any of these errors are false positives, please file a bug against check-webkit-style.
Comment on attachment 195542 [details] Patch Attachment 195542 [details] did not pass efl-ews (efl): Output: http://webkit-commit-queue.appspot.com/results/17320323
Comment on attachment 195542 [details] Patch Attachment 195542 [details] did not pass gtk-ews (gtk): Output: http://webkit-commit-queue.appspot.com/results/17358014
Comment on attachment 195542 [details] Patch Attachment 195542 [details] did not pass qt-ews (qt): Output: http://webkit-commit-queue.appspot.com/results/17353086
Comment on attachment 195542 [details] Patch Attachment 195542 [details] did not pass qt-wk2-ews (qt): Output: http://webkit-commit-queue.appspot.com/results/17320324
Comment on attachment 195542 [details] Patch Attachment 195542 [details] did not pass cr-linux-debug-ews (chromium-xvfb): Output: http://webkit-commit-queue.appspot.com/results/17351141
Created attachment 195548 [details] Patch
Attachment 195548 [details] did not pass style-queue: Source/WebCore/page/DeviceController.cpp:54: Should have a space between // and comment [whitespace/comments] [4] Source/WebCore/page/DeviceController.cpp:64: Should have a space between // and comment [whitespace/comments] [4] Source/WebCore/page/DeviceController.cpp:74: Should have a space between // and comment [whitespace/comments] [4] Source/WebCore/platform/chromium/DeviceOrientationChromium.cpp:27: Alphabetical sorting problem. [build/include_order] [4] Source/WebCore/platform/chromium/DeviceOrientationChromium.cpp:40: Place brace on its own line for function definitions. [whitespace/braces] [4] Source/WebCore/platform/chromium/DeviceOrientationChromium.cpp:41: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] Source/WebCore/platform/chromium/DeviceOrientationChromium.cpp:42: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] Source/WebCore/platform/chromium/DeviceOrientationChromium.cpp:43: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] Source/WebCore/platform/chromium/DeviceOrientationChromium.cpp:44: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] Source/WebCore/platform/chromium/DeviceOrientationChromium.cpp:47: Place brace on its own line for function definitions. [whitespace/braces] [4] Source/WebCore/platform/chromium/DeviceOrientationChromium.cpp:48: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] Source/WebCore/platform/chromium/DeviceOrientationChromium.cpp:49: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] Source/WebCore/platform/chromium/DeviceOrientationChromium.cpp:50: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] Source/WebCore/platform/chromium/DeviceOrientationChromium.cpp:51: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] Source/WebCore/platform/qt/DeviceMotionProviderQt.cpp:39: Place brace on its own line for function definitions. [whitespace/braces] [4] Source/WebCore/platform/qt/DeviceMotionProviderQt.cpp:42: One line control clauses should not use braces. [whitespace/braces] [4] Source/WebCore/platform/qt/DeviceOrientationProviderQt.cpp:34: Place brace on its own line for function definitions. [whitespace/braces] [4] Source/WebCore/platform/qt/DeviceOrientationProviderQt.cpp:37: One line control clauses should not use braces. [whitespace/braces] [4] Source/WebCore/dom/DeviceMotionController.cpp:43: Place brace on its own line for function definitions. [whitespace/braces] [4] Source/WebCore/dom/DeviceMotionController.cpp:44: Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons. [readability/comparison_to_zero] [5] Source/WebCore/dom/DeviceMotionController.cpp:46: One line control clauses should not use braces. [whitespace/braces] [4] Source/WebCore/dom/DeviceMotionController.cpp:59: Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons. [readability/comparison_to_zero] [5] Source/WebCore/dom/DeviceMotionController.cpp:67: Place brace on its own line for function definitions. [whitespace/braces] [4] Source/WebCore/dom/DeviceMotionController.cpp:71: Place brace on its own line for function definitions. [whitespace/braces] [4] Source/WebCore/platform/qt/DeviceMotionQt.cpp:27: Alphabetical sorting problem. [build/include_order] [4] Source/WebCore/platform/qt/DeviceMotionQt.cpp:35: Place brace on its own line for function definitions. [whitespace/braces] [4] Source/WebCore/platform/qt/DeviceMotionQt.cpp:36: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] Source/WebCore/platform/qt/DeviceMotionQt.cpp:37: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] Source/WebCore/platform/qt/DeviceMotionQt.cpp:38: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] Source/WebCore/platform/qt/DeviceMotionQt.cpp:39: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] Source/WebCore/platform/qt/DeviceMotionQt.cpp:40: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] Source/WebCore/platform/qt/DeviceMotionQt.cpp:43: Place brace on its own line for function definitions. [whitespace/braces] [4] Source/WebCore/platform/qt/DeviceMotionQt.cpp:44: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] Source/WebCore/platform/qt/DeviceMotionQt.cpp:45: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] Source/WebCore/platform/qt/DeviceMotionQt.cpp:46: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] Source/WebCore/platform/qt/DeviceMotionQt.cpp:47: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] Source/WebCore/platform/qt/DeviceOrientationQt.cpp:27: Alphabetical sorting problem. [build/include_order] [4] Source/WebCore/platform/qt/DeviceOrientationQt.cpp:35: Place brace on its own line for function definitions. [whitespace/braces] [4] Source/WebCore/platform/qt/DeviceOrientationQt.cpp:36: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] Source/WebCore/platform/qt/DeviceOrientationQt.cpp:37: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] Source/WebCore/platform/qt/DeviceOrientationQt.cpp:38: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] Source/WebCore/platform/qt/DeviceOrientationQt.cpp:39: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] Source/WebCore/platform/qt/DeviceOrientationQt.cpp:42: Place brace on its own line for function definitions. [whitespace/braces] [4] Source/WebCore/platform/qt/DeviceOrientationQt.cpp:43: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] Source/WebCore/platform/qt/DeviceOrientationQt.cpp:44: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] Source/WebCore/platform/qt/DeviceOrientationQt.cpp:45: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] Source/WebCore/platform/qt/DeviceOrientationQt.cpp:46: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] Source/WebCore/platform/chromium/DeviceMotionChromium.cpp:35: Place brace on its own line for function definitions. [whitespace/braces] [4] Source/WebCore/platform/chromium/DeviceMotionChromium.cpp:36: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] Source/WebCore/platform/chromium/DeviceMotionChromium.cpp:39: Place brace on its own line for function definitions. [whitespace/braces] [4] Source/WebCore/platform/chromium/DeviceMotionChromium.cpp:40: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] Source/WebCore/dom/DeviceOrientationController.cpp:47: Place brace on its own line for function definitions. [whitespace/braces] [4] Source/WebCore/dom/DeviceOrientationController.cpp:48: Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons. [readability/comparison_to_zero] [5] Source/WebCore/dom/DeviceOrientationController.cpp:50: One line control clauses should not use braces. [whitespace/braces] [4] Source/WebCore/dom/DeviceOrientationController.cpp:59: Should have a space between // and comment [whitespace/comments] [4] Source/WebCore/dom/DeviceOrientationController.cpp:65: One space before end of line comments [whitespace/comments] [5] Source/WebCore/dom/DeviceOrientationController.cpp:65: ShouldFailed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/Platform/ChangeLog', u'Source/Platform/chromium/public/Platform.h', u'Source/WebCore/ChangeLog', u'Source/WebCore/Target.pri', u'Source/WebCore/WebCore.gypi', u'Source/WebCore/dom/DeviceMotionController.cpp', u'Source/WebCore/dom/DeviceMotionController.h', u'Source/WebCore/dom/DeviceOrientationController.cpp', u'Source/WebCore/dom/DeviceOrientationController.h', u'Source/WebCore/history/PageCache.cpp', u'Source/WebCore/inspector/InspectorPageAgent.cpp', u'Source/WebCore/page/DOMWindow.cpp', u'Source/WebCore/page/DOMWindow.h', u'Source/WebCore/page/DeviceController.cpp', u'Source/WebCore/page/DeviceController.h', u'Source/WebCore/platform/PlatformDeviceMotion.h', u'Source/WebCore/platform/PlatformDeviceOrientation.h', u'Source/WebCore/platform/chromium/DeviceMotionChromium.cpp', u'Source/WebCore/platform/chromium/DeviceOrientationChromium.cpp', u'Source/WebCore/platform/qt/DeviceMotionClientQt.cpp', u'Source/WebCore/platform/qt/DeviceMotionProviderQt.cpp', u'Source/WebCore/platform/qt/DeviceMotionProviderQt.h', u'Source/WebCore/platform/qt/DeviceMotionQt.cpp', u'Source/WebCore/platform/qt/DeviceOrientationClientQt.cpp', u'Source/WebCore/platform/qt/DeviceOrientationProviderQt.cpp', u'Source/WebCore/platform/qt/DeviceOrientationProviderQt.h', u'Source/WebCore/platform/qt/DeviceOrientationQt.cpp', u'Source/WebKit/chromium/ChangeLog', u'Source/WebKit/chromium/src/WebDeviceOrientationController.cpp', u'Source/WebKit/chromium/src/WebViewImpl.cpp']" exit_code: 1 have a space between // and comment [whitespace/comments] [4] Source/WebCore/dom/DeviceOrientationController.cpp:74: Place brace on its own line for function definitions. [whitespace/braces] [4] Source/WebCore/dom/DeviceOrientationController.cpp:79: Place brace on its own line for function definitions. [whitespace/braces] [4] Total errors found: 59 in 30 files If any of these errors are false positives, please file a bug against check-webkit-style.
Comment on attachment 195548 [details] Patch Attachment 195548 [details] did not pass gtk-ews (gtk): Output: http://webkit-commit-queue.appspot.com/results/17320327
Comment on attachment 195548 [details] Patch Attachment 195548 [details] did not pass efl-ews (efl): Output: http://webkit-commit-queue.appspot.com/results/17358022
Comment on attachment 195548 [details] Patch Attachment 195548 [details] did not pass qt-ews (qt): Output: http://webkit-commit-queue.appspot.com/results/17358025
Comment on attachment 195548 [details] Patch Attachment 195548 [details] did not pass cr-linux-debug-ews (chromium-xvfb): Output: http://webkit-commit-queue.appspot.com/results/17298440
Comment on attachment 195548 [details] Patch Attachment 195548 [details] did not pass qt-wk2-ews (qt): Output: http://webkit-commit-queue.appspot.com/results/17329306
Created attachment 195553 [details] Patch
Comment on attachment 195553 [details] Patch Attachment 195553 [details] did not pass qt-ews (qt): Output: http://webkit-commit-queue.appspot.com/results/17340183
Comment on attachment 195553 [details] Patch Attachment 195553 [details] did not pass gtk-ews (gtk): Output: http://webkit-commit-queue.appspot.com/results/17322244
Created attachment 195558 [details] Patch
Comment on attachment 195558 [details] Patch Attachment 195558 [details] did not pass gtk-ews (gtk): Output: http://webkit-commit-queue.appspot.com/results/17353112
Comment on attachment 195558 [details] Patch Attachment 195558 [details] did not pass efl-ews (efl): Output: http://webkit-commit-queue.appspot.com/results/17292412
Comment on attachment 195558 [details] Patch Attachment 195558 [details] did not pass qt-ews (qt): Output: http://webkit-commit-queue.appspot.com/results/17351154
Comment on attachment 195558 [details] Patch Attachment 195558 [details] did not pass qt-wk2-ews (qt): Output: http://webkit-commit-queue.appspot.com/results/17320344
Created attachment 195564 [details] Patch
Comment on attachment 195564 [details] Patch Attachment 195564 [details] did not pass efl-ews (efl): Output: http://webkit-commit-queue.appspot.com/results/17320353
Comment on attachment 195564 [details] Patch Attachment 195564 [details] did not pass qt-ews (qt): Output: http://webkit-commit-queue.appspot.com/results/17311610
Comment on attachment 195564 [details] Patch Attachment 195564 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://webkit-commit-queue.appspot.com/results/17311605
Comment on attachment 195564 [details] Patch Attachment 195564 [details] did not pass qt-wk2-ews (qt): Output: http://webkit-commit-queue.appspot.com/results/17329334
Comment on attachment 195564 [details] Patch Attachment 195564 [details] did not pass cr-linux-debug-ews (chromium-xvfb): Output: http://webkit-commit-queue.appspot.com/results/17330332
Comment on attachment 195564 [details] Patch Attachment 195564 [details] did not pass win-ews (win): Output: http://webkit-commit-queue.appspot.com/results/17353135
Comment on attachment 195564 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=195564&action=review What is the goal of this refactoring? I realize that this is not marked for review yet, but too much in this patch looks questionable. > Source/WebCore/dom/DeviceMotionController.cpp:43 > +DeviceMotionController* DeviceMotionController::getInstance() This is wrong style - getXXX functions in WebKit return their results by reference as out arguments. The common name for this is shared(), and the common way to implement is through DEFINE_STATIC_LOCAL macro. > Source/WebCore/dom/DeviceMotionController.cpp:51 > +void DeviceMotionController::didChangeDeviceMotion(PassRefPtr<DeviceMotionData> deviceMotionData) There is no ownership passing here, so it's incorrect to use PassRefPtr. A plain pointer should be used. The purpose of PassRefPtr is to avoid reference count thrashing. It also gives a little protection against leaks when returning results from functions. It's not the goal to use it everywhere. > Source/WebCore/dom/DeviceMotionController.cpp:60 > + // TODO(timvolodine): check how to implement this properly Are you planning to get rid of this before sending the patch for review? This is not a proper style for FIXME comments in WebKit. > Source/WebCore/dom/DeviceOrientationController.cpp:54 > +void DeviceOrientationController::didChangeDeviceOrientation(PassRefPtr<DeviceOrientationData> orientation) Ditto about PassRefPtr. > Source/WebCore/dom/DeviceOrientationController.cpp:57 > + LOG_ERROR(WTF::String::number(orientation.get()->alpha()).ascii().data()); > + LOG_ERROR(WTF::String::number(orientation.get()->canProvideGamma()).ascii().data()); Please don't prefix WTF::. > Source/WebCore/dom/DeviceOrientationController.cpp:59 > + // orientation = InspectorInstrumentation::overrideDeviceOrientation(m_page, orientation); Please remove commented out code before sending a patch for review. > Source/WebCore/dom/DeviceOrientationController.cpp:66 > + // TODO(timvolodine): figure out how to implement this > + // maybe : (m_lastOrientation.get() != 0); ... > Source/WebCore/dom/DeviceOrientationController.cpp:70 > PassRefPtr<Event> DeviceOrientationController::getLastEvent() Existing code, but this is very wrong, as explained in above comments. > Source/WebCore/page/DOMWindow.h:411 > + Page* page(); Did you check why this function was private? I suspect that another idiom could be preferred for getting the page. > Source/WebCore/page/DeviceController.h:41 > + explicit DeviceController(); "Explicit" is meaningless with this constructor, now that it has no arguments.
Comment on attachment 195564 [details] Patch Attachment 195564 [details] did not pass chromium-ews (chromium-xvfb): Output: http://webkit-commit-queue.appspot.com/results/17235780 New failing tests: accessibility/aria-checkbox-checked.html accessibility/aria-labelledby-on-input.html animations/3d/matrix-transform-type-animation.html http/tests/appcache/cyrillic-uri.html animations/animation-add-events-in-handler.html http/tests/appcache/deferred-events-delete-while-raising.html canvas/philip/tests/2d.clearRect.globalalpha.html accessibility/aria-describedby-on-input.html http/tests/appcache/crash-when-navigating-away-then-back.html animations/animation-direction-alternate-reverse.html accessibility/aria-labelledby-overrides-label.html http/tests/appcache/credential-url.html http/tests/appcache/access-via-redirect.php http/tests/appcache/different-scheme.html accessibility/aria-hidden.html http/tests/appcache/destroyed-frame.html http/tests/appcache/deferred-events-delete-while-raising-timer.html animations/animation-controller-drt-api.html accessibility/aria-hidden-hides-all-elements.html animations/3d/transform-perspective.html accessibility/aria-hidden-updates-alldescendants.html accessibility/accessibility-node-reparent.html canvas/philip/tests/2d.canvas.reference.html animations/3d/state-at-end-event-transform.html canvas/philip/tests/2d.clearRect.basic.html accessibility/aria-fallback-roles.html animations/animation-direction-reverse-fill-mode-hardware.html accessibility/adjacent-continuations-cause-assertion-failure.html http/tests/appcache/detached-iframe.html canvas/philip/tests/2d.clearRect+fillRect.alpha0.html
Created attachment 195609 [details] Archive of layout-test-results from gce-cr-linux-06 for chromium-linux-x86_64 The attached test failures were seen while running run-webkit-tests on the chromium-ews. Bot: gce-cr-linux-06 Port: chromium-linux-x86_64 Platform: Linux-3.3.8-gcg-201212281604-x86_64-with-GCEL-10.04-gcel_10.04
Comment on attachment 195564 [details] Patch Attachment 195564 [details] did not pass mac-ews (mac): Output: http://webkit-commit-queue.appspot.com/results/17345710
Created attachment 196175 [details] Patch
Comment on attachment 195564 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=195564&action=review Thanks Alexey, I addressed most of your comments, so now the code should look better. >> Source/WebCore/dom/DeviceMotionController.cpp:43 >> +DeviceMotionController* DeviceMotionController::getInstance() > > This is wrong style - getXXX functions in WebKit return their results by reference as out arguments. > > The common name for this is shared(), and the common way to implement is through DEFINE_STATIC_LOCAL macro. done >> Source/WebCore/dom/DeviceMotionController.cpp:51 >> +void DeviceMotionController::didChangeDeviceMotion(PassRefPtr<DeviceMotionData> deviceMotionData) > > There is no ownership passing here, so it's incorrect to use PassRefPtr. A plain pointer should be used. > > The purpose of PassRefPtr is to avoid reference count thrashing. It also gives a little protection against leaks when returning results from functions. It's not the goal to use it everywhere. done >> Source/WebCore/dom/DeviceMotionController.cpp:60 >> + // TODO(timvolodine): check how to implement this properly > > Are you planning to get rid of this before sending the patch for review? This is not a proper style for FIXME comments in WebKit. done >> Source/WebCore/dom/DeviceOrientationController.cpp:54 >> +void DeviceOrientationController::didChangeDeviceOrientation(PassRefPtr<DeviceOrientationData> orientation) > > Ditto about PassRefPtr. done >> Source/WebCore/dom/DeviceOrientationController.cpp:57 >> + LOG_ERROR(WTF::String::number(orientation.get()->canProvideGamma()).ascii().data()); > > Please don't prefix WTF::. done >> Source/WebCore/dom/DeviceOrientationController.cpp:59 >> + // orientation = InspectorInstrumentation::overrideDeviceOrientation(m_page, orientation); > > Please remove commented out code before sending a patch for review. done >> Source/WebCore/dom/DeviceOrientationController.cpp:66 >> + // maybe : (m_lastOrientation.get() != 0); > > ... done >> Source/WebCore/dom/DeviceOrientationController.cpp:70 >> PassRefPtr<Event> DeviceOrientationController::getLastEvent() > > Existing code, but this is very wrong, as explained in above comments. don't really see why this is wrong now >> Source/WebCore/page/DOMWindow.h:411 >> + Page* page(); > > Did you check why this function was private? I suspect that another idiom could be preferred for getting the page. hmm, at this point not sure how to get to the page from window in any other way. >> Source/WebCore/page/DeviceController.h:41 >> + explicit DeviceController(); > > "Explicit" is meaningless with this constructor, now that it has no arguments. done
The latest patch contains the WebCore side of the new device motion/orientation implementation. We decided not to include changes to ports for now, as this would make the patch much larger and difficult to review.
Comment on attachment 196175 [details] Patch Attachment 196175 [details] did not pass qt-ews (qt): Output: http://webkit-commit-queue.appspot.com/results/17352332
Comment on attachment 196175 [details] Patch Attachment 196175 [details] did not pass qt-wk2-ews (qt): Output: http://webkit-commit-queue.appspot.com/results/17404029
Comment on attachment 196175 [details] Patch Attachment 196175 [details] did not pass efl-ews (efl): Output: http://webkit-commit-queue.appspot.com/results/17367295
Comment on attachment 196175 [details] Patch Attachment 196175 [details] did not pass mac-ews (mac): Output: http://webkit-commit-queue.appspot.com/results/17320682
Comment on attachment 196175 [details] Patch Attachment 196175 [details] did not pass chromium-ews (chromium-xvfb): Output: http://webkit-commit-queue.appspot.com/results/17313523 New failing tests: inspector/device-orientation-success.html fast/dom/DeviceOrientation/no-synchronous-events.html fast/dom/DeviceOrientation/basic-operation.html fast/dom/DeviceOrientation/multiple-frames.html fast/dom/DeviceOrientation/null-values.html fast/dom/DeviceOrientation/updates.html fast/dom/DeviceOrientation/add-listener-from-callback.html
Created attachment 196199 [details] Archive of layout-test-results from gce-cr-linux-06 for chromium-linux-x86_64 The attached test failures were seen while running run-webkit-tests on the chromium-ews. Bot: gce-cr-linux-06 Port: chromium-linux-x86_64 Platform: Linux-3.3.8-gcg-201212281604-x86_64-with-GCEL-10.04-gcel_10.04
Comment on attachment 196175 [details] Patch Attachment 196175 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://webkit-commit-queue.appspot.com/results/17358457
Comment on attachment 196175 [details] Patch Attachment 196175 [details] did not pass win-ews (win): Output: http://webkit-commit-queue.appspot.com/results/17385124