Bug 39589 - Add LayoutTestController methods to test DeviceOrientation
Summary: Add LayoutTestController methods to test DeviceOrientation
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebCore Misc. (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on: 42834 43237
Blocks: 30335 39590 43181
  Show dependency treegraph
 
Reported: 2010-05-24 03:21 PDT by Steve Block
Modified: 2010-07-30 07:17 PDT (History)
14 users (show)

See Also:


Attachments
Patch (20.75 KB, patch)
2010-07-16 03:26 PDT, Steve Block
no flags Details | Formatted Diff | Diff
Patch (20.22 KB, patch)
2010-07-16 04:27 PDT, Steve Block
no flags Details | Formatted Diff | Diff
Patch (68.93 KB, patch)
2010-07-23 01:20 PDT, Steve Block
no flags Details | Formatted Diff | Diff
Patch (67.46 KB, patch)
2010-07-23 10:19 PDT, Steve Block
no flags Details | Formatted Diff | Diff
Patch (82.24 KB, patch)
2010-07-28 09:35 PDT, Steve Block
no flags Details | Formatted Diff | Diff
Patch (30.45 KB, patch)
2010-07-29 02:31 PDT, Steve Block
no flags Details | Formatted Diff | Diff
Patch (34.67 KB, patch)
2010-07-29 03:00 PDT, Steve Block
no flags Details | Formatted Diff | Diff
Patch (34.43 KB, patch)
2010-07-29 03:17 PDT, Steve Block
no flags Details | Formatted Diff | Diff
Patch (38.22 KB, patch)
2010-07-30 03:13 PDT, Steve Block
no flags Details | Formatted Diff | Diff
Patch (38.31 KB, patch)
2010-07-30 05:45 PDT, Steve Block
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Steve Block 2010-05-24 03:21:08 PDT
These LayoutTestsController methods will be used to configure a mock DeviceOrientationClient to test DeviceOrientation
Comment 1 Steve Block 2010-07-16 03:26:34 PDT
Created attachment 61790 [details]
Patch
Comment 2 Steve Block 2010-07-16 03:54:45 PDT
This patch isn't complete but I'd value some feedback on the structure I've used to provide the mock implementation, as I don't think there's much precedent to follow. The only other instance of a mock that's configurable from LayoutTestController that I'm aware of is Geolocation, and that's currently in turmoil as we have two parallel implementations.

My intention is to provide a platform-independent mock for DeviceOrientationClient. This will ensure that all platforms use the same mock for consistency in testing. At the same time, the way in which the mock is inserted is left up to the platform.

Included in the patch is DeviceOrientationClientAndroid, to provide an example. It acts as a proxy to the real DeviceOrientationClient, which is owned by the WebViewCore. In normal use, the WebViewCore provides DeviceOrientationClientImpl, but provides DeviceOrientationClientMock when required in DumpRenderTree.

Currently, the decision of whether to use the mock is delayed until the corresponding LayoutTestController method is called. This allows LayoutTests which use both the real implementation and the mock implementation.

An alternative would be to have the WebViewCore simply provide either DeviceOrientationClientImpl or DeviceOrientationClientMock when instantiating the Page object, thus eliminating the DeviceOrientationClientAndroid proxy. The disadvantage of this approach is that the decision to use the mock would then have to be made at start-up time. DumpRenderTree would have to set the use of the mock when configuring the WebViewCore. This means that no layout tests can use the real DeviceOrientationClient implementation.

In reality, I think there are unlikely to be layout tests for DeviceOrientation (and other similarly mocked features) which need the real implementation, so I'm leaning towards the latter approach.

What do people think?

Once we have agreed on the approach, I'll complete the patch. Is it required to provide a complete working implementation for Mac, even though the feature is not enabled for Mac?
Comment 3 WebKit Review Bot 2010-07-16 04:01:30 PDT
Attachment 61790 [details] did not build on gtk:
Build output: http://webkit-commit-queue.appspot.com/results/3562058
Comment 4 Steve Block 2010-07-16 04:27:48 PDT
Created attachment 61793 [details]
Patch
Comment 5 WebKit Review Bot 2010-07-16 04:36:28 PDT
Attachment 61793 [details] did not build on gtk:
Build output: http://webkit-commit-queue.appspot.com/results/3560053
Comment 6 Steve Block 2010-07-16 06:56:47 PDT
> An alternative would be to have the WebViewCore simply provide either
> DeviceOrientationClientImpl or DeviceOrientationClientMock when instantiating
> the Page object, thus eliminating the DeviceOrientationClientAndroid proxy. The
> disadvantage of this approach is that the decision to use the mock would then
> have to be made at start-up time. DumpRenderTree would have to set the use of
> the mock when configuring the WebViewCore. This means that no layout tests can
> use the real DeviceOrientationClient implementation.
Looking at the Mac implementation of WebView and DumpRenderTree, the page object is instantiated immediately from WebView.initWithFrame. So if we're to add the ability to configure the WebView to use the mock DeviceOrientationClient, we need to either ...
- Add 'useMockDeviceOrientationClient' parameter to WebView.initWithFrame.
- Add a WebView.useMockDeviceOrientationClient method, and modify Page to use a new setDeviceOrientationClient method, rather than the DeviceOrientationClient constructor argument.

This former approach isn't scalable. The latter is, and by removing the DeviceOrientationClient argument from the Page constructor, avoids the scalability problem there too. Note that this approach was taken for the SpeechInputClient in Bug 41518.

I think it's probably best to make the change to add Page.setDeviceOrientationClient. What do you think?
Comment 7 Satish Sampath 2010-07-16 07:16:18 PDT
Not a review, just a drive-by comment:

> This former approach isn't scalable. The latter is, and by removing the DeviceOrientationClient argument from the Page constructor, avoids the scalability problem there too. Note that this approach was taken for the SpeechInputClient in Bug 41518.
> 
> I think it's probably best to make the change to add Page.setDeviceOrientationClient. What do you think?

Yes this was the reason I chose a setter for the client pointer in the speech input patch. The Page constructor is quite overloaded with 9 parameters now and many of them are used in only few of the ports. A setter would clean it up, while allowing the webkit layer to switch to the mock when necessary.
Comment 8 WebKit Review Bot 2010-07-16 07:46:33 PDT
Attachment 61793 [details] did not build on win:
Build output: http://webkit-commit-queue.appspot.com/results/3580054
Comment 9 Hans Wennborg 2010-07-16 09:44:44 PDT
(In reply to comment #7)
> Not a review, just a drive-by comment:
> 
> > This former approach isn't scalable. The latter is, and by removing the DeviceOrientationClient argument from the Page constructor, avoids the scalability problem there too. Note that this approach was taken for the SpeechInputClient in Bug 41518.
> > 
> > I think it's probably best to make the change to add Page.setDeviceOrientationClient. What do you think?
> 
> Yes this was the reason I chose a setter for the client pointer in the speech input patch. The Page constructor is quite overloaded with 9 parameters now and many of them are used in only few of the ports. A setter would clean it up, while allowing the webkit layer to switch to the mock when necessary.

I agree with Satish's comment. It makes some sense to have these members initialized to null, and allow them to be set by embedders that implement them. This would be a nice bonus on top of being able to easily inject a mock implementation.

One downside would be that doing lazy initialization (which we mentioned in Bug 42265) of these members becomes more complicated. But, as was also pointed out in that discussion, these objects are very light-weight, so I don't think it's an important thing to do.
Comment 10 Steve Block 2010-07-21 12:48:16 PDT
> A setter would clean it up, while allowing the webkit layer to switch to the
> mock when necessary.
The more I think about it, the more I think that the argument about providing a mock isn't a good one. In all cases, (assuming the feature is enabled) we should provide a non-null client before the feature can be used ...
- For normal use, we provide the real client
- For DRT when the mock is always used, we provide the mock
- For DRT when a mock is injected in response to LayoutTestController method, we should use a proxy client that can handle switching between the real implementation and the mock

All of these can be achieved by passing a client to the constructor, without the need for a client setter method.

I think that an approach whereby the client is replaced with a mock in response to a LayoutTestController method, using a second call to a setter, is somewhat abusing the idea of a client.

I think the advantage of a setter method is that it avoids the long list of arguments to the page constructor.
Comment 11 Satish Sampath 2010-07-21 12:53:37 PDT
There are many instances in WebKit/DRT codebase where Page() is constructed with many of the clients set to 0. All those are valid scenarios, in some cases the particular feature is not enabled and in other cases not implemented for the platform (e.g. client based geolocation only on few platforms). Wouldn't a setter make this relationship clearer, since the optional client parameters could be set by the platforms which have these features enabled and the rest can safely ignore without passing 0 ?
Comment 12 Steve Block 2010-07-21 13:00:24 PDT
> There are many instances in WebKit/DRT codebase where Page() is constructed
> with many of the clients set to 0. All those are valid scenarios, in some cases
> the particular feature is not enabled and in other cases not implemented for
> the platform (e.g. client based geolocation only on few platforms). Wouldn't a
> setter make this relationship clearer, since the optional client parameters
> could be set by the platforms which have these features enabled and the rest
> can safely ignore without passing 0 ?

Agreed.

See also the webkit-dev thread 'Clients and the Page constructor'
Comment 13 Steve Block 2010-07-23 01:20:44 PDT
Created attachment 62392 [details]
Patch
Comment 14 Steve Block 2010-07-23 01:21:34 PDT
Comment on attachment 62392 [details]
Patch

Not yet ready for review
Comment 15 Steve Block 2010-07-23 10:19:36 PDT
Created attachment 62442 [details]
Patch
Comment 16 Steve Block 2010-07-23 10:32:17 PDT
The latest patch reflects my current thinking on the best way to add a mock for testing in a way that allows the mock to be shared between platforms. I've provided the new LayoutTestController methods, a common mock, and the plumbing for Mac. The patch isn't in a state that's ready for submission, but should demonstrate the idea.

The mock client implementation lives in WebCore/platform/mock/. Each platform's WebView passes a client implementation to the Page. In DRT, this implementation is the mock. Both the client interface and the mock will have to be exposed in WebKit, so WebKit wrappers are needed. The wrappers can be (largely) platform-independent, so are provided to prevent (most) platforms from having to replicate this wrapping. These are currently in a new WebKit/mock directory.

On Mac, because WebKit types are ObjectiveC, the shared WebKit wrappers can not be used. Instead, Mac provides its own wrappers around the WebCore types. Any platform is able to do this if need be.

Note that the changes to WebView.mm obviously aren't suitable for submission, as WebDeviceOrientationClient is added as a parameter to the main init method. The problem is that the Mac WebView instantiates the Page object directly from the init method, so a setter for the WebDeviceOrientationClient can't be used. One alternative is to modify the Page object to set the client with a setter, but this creates the problem of a time window in which the client is not set. Another possibility is for Mac to use proxy client, which can be later configured to use either the real or mock client, but this adds the need for more plumbing.
Comment 17 Satish Sampath 2010-07-26 08:45:13 PDT
The file WebGeolocationclient.h is present under WebKit/mock, even though the interface defined seems to be useful for both the mock and the real implementation. How about moving it to somewhere like WebKit/common?

In fact a slightly better layout for the mock and common code in WebKit may be
  WebKit/common/public : platform independent public headers like the one above
  WebKit/common/src : platform independent .cpp/.h internal to WebKit
The mock headers could go under /public and implementation could go under /src. Or if you prefer there could be a /mock directory under /public and /src.
Comment 18 Satish Sampath 2010-07-27 07:46:21 PDT
Looks like WebDeviceOrientationClient is used only decide whether to use the real Webcore client or a mock client. To handle the Webcore client methods startUpdating() etc, you'd need to add more methods to WebDeviceOrientationClient or define a new interface which needs to be implemented by the WebKit embedder.

To simplify things, how about making DRT set a flag in WebKit to create mock objects instead of real ones? That way the embedder doesn't need to know the implementation (real or mock) of these objects. Access to the mock objects can be via an exported/public interface in WebKit and layoutTestController could call WebView after initialization to get a struct filled with all the mock object pointers.
Comment 19 Adam Barth 2010-07-27 08:35:22 PDT
Comment on attachment 62442 [details]
Patch

There'a another patch up for review that uses a mock client.  Please coordinate so we don't end up with a mess of where the mocks go.
Comment 20 Steve Block 2010-07-27 08:38:19 PDT
Hi Adam. Are you referring to Satish's mock for SpeechInput? I'm aware of that one and we're coordinating, but I'd appreciate your comments on the approach here.
Comment 21 Steve Block 2010-07-27 10:59:30 PDT
> To handle the Webcore client methods startUpdating() etc, you'd need to add
> more methods to WebDeviceOrientationClient or define a new interface which
> needs to be implemented by the WebKit embedder.
Exactly. My intent was the latter. Each platform would provide a WebDeviceOrientationClientFoo class for the 'real' client that implements WebDeviceOrientationClient. This is exactly the same as each platform does for other types that it exposes in its WebKit API. The only difference is that because both the 'real' implementation and the mock both implement WebDeviceOrientationClient, the mock can be switched in for testing.

> To simplify things, how about making DRT set a flag in WebKit to create mock
> objects instead of real ones? That way the embedder doesn't need to know the
> implementation (real or mock) of these objects. Access to the mock objects can
> be via an exported/public interface in WebKit and layoutTestController could
> call WebView after initialization to get a struct filled with all the mock
> object pointers.
Yes, you could do this. But as you point out, DRT still needs to get at the mock implementation to configure it, so each platform's WebKit layer would have to expose an interface for this mock. Also, it puts testing logic in the WebView layer, rather than keeping it in DRT. With my proposal, although the mock is exposed by the WebKit layer for use by the embedder, it doesn't add the need for any test logic to WebKit.
Comment 22 Steve Block 2010-07-28 09:35:48 PDT
Created attachment 62834 [details]
Patch
Comment 23 WebKit Review Bot 2010-07-28 09:39:35 PDT
Attachment 62834 [details] did not pass style-queue:

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


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 24 Early Warning System Bot 2010-07-28 09:52:08 PDT
Attachment 62834 [details] did not build on qt:
Build output: http://queues.webkit.org/results/3634092
Comment 25 Eric Seidel 2010-07-28 10:10:24 PDT
Attachment 62834 [details] did not build on mac:
Build output: http://queues.webkit.org/results/3571594
Comment 26 Steve Block 2010-07-29 02:31:58 PDT
Created attachment 62930 [details]
Patch
Comment 27 Early Warning System Bot 2010-07-29 02:37:54 PDT
Attachment 62930 [details] did not build on qt:
Build output: http://queues.webkit.org/results/3571621
Comment 28 Eric Seidel 2010-07-29 02:39:01 PDT
Attachment 62930 [details] did not build on mac:
Build output: http://queues.webkit.org/results/3576627
Comment 29 Steve Block 2010-07-29 03:00:31 PDT
Created attachment 62931 [details]
Patch
Comment 30 Eric Seidel 2010-07-29 03:06:09 PDT
Attachment 62931 [details] did not build on mac:
Build output: http://queues.webkit.org/results/3597557
Comment 31 Steve Block 2010-07-29 03:17:43 PDT
Created attachment 62934 [details]
Patch
Comment 32 Jeremy Orlow 2010-07-29 03:39:54 PDT
Comment on attachment 62934 [details]
Patch

WebKitTools/DumpRenderTree/chromium/LayoutTestController.cpp:1319
 +  }
ASSERT_NOT_IMPLEMENTED();

WebKitTools/DumpRenderTree/gtk/LayoutTestControllerGtk.cpp:448
 +      // See https://bugs.webkit.org/show_bug.cgi?id=30335.
ditto

WebKitTools/DumpRenderTree/mac/LayoutTestControllerMac.mm:326
 +      // See https://bugs.webkit.org/show_bug.cgi?id=30335.
ditto

WebKitTools/DumpRenderTree/qt/LayoutTestControllerQt.cpp:690
 +  }
