| Summary: | Add LayoutTestController methods to test DeviceOrientation | ||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| Product: | WebKit | Reporter: | Steve Block <steveblock> | ||||||||||||||||||||||
| Component: | WebCore Misc. | Assignee: | Nobody <webkit-unassigned> | ||||||||||||||||||||||
| Status: | RESOLVED FIXED | ||||||||||||||||||||||||
| Severity: | Normal | CC: | andreip, bulach, commit-queue, eric, gustavo, hans, jorlow, morrita, satish, steveblock, victorw, webkit-ews, webkit.review.bot, xan.lopez | ||||||||||||||||||||||
| Priority: | P2 | ||||||||||||||||||||||||
| Version: | 528+ (Nightly build) | ||||||||||||||||||||||||
| Hardware: | All | ||||||||||||||||||||||||
| OS: | All | ||||||||||||||||||||||||
| Bug Depends on: | 42834, 43237 | ||||||||||||||||||||||||
| Bug Blocks: | 30335, 39590, 43181 | ||||||||||||||||||||||||
| Attachments: |
|
||||||||||||||||||||||||
|
Description
Steve Block
2010-05-24 03:21:08 PDT
Created attachment 61790 [details]
Patch
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? Attachment 61790 [details] did not build on gtk: Build output: http://webkit-commit-queue.appspot.com/results/3562058 Created attachment 61793 [details]
Patch
Attachment 61793 [details] did not build on gtk: Build output: http://webkit-commit-queue.appspot.com/results/3560053 > 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? 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.
Attachment 61793 [details] did not build on win: Build output: http://webkit-commit-queue.appspot.com/results/3580054 (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. > 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.
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 ? > 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'
Created attachment 62392 [details]
Patch
Comment on attachment 62392 [details]
Patch
Not yet ready for review
Created attachment 62442 [details]
Patch
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. 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. 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 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.
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. > 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. Created attachment 62834 [details]
Patch
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.
Attachment 62834 [details] did not build on qt: Build output: http://queues.webkit.org/results/3634092 Attachment 62834 [details] did not build on mac: Build output: http://queues.webkit.org/results/3571594 Created attachment 62930 [details]
Patch
Attachment 62930 [details] did not build on qt: Build output: http://queues.webkit.org/results/3571621 Attachment 62930 [details] did not build on mac: Build output: http://queues.webkit.org/results/3576627 Created attachment 62931 [details]
Patch
Attachment 62931 [details] did not build on mac: Build output: http://queues.webkit.org/results/3597557 Created attachment 62934 [details]
Patch
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 on attachment 62934 [details]
Patch
After discussion with jorlow, decided that asserting isn't appropriate and that the precedent here is a FIXME.
Comment on attachment 62934 [details] Patch Clearing flags on attachment: 62934 Committed r64270: <http://trac.webkit.org/changeset/64270> All reviewed patches have been landed. Closing bug. 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. Reverted r64270 for reason: The patch breaks chromium webkit unittest Committed r64299: <http://trac.webkit.org/changeset/64299> (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. Created attachment 63042 [details]
Patch
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 on attachment 63042 [details]
Patch
r=me
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 Created attachment 63052 [details]
Patch
Comment on attachment 63052 [details]
Patch
rebased
Comment on attachment 63052 [details] Patch Clearing flags on attachment: 63052 Committed r64356: <http://trac.webkit.org/changeset/64356> All reviewed patches have been landed. Closing bug. |