Bug 113499

Summary: WebCore refactoring of device motion/orientation, to use the platform-based approach
Product: WebKit Reporter: timvolodine
Component: New BugsAssignee: Nobody <webkit-unassigned>
Status: UNCONFIRMED ---    
Severity: Normal CC: abarth, buildbot, bulach, dglazkov, esprehn+autocc, fishd, gtk-ews, jamesr, miguelg, ojan.autocc, peter, philn, rego+ews, rniwa, tkent+wkapi, webkit.review.bot, xan.lopez
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Patch
none
Archive of layout-test-results from gce-cr-linux-06 for chromium-linux-x86_64
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Archive of layout-test-results from gce-cr-linux-06 for chromium-linux-x86_64
none
Patch
webkit-ews: commit-queue-
Archive of layout-test-results from gce-cr-linux-06 for chromium-linux-x86_64 none

Description timvolodine 2013-03-28 04:51:51 PDT 
WebCore refactoring of device motion/orientation and fixing of webkit ports
Comment 1 timvolodine 2013-03-28 05:11:18 PDT 
Created attachment 195536 [details]
Patch
Comment 2 WebKit Review Bot 2013-03-28 05:13:49 PDT 
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.
Comment 3 WebKit Review Bot 2013-03-28 05:14:05 PDT 
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 4 EFL EWS Bot 2013-03-28 05:18:29 PDT 
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 5 Early Warning System Bot 2013-03-28 05:19:06 PDT 
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 6 Early Warning System Bot 2013-03-28 05:21:18 PDT 
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 7 WebKit Review Bot 2013-03-28 05:24:47 PDT 
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 8 Build Bot 2013-03-28 05:42:03 PDT 
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 9 kov's GTK+ EWS bot 2013-03-28 05:44:47 PDT 
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 10 WebKit Review Bot 2013-03-28 05:45:37 PDT 
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
Comment 11 WebKit Review Bot 2013-03-28 05:45:41 PDT 
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
Comment 12 timvolodine 2013-03-28 05:50:04 PDT 
Created attachment 195542 [details]
Patch
Comment 13 WebKit Review Bot 2013-03-28 05:54:05 PDT 
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 14 EFL EWS Bot 2013-03-28 05:57:26 PDT 
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 15 kov's GTK+ EWS bot 2013-03-28 05:59:40 PDT 
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 16 Early Warning System Bot 2013-03-28 06:00:19 PDT 
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 17 Early Warning System Bot 2013-03-28 06:00:23 PDT 
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 18 WebKit Review Bot 2013-03-28 06:03:52 PDT 
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
Comment 19 timvolodine 2013-03-28 06:05:54 PDT 
Created attachment 195548 [details]
Patch
Comment 20 WebKit Review Bot 2013-03-28 06:09:06 PDT 
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 21 kov's GTK+ EWS bot 2013-03-28 06:12:41 PDT 
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 22 EFL EWS Bot 2013-03-28 06:15:47 PDT 
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 23 Early Warning System Bot 2013-03-28 06:21:13 PDT 
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 24 WebKit Review Bot 2013-03-28 06:22:41 PDT 
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 25 Early Warning System Bot 2013-03-28 06:23:55 PDT 
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
Comment 26 timvolodine 2013-03-28 06:31:50 PDT 
Created attachment 195553 [details]
Patch
Comment 27 Early Warning System Bot 2013-03-28 06:39:57 PDT 
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 28 kov's GTK+ EWS bot 2013-03-28 06:40:24 PDT 
Comment on attachment 195553 [details]
Patch

Attachment 195553 [details] did not pass gtk-ews (gtk):
Output: http://webkit-commit-queue.appspot.com/results/17322244
Comment 29 timvolodine 2013-03-28 06:41:34 PDT 
Created attachment 195558 [details]
Patch
Comment 30 kov's GTK+ EWS bot 2013-03-28 06:48:54 PDT 
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 31 EFL EWS Bot 2013-03-28 06:49:01 PDT 
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 32 Early Warning System Bot 2013-03-28 06:53:40 PDT 
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 33 Early Warning System Bot 2013-03-28 06:57:42 PDT 
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
Comment 34 timvolodine 2013-03-28 07:00:20 PDT 
Created attachment 195564 [details]
Patch
Comment 35 EFL EWS Bot 2013-03-28 07:07:57 PDT 
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 36 Early Warning System Bot 2013-03-28 07:19:13 PDT 
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 37 Build Bot 2013-03-28 07:26:08 PDT 
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 38 Early Warning System Bot 2013-03-28 07:27:20 PDT 
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 39 WebKit Review Bot 2013-03-28 07:34:38 PDT 
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 40 Build Bot 2013-03-28 07:58:02 PDT 
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 41 Alexey Proskuryakov 2013-03-28 09:29:57 PDT 
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 42 WebKit Review Bot 2013-03-28 11:38:55 PDT 
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
Comment 43 WebKit Review Bot 2013-03-28 11:39:00 PDT 
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 44 Build Bot 2013-03-29 11:18:12 PDT 
Comment on attachment 195564 [details]
Patch

Attachment 195564 [details] did not pass mac-ews (mac):
Output: http://webkit-commit-queue.appspot.com/results/17345710
Comment 45 timvolodine 2013-04-02 10:35:15 PDT 
Created attachment 196175 [details]
Patch
Comment 46 timvolodine 2013-04-02 10:41:15 PDT 
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
Comment 47 timvolodine 2013-04-02 10:45:27 PDT 
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 48 Early Warning System Bot 2013-04-02 10:50:51 PDT 
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 49 Early Warning System Bot 2013-04-02 10:51:46 PDT 
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 50 EFL EWS Bot 2013-04-02 10:59:38 PDT 
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 51 Build Bot 2013-04-02 11:06:38 PDT 
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 52 WebKit Review Bot 2013-04-02 11:47:06 PDT 
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
Comment 53 WebKit Review Bot 2013-04-02 11:47:12 PDT 
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 54 Build Bot 2013-04-02 15:32:27 PDT 
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 55 Build Bot 2013-04-03 00:53:54 PDT 
Comment on attachment 196175 [details]
Patch

Attachment 196175 [details] did not pass win-ews (win):
Output: http://webkit-commit-queue.appspot.com/results/17385124