ditto

WebKitTools/DumpRenderTree/win/LayoutTestControllerWin.cpp:381
 +      // See https://bugs.webkit.org/show_bug.cgi?id=30335.
ditto

WebKitTools/DumpRenderTree/wx/LayoutTestControllerWx.cpp:277
 +      // See https://bugs.webkit.org/show_bug.cgi?id=30335.
ditto


r=me
Comment 33 Steve Block 2010-07-29 04:10:04 PDT
Comment on attachment 62934 [details]
Patch

After discussion with jorlow, decided that asserting isn't appropriate and that the precedent here is a FIXME.
Comment 34 WebKit Commit Bot 2010-07-29 04:41:47 PDT
Comment on attachment 62934 [details]
Patch

Clearing flags on attachment: 62934

Committed r64270: <http://trac.webkit.org/changeset/64270>
Comment 35 WebKit Commit Bot 2010-07-29 04:41:55 PDT
All reviewed patches have been landed.  Closing bug.
Comment 36 Jeremy Orlow 2010-07-29 11:23:26 PDT
Steve:

This is causing a lot of problems for us in Chromium land.  I know it's not really procedure since this didn't break anything on an official builder, but I think (hope) you won't mind if we revert this temporarily reverting this.

Hans: in a local Chromium client, please revert the revert (git revert <the change>) and run the ui tests and unit tests.  It's probably easy fixes, but I'm not sure what to do for now.

