Bug 101452 – [chromium] Create proxy classes for WebViewClient and WebWidgetClient and use themn for layout tests

• Component: New Bugs
• Status: RESOLVED FIXED

[chromium] Create proxy classes for WebViewClient and WebWidgetClient and use themn for layout tests

Created attachment 172762 [details]
Patch

[WebKit Review Bot]

Please wait for approval from [e-mail], [e-mail], [e-mail], [e-mail] or [e-mail] before submitting, as this patch contains changes to the Chromium public API. See also https://trac.webkit.org/wiki/ChromiumWebKitAPI.

Created attachment 172764 [details]
script used to generate proxy classes

I generated the proxy classes using this script. Turns out that parsing a header in python is not really straight forward, so I guess we shouldn't use this script to automatically generate the proxy for now, but instead rely on people updating both classes at the same time for now.

[Adam Barth]

I don't understand why you need to modify the API to do this.

[Adam Barth]

This also seems like a large maintenance burden.

[James Robinson]

Comment on attachment 172762 [details]
Patch

Can't you just do this on the chromium side?  There is absolutely no way we'd be able to maintain this.

I don't think it's that much of a maintenance burden in the long term. If you change e.g. WebViewClient, you have to update three different implementations of this class (WebViewHost in DRT, WebViewHost in TestShell, and RenderViewImpl) already now, and in the future, at least the TestShell one will go away.

The goal of this design is to not incur any extra cost for non-layout test code paths.

The reason for putting this into WebKit instead of chrome is that it allows for implementing the testrunner related bits inside webkit - if somebody wanted to add a new method to the testRunner that requires some interaction with the WebViewClient, this design allows for implementing this entirely in webkit

[Darin Fisher (:fishd, Google)]

Adam, James:  I'll echo what Jochen wrote.  He and I discussed this recently.  

The idea is to make the TestRunner into a library that can be either embedded into DRT or content-shell.  TestRunner needs to implement WebViewClient to do its work, but RenderViewImpl (in Chromium) also needs to implement WebViewClient.  The idea is that RenderViewImpl would provide users with a way to override the WebViewClient.  content-shell would initiate the TestRunner library and use its WebViewClient instead.  The TestRunner's WebViewClient would need to delegate many calls to the real WebViewClient in order for content-shell to still function.

The challenge in overriding WebViewClient is maintenance.  If a method is added or removed, then the proxy in TestRunner needs to be updated.  TestRunner does not need to intercept all WebViewClient methods.  Many of the methods should just be passed through to the real WebViewClient.  It seemed like teasing the forwarding of methods out into a separate proxy interface would help with this, but I could also see us just incorporating the proxy interface directly into TestRunner.

Jochen, I think we should avoid the setter for WebViewClient.  DRT or RenderViewImpl can provide a mechanism for overriding the WebViewClient that they pass into WebKit when initializing a WebView.  I don't think we should need to support changing the WebViewClient at runtime.

[Adam Barth]

I'm happy to defer to fishd.  It seems like there should be a way to share a single vtable rather than having a vtable full of self-thunks.

