Bug 43181

Summary: Hook up LayoutTestController.setMockDeviceOrientation() on Mac.
Product: WebKit Reporter: Steve Block <steveblock>
Component: WebKit APIAssignee: Nobody <webkit-unassigned>
Status: RESOLVED FIXED    
Severity: Normal CC: cmarrin, commit-queue, dino, eric, hans, simon.fraser, steveblock, webkit.review.bot
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Mac   
OS: All   
Bug Depends on: 39589    
Bug Blocks: 30335, 39590    
Attachments:
Description Flags
Patch
none
Patch
none
Patch
none
Patch none

Description Steve Block 2010-07-29 02:09:22 PDT
This new LayoutTestCotroller method is being added in Bug 39589
Comment 1 Steve Block 2010-07-29 15:22:48 PDT
Created attachment 62998 [details]
Patch
Comment 2 Steve Block 2010-07-29 15:29:27 PDT
Note that this patch will fail to apply, let alone build, until the Chromium problems are sorted out and the patch in Bug 39589 is re-landed.
Comment 3 Steve Block 2010-07-30 08:22:48 PDT
Created attachment 63060 [details]
Patch
Comment 4 WebKit Review Bot 2010-07-30 08:24:33 PDT
Attachment 63060 [details] did not pass style-queue:

Failed to run "['WebKitTools/Scripts/check-webkit-style']" exit_code: 1
WebKit/mac/WebView/WebDeviceOrientationInternal.h:39:  Extra space before ( in function call  [whitespace/parens] [4]
WebKit/mac/WebView/WebDeviceOrientationControllerInternal.h:39:  Extra space before ( in function call  [whitespace/parens] [4]
WebKit/mac/WebView/WebViewPrivate.h:610:  Extra space before ( in function call  [whitespace/parens] [4]
Total errors found: 3 in 22 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 5 Steve Block 2010-07-30 08:28:39 PDT
> Attachment 63060 [details] did not pass style-queue:
This is a false positive. Filed Bug 43252.
Comment 6 Eric Seidel (no email) 2010-07-30 08:31:37 PDT
Attachment 63060 [details] did not build on mac:
Build output: http://queues.webkit.org/results/3615342
Comment 7 Steve Block 2010-08-02 05:26:31 PDT
Created attachment 63195 [details]
Patch
Comment 8 WebKit Review Bot 2010-08-02 05:27:52 PDT
Attachment 63195 [details] did not pass style-queue:

Failed to run "['WebKitTools/Scripts/check-webkit-style']" exit_code: 1
WebKit/mac/WebView/WebDeviceOrientationControllerInternal.h:39:  Extra space before ( in function call  [whitespace/parens] [4]
WebKit/mac/WebView/WebViewPrivate.h:610:  Extra space before ( in function call  [whitespace/parens] [4]
WebKit/mac/WebView/WebDeviceOrientationInternal.h:39:  Extra space before ( in function call  [whitespace/parens] [4]
Total errors found: 3 in 21 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 9 Steve Block 2010-08-10 12:46:18 PDT
Dean, do you know anybody who could review this? It's been in the queue for over a week now.
Comment 10 Dean Jackson 2010-08-11 04:06:03 PDT
Chris, can you look at this with your new reviewing power?
Comment 11 Steve Block 2010-08-18 04:58:01 PDT
Ping!
Comment 12 Simon Fraser (smfr) 2010-08-18 14:36:05 PDT
Comment on attachment 63195 [details]
Patch

I'm a little surprised that there are so many classes here: WebDeviceOrientationClient, WebDeviceOrientation, WebDeviceOrientationInternal , WebDeviceOrientationInternal, WebDeviceOrientationController, WebDeviceOrientationControllerInternal, WebDeviceOrientationProvider, WebDeviceOrientationProviderMock and WebDeviceOrientationProviderMockInternal. Are they really all needed?
Comment 13 Steve Block 2010-08-19 07:56:41 PDT
Created attachment 64853 [details]
Patch
Comment 14 WebKit Review Bot 2010-08-19 07:58:13 PDT
Attachment 64853 [details] did not pass style-queue:

Failed to run "['WebKitTools/Scripts/check-webkit-style']" exit_code: 1
WebKit/mac/WebView/WebViewPrivate.h:609:  Extra space before ( in function call  [whitespace/parens] [4]
WebKit/mac/WebView/WebDeviceOrientationProviderMockInternal.h:46:  Extra space before ( in function call  [whitespace/parens] [4]
WebKit/mac/WebView/WebDeviceOrientationInternal.h:39:  Extra space before ( in function call  [whitespace/parens] [4]
Total errors found: 3 in 19 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 15 Steve Block 2010-08-19 07:59:00 PDT
Thanks for the review Simon. The reason for the numerous classes is that we're  implementing a WebCore client interface for which we need both a real and a mock implementation.

I've uploaded a new patch which eliminates the need for WebDeviceOrientationController by doing a runtime check. I think all of the other classes are required. I'm no expert on Mac, but I've followed the pattern used by Geolocation. Here's a summary of the new classes ...

- WebDeviceOrientationClient - Mac implementation of WebCore::DeviceOrientationClient. Acts a proxy to either the mock or real provider.
- WebDeviceOrientationProvider - New interface used by WebDeviceOrientationClient and implemented by the real and mock providers.
- WebDeviceOrientationProviderMock - Mock provider, wrapper around WebCore::DeviceOrientationClientMock
- WebDeviceOrientation - Wrapper around WebCore::DeviceOrientation. Required by DRT to configure the mock, without exposing WebCore types.

Where these classes are wrappers around WebCore types, I've used a FooInternal internal class to hide the WebCore types from the WebKit API.
Comment 16 Adam Barth 2010-08-31 20:02:41 PDT
Comment on attachment 64853 [details]
Patch

This patch might be a bit outside my area (since I don't really have more than a passing understanding of ObjectiveC), but this looks reasonable to me.
Comment 17 Steve Block 2010-09-01 01:04:10 PDT
Thanks for the review Adam.

Unless anybody else has comments, I'll submit tomorrow, as this has now been in the review queue for some time.
Comment 18 WebKit Commit Bot 2010-09-02 14:23:50 PDT
Comment on attachment 64853 [details]
Patch

Clearing flags on attachment: 64853

Committed r66685: <http://trac.webkit.org/changeset/66685>
Comment 19 WebKit Commit Bot 2010-09-02 14:23:56 PDT
All reviewed patches have been landed.  Closing bug.