Steve, we'll try to make sure this gets back in tomorrow or we'll just disable device orientation in Chromium for now.
Comment 37 Victor Wang 2010-07-29 12:11:09 PDT
Reverted r64270 for reason:

The patch breaks chromium webkit unittest

Committed r64299: <http://trac.webkit.org/changeset/64299>
Comment 38 Hans Wennborg 2010-07-30 02:56:29 PDT
(In reply to comment #36)
> Steve:
> 
> This is causing a lot of problems for us in Chromium land.  I know it's not really procedure since this didn't break anything on an official builder, but I think (hope) you won't mind if we revert this temporarily reverting this.
> 
> Hans: in a local Chromium client, please revert the revert (git revert <the change>) and run the ui tests and unit tests.  It's probably easy fixes, but I'm not sure what to do for now.
> 
> Steve, we'll try to make sure this gets back in tomorrow or we'll just disable device orientation in Chromium for now.

The problem in Chromium is that for some code paths, the runtime flag for device orientation is accidentally left on even though no DeviceOrientationClient is provided.

Creating Bug 43237 to explicitly set the flag to off in chromium until we have a proper implementation.

After that is landed, this patch should be re-landed.

Sorry for the inconvenience.
Comment 39 Steve Block 2010-07-30 03:13:00 PDT
Created attachment 63042 [details]
Patch
Comment 40 Steve Block 2010-07-30 03:14:23 PDT
Updated patch adds new mock files to build files for all platforms, which I probably should have done before.

Should not be landed until Bug 43237 is fixed.
Comment 41 Jeremy Orlow 2010-07-30 03:25:29 PDT
Comment on attachment 63042 [details]
Patch

r=me
Comment 42 WebKit Commit Bot 2010-07-30 05:35:32 PDT
Comment on attachment 63042 [details]
Patch

Rejecting patch 63042 from commit-queue.

Failed to run "[u'/Users/eseidel/Projects/CommitQueue/WebKitTools/Scripts/svn-apply', u'--reviewer', u'Jeremy Orlow', u'--force']" exit_code: 1
Last 500 characters of output:
TestControllerWin.cpp
patching file WebKitTools/DumpRenderTree/wx/LayoutTestControllerWx.cpp
patching file WebKitTools/Scripts/build-webkit
patching file LayoutTests/ChangeLog
Hunk #1 succeeded at 1 with fuzz 3.
patching file LayoutTests/fast/dom/DeviceOrientation/basic-operation-expected.txt
patching file LayoutTests/fast/dom/DeviceOrientation/basic-operation.html
patching file LayoutTests/fast/dom/DeviceOrientation/script-tests/basic-operation.js
patching file LayoutTests/platform/gtk/Skipped

Full output: http://queues.webkit.org/results/3621277
Comment 43 Steve Block 2010-07-30 05:45:58 PDT
Created attachment 63052 [details]
Patch
Comment 44 Steve Block 2010-07-30 06:02:48 PDT
Comment on attachment 63052 [details]
Patch

rebased
Comment 45 WebKit Commit Bot 2010-07-30 07:16:53 PDT
Comment on attachment 63052 [details]
Patch

Clearing flags on attachment: 63052

Committed r64356: <http://trac.webkit.org/changeset/64356>
Comment 46 WebKit Commit Bot 2010-07-30 07:17:05 PDT
All reviewed patches have been landed.  Closing bug.