(In reply to comment #9)
> I'm happy to defer to fishd.  It seems like there should be a way to share a single vtable rather than having a vtable full of self-thunks.

That would require hard-coding some knowledge about how the compiler will layout the vtable. While that's certainly possible, I'm not sure whether this is desirable.

[Adam Barth]

> That would require hard-coding some knowledge about how the compiler will layout the vtable. While that's certainly possible, I'm not sure whether this is desirable.

That's not necessarily true.  The subclassing mechanism in C++ lets you share vtables.

(In reply to comment #11)
> > That would require hard-coding some knowledge about how the compiler will layout the vtable. While that's certainly possible, I'm not sure whether this is desirable.
> 
> That's not necessarily true.  The subclassing mechanism in C++ lets you share vtables.

You're right

For that the proxy would need to be a template like

template<class client>
class WebViewClientProxy : public client {
public:
  //... interesting methods
};

Created attachment 172951 [details]
Patch

Much smaller version using templates

Created attachment 172956 [details]
Patch

Created attachment 173016 [details]
Patch

Created attachment 173046 [details]
Patch

This is how I'd hook this up https://codereview.chromium.org/11362161/

[Adam Barth]

Comment on attachment 173046 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=173046&action=review

> Tools/DumpRenderTree/chromium/TestRunner/public/WebTestProxy.h:55
> +    WebTestProxy(T t)

explicit

> Tools/DumpRenderTree/chromium/TestRunner/public/WebTestProxy.h:74
> +    virtual void postAccessibilityNotification(const WebKit::WebAccessibilityObject& obj, WebKit::WebAccessibilityNotification notification)

This function seems really long to put in an API.  Will all the overrides need to be in public headers?

(In reply to comment #19)
> (From update of attachment 173046 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=173046&action=review
> 
> > Tools/DumpRenderTree/chromium/TestRunner/public/WebTestProxy.h:55
> > +    WebTestProxy(T t)
> 
> explicit
> 
> > Tools/DumpRenderTree/chromium/TestRunner/public/WebTestProxy.h:74
> > +    virtual void postAccessibilityNotification(const WebKit::WebAccessibilityObject& obj, WebKit::WebAccessibilityNotification notification)
> 
> This function seems really long to put in an API.  Will all the overrides need to be in public headers?

As it's a template, the entire code has to go into the header.

Another possibility would be to move the actual implementation on WebTestProxyBase, and have the WebTestProxy template just dispatch the method. Was that preferable?

Created attachment 173093 [details]
Patch

Here's a version that hides the implementation in WebTestProxyBase

[James Robinson]

Comment on attachment 173093 [details]
Patch

Why the templates?  Can't this just be

class WebTestProxy : WebViewClient {
private:
   WebViewClient* m_realClient;
};

?

(In reply to comment #24)
> (From update of attachment 173093 [details])
> Why the templates?  Can't this just be
> 
> class WebTestProxy : WebViewClient {
> private:
>    WebViewClient* m_realClient;
> };
> 
> ?

Then I need to override all methods of WebViewClient (see the first patch)

[Adam Barth]

Comment on attachment 173093 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=173093&action=review

Thanks.  This approach looks much easier to maintain.

> Tools/DumpRenderTree/chromium/TestRunner/public/WebTestProxy.h:63
> +template<class WebViewClientImpl, typename T>

WebViewClientImpl -> can we use a name for this template that seems more like a variable?

> Tools/DumpRenderTree/chromium/TestRunner/public/WebTestProxy.h:73
> +    virtual void postAccessibilityNotification(const WebKit::WebAccessibilityObject& obj, WebKit::WebAccessibilityNotification notification)

obj -> object

Created attachment 173227 [details]
Patch for landing

(In reply to comment #26)
> (From update of attachment 173093 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=173093&action=review
> 
> Thanks.  This approach looks much easier to maintain.
> 
> > Tools/DumpRenderTree/chromium/TestRunner/public/WebTestProxy.h:63
> > +template<class WebViewClientImpl, typename T>
> 
> WebViewClientImpl -> can we use a name for this template that seems more like a variable?

I like this name, as it indicates what the template parameter is supposed to be.

Also, WTF uses similarly formatted names (either T,U, ... or descriptive names)


> 
> > Tools/DumpRenderTree/chromium/TestRunner/public/WebTestProxy.h:73
> > +    virtual void postAccessibilityNotification(const WebKit::WebAccessibilityObject& obj, WebKit::WebAccessibilityNotification notification)
> 
> obj -> object

done

[WebKit Review Bot]

Comment on attachment 173227 [details]
Patch for landing

Clearing flags on attachment: 173227

Committed r134027: <http://trac.webkit.org/changeset/134027>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.