Bug 92975 – Introduce InternalsClient for handling port specific details as resource urls etc. for Internals object

• Status: RESOLVED
• Resolution: WONTFIX
• Component: Tools / Tests

[Vivek Galatage]

Introduce InternalsClient for handling port specific details as resource urls etc. for Internals object

[Vivek Galatage]

Created attachment 156062 [details]
Work In Progress Patch

[Yury Semikhatsky]

Comment on attachment 156062 [details]
Work In Progress Patch

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

> Source/WebCore/testing/InternalsClient.h:40
> +PassOwnPtr<InternalsClient> provideInternalsClient();

What prevents you from adding InternalsClient* a parameter to Internals::Internals ?

[Vivek Galatage]

(In reply to comment #2)
> (From update of attachment 156062 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=156062&action=review
> 
> > Source/WebCore/testing/InternalsClient.h:40
> > +PassOwnPtr<InternalsClient> provideInternalsClient();
> 
> What prevents you from adding InternalsClient* a parameter to Internals::Internals ?

:) I thought of doing so. As I was rushing back home from office, I thought quickly upload the patch and work on it from home. I will correct this one. It would need modifications to WebCoreTestSupport::injectInternalsObject signature as WebCoreTestSupport::injectInternalsObject(JSContextRef context, InternalsClient* client).  This function is being called from all the platform specific DumpRenderTree implementations. So its easy to pass the direct client implementation.

[Vivek Galatage]

Created attachment 156230 [details]
Patch

[Yury Semikhatsky]

We missed one important thing: Internals object was introduced to avoid writing platform-specific code when we need to test platform-independent WebCore functionality. Now we are going to introduce a platform-specific part to it, this seems to contradict Internals design. Sorry for not bringing this up earlier. If opening an alternative front-end for the protocol tests requires some platform-specific logic then I'd rather put it into layoutTestController(or testRunner whichever name now is in fashion).

[Hajime Morrita]

(In reply to comment #5)
> We missed one important thing: Internals object was introduced to avoid writing platform-specific code when we need to test platform-independent WebCore functionality. Now we are going to introduce a platform-specific part to it, this seems to contradict Internals design. Sorry for not bringing this up earlier. If opening an alternative front-end for the protocol tests requires some platform-specific logic then I'd rather put it into layoutTestController(or testRunner whichever name now is in fashion).

This is true in principle.
But for this case,that only platform specific bit is the URL of inspector.html
and bunch of other logic can be shared. 
If we move things to TestRunner, we need to duplicate these for each port,
which will be sad. IMO having InternalsClient is not that bad.

[Vivek Galatage]

(In reply to comment #6)
> (In reply to comment #5)
> > We missed one important thing: Internals object was introduced to avoid writing platform-specific code when we need to test platform-independent WebCore functionality. Now we are going to introduce a platform-specific part to it, this seems to contradict Internals design. Sorry for not bringing this up earlier. If opening an alternative front-end for the protocol tests requires some platform-specific logic then I'd rather put it into layoutTestController(or testRunner whichever name now is in fashion).
> 
> This is true in principle.
> But for this case,that only platform specific bit is the URL of inspector.html
> and bunch of other logic can be shared. 
> If we move things to TestRunner, we need to duplicate these for each port,
> which will be sad. IMO having InternalsClient is not that bad.

Thank you Yury and Morrita, so whats the next step that we are gonna take?

[Pavel Feldman]

Comment on attachment 156230 [details]
Patch

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

> Tools/DumpRenderTree/win/FrameLoadDelegate.cpp:58
> +    String inspectorProtocolVerifierURL() { return String("TO-BE-ADDED-URL"); }

Why not to use http://localhost:8000/inspector/resources/protocol-tests-frontend.html ?

[Yury Semikhatsky]

(In reply to comment #8)
> (From update of attachment 156230 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=156230&action=review
> 
> > Tools/DumpRenderTree/win/FrameLoadDelegate.cpp:58
> > +    String inspectorProtocolVerifierURL() { return String("TO-BE-ADDED-URL"); }
> 
> Why not to use http://localhost:8000/inspector/resources/protocol-tests-frontend.html ?

Sounds like a good solution as we are going to use the front-end only in layout tests. If we decide to serve the front-end this way we will need to put all protocol tests under LayoutTests/http so that we have the web server started for them but that shouldn't be an issue.

[Vivek Galatage]

(In reply to comment #9)
> (In reply to comment #8)
> > (From update of attachment 156230 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=156230&action=review
> > 
> > > Tools/DumpRenderTree/win/FrameLoadDelegate.cpp:58
> > > +    String inspectorProtocolVerifierURL() { return String("TO-BE-ADDED-URL"); }
> > 
> > Why not to use http://localhost:8000/inspector/resources/protocol-tests-frontend.html ?
> 
> Sounds like a good solution as we are going to use the front-end only in layout tests. If we decide to serve the front-end this way we will need to put all protocol tests under LayoutTests/http so that we have the web server started for them but that shouldn't be an issue.

+1. Sounds great. 

One question about inspector/resources/protocol-tests-frontend.html. How will this resource be placed? Or we need not worry about this at all?

[Vivek Galatage]

So now that we are considering this localhost based url, then I guess we wouldn't need this InternalsClient. Correct me if you have other opinion.

[Yury Semikhatsky]

(In reply to comment #10)
> (In reply to comment #9)
> > (In reply to comment #8)
> > > (From update of attachment 156230 [details] [details] [details])
> > > View in context: https://bugs.webkit.org/attachment.cgi?id=156230&action=review
> 
> One question about inspector/resources/protocol-tests-frontend.html. How will this resource be placed? Or we need not worry about this at all?

Layout test runner will launch a web server for tests and we just need to make sure that it serves the directory where the protocol-tests-frontend.html lies.


(In reply to comment #11)
> So now that we are considering this localhost based url, then I guess we wouldn't need this InternalsClient. Correct me if you have other opinion.

That's true. We can hardcode the URL into Internals.cpp as it is going to be the same on all platforms.

[Vivek Galatage]

With the URL, we now I have another problem in hand. Now with the raw WebCore::Page object created, when we try to load the above URL(giving the EmptyFrameLoaderClient instance for frame loading), no n/w request is being made. This behavior is obvious considering none of the methods of FrameLoaderClient being implemented in EmptyFrameLoaderClient. 

In order to have this request go through, we need to have the FrameLoaderClient interface with the necessary methods implemented. But this implementation would need the n/w stack for its operation. This mandates of using port specific n/w stack. So having this implemented from Internals is not completely straight forward in that it involves port specific components.

Hence I was thinking of following options:

1. Move the logic from internals to DRT and implement port specific Page object and url/resource loading mechanism.

pros:
* implementation becomes straight forward
* we can think of using the resource location directly as now we are platform dependent. We can have a re-look of using either the localhost based url or port specific resource location.
* limits(if not completely eliminates the need for) the export symbols from webcore 

cons:
* need to patch almost all ports
* code duplication with similar logic
* doubtful about a chrome-less(headless) page as we will be using port specific page objects such as QWebPage, WebPage etc.

2. We can reuse the existing frameloader client from the layout test's Page object. Again this object can't be inferred directly. Hence we need to implement something like prototype design pattern ( http://en.wikipedia.org/wiki/Prototype_pattern ) and providing method like
virtual PassOwnPtr<FrameLoaderClient> FrameLoaderClient::clone() = 0;
Using this clone method, we can get the handle to the port specific implementation of frame loader client and simply pass this instance for loading the above url. 

pros:
* looks elegant from code re-usability
* requires minimal coding effort as most of the code is already implemented

cons:
* need to modify core interface class(es) like FrameLoaderClient etc.
* the modification is required only for the test harness. So not sure if this will be accepted in general by most of the community members as this would add the overhead of a new virtual method.

3. Implement the FrameLoaderClient within internals for all the ports

pros:
* none. Rather it has all drawbacks.

cons:
* code duplication as ports already implements this
* maintenance would be a difficult
* keeping in sync would be tedious because of the code duplication.


Please let me know your thoughts on this.

[Adam Barth]

I'd like to help by suggesting approaches, but I feel like I'm jump into the middle of a movie here.  Would you be willing to explain what problem you're trying to solve?  I understand that this is related to being able to test the inspector protocol without the normal inspector front-end, but I don't understand how that relates to needing to fake out FrameLoader.

[Vivek Galatage]

(In reply to comment #14)
> I'd like to help by suggesting approaches, but I feel like I'm jump into the middle of a movie here. 

Thank you Adam for your help. 

> Would you be willing to explain what problem you're trying to solve?  I understand that this is related to being able to test the inspector protocol without the normal inspector front-end, but I don't understand how that relates to needing to fake out FrameLoader.

You are right. We are trying to come up with the test harness for testing pure inspector protocol only tests. As for this, we are trying the option of creating a dummy page (in our case, we are trying to create a raw WebCore::Page object) and then loading only the bare minimal JavaScript resources needed for protocol handling like InspectorBackend.js and InspectorBackendCommands.js in this new page through the protocol-tests-frontend.html. 

So as to minimize having platform specific resource handling for this protocol-tests-frontend.html, we thought of using the localhost based url. Now, the page is created with EmptyFrameLoaderClient. And this is where the url will not be loaded as we have empty frame loader client.

Hence I proposed above approaches to address this issue. I would be glad to receive your feedback. Thanks once again for helping.

[Vivek Galatage]

This is very rough outline code which illustrates what we are trying to achieve. http://pastebin.com/VyeJQq0y (this code is not following any standards as such. It might contain lot of errors as well as it also leaks the memory. This is just for illustration purpose)

[Adam Barth]

Sorry if this is a dumb question, but what do you gain by using a fake page rather than a real page?  It looks like the code is trying to inject a mock InspectorFrontendClient of some sort.  I wonder if it would be easier to create a real page and then replace its InspectorFrontendClient.

[Vivek Galatage]

(In reply to comment #17)
> Sorry if this is a dumb question, but what do you gain by using a fake page rather than a real page?  It looks like the code is trying to inject a mock InspectorFrontendClient of some sort.  I wonder if it would be easier to create a real page and then replace its InspectorFrontendClient.

The reason of using the fake page(raw Page object) is that we just needed a placeholder for containing the backend js code without the need of rendering capability. Also creating this way, we would avoid platform specific code as there isn't much support needed from port specific details. Hence we thought we would create a fake page and then install the mock frontend client using a mock channel so that the protocol alone can be tested.

[Adam Barth]

I would just use a real page.  There are plenty of tests that create a new window using window.open.  You can then mock out whatever you need in that page, but at least the rest of the machinery will actually exist.

[Vivek Galatage]

Closing the bug as introducing InternalsClient is not required.