Created attachment 154766 [details] 'testRunner' chrome extension Currently LayoutTests must be developed without the aid of a debugger. The following changes would allow WebInspector to debug these tests on itself: 1. Add an ExperimentsSetting "testRunner" (Settings.js) 2. When the developer has enabled this experiment, accept "message" events from extensions that eval() the test driver code. 3. Add a removeSniffers function to InspectorTest, so the WebInspector can be cleaned up after each test debugging run. In addition 'testRunner' extension needs to implement the equivalent of DumpRenderTree 'testRunner'. The attached Chrome extension does this for the majority of simple LayoutTests. This scheme may not work for all LayoutTests, either because the extension interferes or does not implement enough of DumpRenderTree testRunner. But most tests are simple and would benefit.
Created attachment 154781 [details] Patch
I like the general idea of enabling debugging of layout tests, but in my view, this particular approach is too much of a hack to expose even as an experiment. How about requiring a remote front-end for debugging layout tests? This way, it will run as a "normal" web page and you'll be able to inject scripts there with your extension. This could even be taken further -- your mock test runner, upon a call to showWebInspector() could request the background page to open a new tab and navigate it to the (remote) front-end and inject the code that install a listener for the message event on window. Perhaps, you could also monkey-patch the InspectedFrontendHostStub to have its sendMessageToBackend() to actually send message to background page, which would relay it to back-end through chrome.debugger API -- this way, one wouldn't event have to explicitly enable remote debugging ws server. Actually, many other methods of InspectedFrontendHost may be implemented via an extension, which would allow for a nearly full-functional remote front-end -- quite useful beyond the scope of debugging, I think.
(In reply to comment #2) > I like the general idea of enabling debugging of layout tests, but in my view, this particular approach is too much of a hack to expose even as an experiment. I agree that this looks hacky at first, but only because we are calling eval() on something sent to us. The eval() is a consequence of how LayoutTests work; I guess all solutions will require eval(). We might have other ways to send the script but this one gives a high degree of security. One small improvement I thought about this morning would 1) add a function to ExtensionServer to check if a URL is a registered extension, 2) move the eval() into a separate onmessage handler in another small .js file and have it check the origins using the new function from #1. Would this be better? >How about requiring a remote front-end for debugging layout tests? This way, it will run as a "normal" web page and you'll be able to inject scripts there with your extension. This could even be taken further -- your mock test runner, upon a call to showWebInspector() could request the background page to open a new tab and navigate it to the (remote) front-end and inject the code that install a listener for the message event on window. Perhaps, you could also monkey-patch the InspectedFrontendHostStub to have its sendMessageToBackend() to actually send message to background page, which would relay it to back-end through chrome.debugger API -- this way, one wouldn't event have to explicitly enable remote debugging ws server. > Actually, many other methods of InspectedFrontendHost may be implemented via an extension, which would allow for a nearly full-functional remote front-end -- quite useful beyond the scope of debugging, I think. Yes, I agree this would be a great idea, but I've already tried all of the combinations like this I can think up. I have a functional inspector/front_end in a chrome extension operating over chrome.debugger and I can monkey-patch it to accomplish the same thing as we have here. (https://github.com/johnjbarton/sirius) However this solution requires a) changes to devtools extensions because there is no way to inject the chrome.devtools API, b) synchronization of the extension copy of inspector/front_end with trunk (I do that myself but it would put a big barrier for others to do). c) adoption/learning of the extension by other devs Note that the extension solution is much less attractive after you realize: 1) content-scripts can only act on https://, not chrome:// or chrome-extension://, 2) devtools extensions need to be tested and devtools need be tested with extensions installed, 3) devtools-extensions only function properly when loaded via normal devtools operation. However, perhaps I don't understand what you have in mind?
Created attachment 155168 [details] [Patch] Standalone test running proof of concept
Here is a how to use my patch (I think we can land it as is): [Set up] 1) Start HTTP server on port 8000 that serves WebKit/ folder 2) Run Chrome Canary as /Applications/Google\ Chrome\ Canary.app/Contents/MacOS/Google\ Chrome\ Canary --remote-debugging-port=9222 --no-first-run --user-data-dir=testProfile http://localhost:8000/LayoutTests/inspector/ [Run] Clicking on the test will run inspector test in a standalone mode. Unblock popups to see inspector window opening and running through the test.
Comment on attachment 155168 [details] [Patch] Standalone test running proof of concept View in context: https://bugs.webkit.org/attachment.cgi?id=155168&action=review > LayoutTests/http/tests/inspector/inspector-test.js:519 > +function StandaloneTestRunner() StandaloneInspectorTestRunner? > LayoutTests/http/tests/inspector/inspector-test.js:553 > + stylesheet.rel="stylesheet"; " = " > LayoutTests/http/tests/inspector/inspector-test.js:555 > + stylesheet.href = "/LayoutTests/http/tests/inspector/resources/jsdifflib/diffview.css"; Extract the base name as a const? > LayoutTests/http/tests/inspector/inspector-test.js:587 > + this._postMessages([command]); command?? so does this work? > LayoutTests/http/tests/inspector/inspector-test.js:603 > + setTimeout(function() { Why do we need this?
Created attachment 155297 [details] [Patch] review comments addressed
(In reply to comment #5) > Here is a how to use my patch (I think we can land it as is): > > [Set up] > > 1) Start HTTP server on port 8000 that serves WebKit/ folder > > 2) Run Chrome Canary as > > /Applications/Google\ Chrome\ Canary.app/Contents/MacOS/Google\ Chrome\ Canary --remote-debugging-port=9222 --no-first-run --user-data-dir=testProfile http://localhost:8000/LayoutTests/inspector/ > > [Run] > > Clicking on the test will run inspector test in a standalone mode. Unblock popups to see inspector window opening and running through the test. I get: Failed to load resource: the server responded with a status of 404 (Not Found) http://localhost:8000/Source/WebCore/inspector/front-end/InspectorBackendCommands.js Unexpected response code: 500 :8000:1
> Failed to load resource: the server responded with a status of 404 (Not Found) http://localhost:8000/Source/WebCore/inspector/front-end/InspectorBackendCommands.js > Unexpected response code: 500 :8000:1 Try restarting the Canary and/or changing the remote connection address in inspector-test.js. I'll fix it once we introduce localhost:9222/jsonp.
(In reply to comment #9) > > Failed to load resource: the server responded with a status of 404 (Not Found) http://localhost:8000/Source/WebCore/inspector/front-end/InspectorBackendCommands.js > > Unexpected response code: 500 :8000:1 > > Try restarting the Canary and/or changing the remote connection address in inspector-test.js. I'll fix it once we introduce localhost:9222/jsonp. Sorry, neither restarting nor changing the address had any effect.
This link does work: http://localhost:9220/out/Release/resources/inspector/devtools.html?ws=localhost:9222/devtools/page/2_1 The port is different, but I don't think this matters. The path points to the built devtools so I don't get the 404 The inspectable pages link points to 2_1 instead of 4_1 so I don't get the 500. A simple workaround before the jsonp is to use the link http://localhost:9222 and manually select the page with the test. If the link were configurable then I believe I could also launch sirius this way. As soon as the devtools load they run the tests and exit. For debugging we'll need to clear the WebInspector.InspectorTest and removeSniffers as I did in my patch. I also ran into a few issues related to the difference between tests and real-life. One I ran down: if the Debugger is disabled, then you reload the web page, the Console is broken. Console linkifies and consults DebuggerModel but it has stale info because it ignores the globalObjectReset message when disabled. I guess Debugger disable is not supported in real life.
Created attachment 155894 [details] Patch
This version succeeds in debugging a simple case: As before start a server root /WebKit, any port. Open a fresh copy of the browser on the /LayoutTests/inspector, navigate to your test. When the page opens, a popup offers to debug, giving two choices, click on the LayoutTest one. The inspector loads and runs the test case. Diffs nicely shown, thanks Pavel. Now open inspector on the test-inspector. If you use control-R in the outer inspector, the test-inspector will reload and then the test page will fire the test again. I don't know if this version will work on tests that involve reloading the web page. Changes over Pavel's version: Test page opens http://localhost:9222 and the tester has to pick the test case out of the little windows. Workaround for no jsonp solution but it's ok. This change also means the server port need not be embedded in the inspector-test. The eval() in TestController gets a // @ sourceURL so we can pick out the driver code. StandaloneTestRunner no longer closes the test-inspector, but prepares to clear the test results when we detect a reload of the test-inspector.
We can't run extension/ tests because this code in inspector-test.js fails: var extensionURL = (/^https?:/.test(pageURL) ? pageURL.replace(/^(https?:\/\/[^/]*\/).*$/,"$1") : pageURL.replace(/\/inspector\/extensions\/[^/]*$/, "/http/tests")) + "/inspector/resources/extension-main.html"; The condition is true, but in our case we want the 'else' branch. I don't understand what the true case supports.
Created attachment 157383 [details] Patch
Ok here is another try, let me know what you think. I kept running in to problems in debugging, eventually discovering that we can't use WebInspector on WebInspector for breakpoint debugging (https://code.google.com/p/chromium/issues/detail?id=139747). So we need a scheme where the test-victim is running as a Web App, eg as a remote debugger in a different browser. (It's possible that running the client and server of remote debugging and the debugger-on-debugger all in one browser will work, but I had problems and I want I scheme that I know can work). This solution uses two browsers and two copies of the test case. One browser is the backend for testing and output of the test run. The other browser is the front-end for testing, debugger for that test front-end, and reads the test script from the second copy of the test case. Sounds complicated but I think it's a bit cleaner than the other two tries. We have two completely independent debuggers and the devtools extension just needs to reload the victim with a script to trigger the test-script-loading. Biggest source of issues is async timing as usual. [Install] Install testRunner devtools extension: https://github.com/johnjbarton/testRunner [Set up] 1) Start HTTP server on port 8000 that serves WebKit/ folder 2) Run Chrome Canary as /Applications/Google\ Chrome\ Canary.app/Contents/MacOS/Google\ Chrome\ Canary --remote-debugging-port=9222 --no-first-run --user-data-dir=testProfile http://localhost:8000/LayoutTests/inspector/ 3) Run another copy of Chrome (different --user-data-dir), open http://localhost:9222 and select the box that matches step 2. 4) Open WebInspector on the Web page result from #3 (debugger on debugger). [Run] Navigate the first browser to a test case. Open the testRunner panel in the debugger-on-debugger. Click Set... to load the current test case URL into testRunner Click runTest to reload the to-be-tested debugger and run the tests. Results appear in the test case web with jsdifflib
Created attachment 157577 [details] Patch
I had good luck with the latest version. Two problems just solved: 1) sometimes difflib failed to load in time; now we wait for both onLoad events. 2) In fixing the test-case code I often just pressed "runTest" on the debugger-on-debugger. That exercises the debugger test case, but appends output to the web page. To avoid confusion now we just reload the web-page every time we reload the debugger.
Created attachment 157581 [details] Screenshot of setup In case my explanations are unclear, here is how the setup looks From deepest (top of screen) the three windows: 1) test web-page, browser 1, 9222 server 2) to-be-tested debugger, browser 2, 9222 client 3) debugger-on-debugger, browser 2, built-in devtools, showing testRunner extn.
Created attachment 157582 [details] completely re-written chrome extension, https://github.com/johnjbarton/testRunner
Comment on attachment 157577 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=157577&action=review > Source/WebCore/inspector/front-end/TestController.js:54 > + PageAgent.reload(true); Could you explain why this is necessary?
Created attachment 157745 [details] Patch
(In reply to comment #21) > (From update of attachment 157577 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=157577&action=review > > > Source/WebCore/inspector/front-end/TestController.js:54 > > + PageAgent.reload(true); > > Could you explain why this is necessary? It's for the case where we are re-running a test. The web page will have the results from a previous run, we need to clear that out. I wonder if it might be better to navigate the web page to the test url instead. I originally hoped to have the testRunner panel show the list of tests and drive the selection from there. But I could not figure out how to do that easily in a devtools extension.
I just noticed that my patch on 2012-08-09 also includes the patch from Bug 93520, "Refactor InspectorTest to split evals for initialization from runTest"
(In reply to comment #20) > Created an attachment (id=157582) [details] > completely re-written chrome extension, https://github.com/johnjbarton/testRunner Rather than an extension, we could have TestController add a status-bar button behind an experiments flag. Fewer moving parts.
Created attachment 157970 [details] Patch
Comment on attachment 157577 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=157577&action=review > Source/WebCore/inspector/front-end/TestController.js:48 > + setTimeout(this._clearAndTest.bind(this)); // wait for resourceTreeModel setTimeout looks like a hack. Why do we wait for resourceTreeModel in particular? Please see my newer plumbing - it does not modify TestController, but rather does everything through the embedder API (InspectorFrontendAPI). > Source/WebCore/inspector/front-end/TestController.js:53 > + WebInspector.resourceTreeModel.addEventListener(WebInspector.ResourceTreeModel.EventTypes.OnLoad, this._testPageIsClear, this); We should not rely upon resource tree model in the tests. >>> Source/WebCore/inspector/front-end/TestController.js:54 >>> + PageAgent.reload(true); >> >> Could you explain why this is necessary? > > It's for the case where we are re-running a test. The web page will have the results from a previous run, we need to clear that out. > > I wonder if it might be better to navigate the web page to the test url instead. I originally hoped to have the testRunner panel show the list of tests and drive the selection from there. But I could not figure out how to do that easily in a devtools extension. I added a pure html test runner that lists tests and navigates the page to the test (see my newer patch). The only problem there is that we can't test debugger: we need to use extensions for that. By I wonder if we can use your approach and re-use some of my tests in order to achieve that. > LayoutTests/http/tests/inspector/resources/jsdifflib/difflib.js:1 > +/*** Note that I am now using the bundled version of the difflib and no view at all.
Comment on attachment 157970 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=157970&action=review > Source/WebCore/inspector/front-end/InspectorFrontendAPI.js:152 > +if (window.opener) { > + function onMessageFromOpener(event) > + { > + InspectorFrontendAPI.dispatch(event.data); > + } > + window.addEventListener("message", onMessageFromOpener, true); > +} So we're letting embedders evaluate arbitrary code in the front-end? I think this will be abused. Besides, this does not even check that the message originates from the embedder.
> So we're letting embedders evaluate arbitrary code in the front-end? I think this will be abused. Besides, this does not even check that the message originates from the embedder. Well, in case there is opener, we no longer run in the trusted environment. Could you suggest the way it might be abused? I can check whether the message source == opener, but given that a random page can open front-end url I don't see how it affects the security model.
Comment on attachment 157970 [details] Patch Moved this patch into its own bug: it is about extension-less approach. https://bugs.webkit.org/show_bug.cgi?id=93833
(In reply to comment #29) > > So we're letting embedders evaluate arbitrary code in the front-end? I think this will be abused. Besides, this does not even check that the message originates from the embedder. > > Well, in case there is opener, we no longer run in the trusted environment. Could you suggest the way it might be abused? I can check whether the message source == opener, but given that a random page can open front-end url I don't see how it affects the security model. I'm not concerned with the security in this particular case, rather with the encapsulation -- we don't want embedders desperate for implementing certain functionality to abuse eval(), rather than wait for proper API to be implemented. > I can check whether the message source == opener, but given that a random page can open front-end url I don't see how it affects the security model. It's just me being paranoid. There may be other frames in the embedder, even those original authors of the embedder did not expect.
> I'm not concerned with the security in this particular case, rather with the encapsulation -- we don't want embedders desperate for implementing certain functionality to abuse eval(), rather than wait for proper API to be implemented. There is always a way to serve the front-end form third party hosts and hence eval the code right in place, so I'd rather trust our embedders to not harm themselves via using testing API. > > I can check whether the message source == opener, but given that a random page can open front-end url I don't see how it affects the security model. Fixed.
(In reply to comment #27) > (From update of attachment 157577 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=157577&action=review > > > Source/WebCore/inspector/front-end/TestController.js:48 > > + setTimeout(this._clearAndTest.bind(this)); // wait for resourceTreeModel > > setTimeout looks like a hack. Sure it is a hack. Each part of the WebInspector framework has a 'loadCompleted' callback. In the TestController.loadCompleted() I want to depend upon WebInspector.resourceTreeModel. (or not, see below) I want to ensure that the loadCompleted() callback on resourceTreeModel finishes before I rely upon it. Thus I schedule the dependent code on the turn falling loadCompleted(). Is there another way to say "run the next function after all loadCompleted() callbacks have finished"? Or perhaps we can rely on WebInspector.resourceTreeModel.addEventListener() in our loadCompleted? > Why do we wait for resourceTreeModel in particular? All I want to accomplish is to reload the web page and know that this operation has completed. The only way I found to accomplish this was to add an event handler for ResourceTreeModel.EventTypes.OnLoad, then use PageAgent.reload(). (or to use the test-runner.html, PageAgent.navigate()) Is there another way to listen for the web page 'load' event? > Please see my newer plumbing - it does not modify TestController, but rather does everything through the embedder API (InspectorFrontendAPI). I think that makes sense for the opener approach. For my two-browser approach either way would work, but TestController keeps the feature in one file. > > > Source/WebCore/inspector/front-end/TestController.js:53 > > + WebInspector.resourceTreeModel.addEventListener(WebInspector.ResourceTreeModel.EventTypes.OnLoad, this._testPageIsClear, this); > > We should not rely upon resource tree model in the tests. As above, I need it to know that the web page has completed the reload operation before we fire the InspectorTests (I don't know that it is essential but DumpRenderTree works on loaded pages and I don't want to wonder if the tests is failing because of this difference). > > >>> Source/WebCore/inspector/front-end/TestController.js:54 > >>> + PageAgent.reload(true); > >> > >> Could you explain why this is necessary? > > > > It's for the case where we are re-running a test. The web page will have the results from a previous run, we need to clear that out. > > > > I wonder if it might be better to navigate the web page to the test url instead. I originally hoped to have the testRunner panel show the list of tests and drive the selection from there. But I could not figure out how to do that easily in a devtools extension. > > I added a pure html test runner that lists tests and navigates the page to the test (see my newer patch). The only problem there is that we can't test debugger: we need to use extensions for that. By I wonder if we can use your approach and re-use some of my tests in order to achieve that. Oh that was fun to run! I think we could also use test-runner.html to drive the remote debugger in the two-browser scheme. A nice side effect would be understanding the limitations of these non-DumpRenderTree approaches. The two browser scheme has these advantages: 1) should work on debugger tests 2) should work if the test reloads the web page 3) stable window arrangement for debugging (no popups) 4) debugs WebInspector in the recommended way I'm not sure what disadvantages you see and if there are ways to reduce them.
A glorious thread, but the technical approach doesn't really work for the current state of the inspector. I opened https://bugs.webkit.org/show_bug.cgi?id=130051 to track a new approach.