Bug 31472 – Web Inspector: Make DRT show web inspector for tests in inspector/ folder.

• Component: Web Inspector (Deprecated)
• Status: RESOLVED FIXED

[Pavel Feldman]

Currently we show inspector from within the test and force a reload. This makes tests slow and flaky.

[Pavel Feldman]

Created attachment 43161 [details]
[PATCH] Work in progress patch

[Timothy Hatcher]

Comment on attachment 43161 [details]
[PATCH] Work in progress patch

Looking good.

[Pavel Feldman]

Created attachment 43312 [details]
[PATCH] Proposed changes

[Pavel Feldman]

Created attachment 43313 [details]
[PATCH] Same with Win stuff fixed.

[Eric Ayers]

Comment on attachment 43312 [details]
[PATCH] Proposed changes

> +    // Inject scripts into the frontend on the first pass.  Some other logic may want to
> +    // use them before the reload.
comment out of date.

> +    var toInject = [];
> +    for (var name in window) {
> +        if (name.indexOf("frontend_") === 0 && typeof window[name] === "function")
> +            toInject.push(window[name].toString());
> +    }
> +    // Invoke a setup method if it has been specified
> +    if (window["frontend_setup"]) 
> +        toInject.push("frontend_setup();");
>  
> -    // Make sure web inspector window is closed before the test is interrupted.
> -    setTimeout(function() {
> -        alert("Internal timeout exceeded.")
> -        if (window.layoutTestController) {
> -            layoutTestController.closeWebInspector();
> -            layoutTestController.notifyDone();
> -        }
> -    }, 10000);
> +    evaluateInWebInspector(toInject.join("\n"), doit);
>  }
>  
>  function evaluateInWebInspector(script, callback)
> @@ -61,16 +33,16 @@ function evaluateInWebInspector(script, callback)
>      setTimeout(function() {
>          if (window.layoutTestController)
>              layoutTestController.evaluateInWebInspector(callId, script);
> +        else
> +            callback();

Is something wrong with the diff?  what happened to the lines:

    var callId = lastCallId++;
    callbacks[callId] = callback;

> +    printTimelineRecords(performActions, "Paint");

For this parameter, I'm wondering why you chose to pass a string instead of a value like timelineAgentRecordType.Paint


> diff --git a/LayoutTests/inspector/timeline-test.js b/LayoutTests/inspector/timeline-test.js
> index 19865b2..00bb770 100644
> --- a/LayoutTests/inspector/timeline-test.js
> +++ b/LayoutTests/inspector/timeline-test.js
> @@ -1,7 +1,3 @@
> -// Used to mark timeline records as belonging to the test framework.
> -var timelineOverheadMark = "***Overhead***";
> -
> -// TimelineAgent record type definitions from the inspector
>  var timelineAgentRecordType = {};
>  
>  // Scrub values when printing out these properties in the record or data field.
> @@ -12,27 +8,40 @@ var timelineNonDeterministicProps = {
>      url : 1
>  };
>  
> -// Call this function from the doit() function in the main test page.
> -// Pass a function that will get an array of timeline data to process.
> -function retrieveTimelineData(analyzeFunction)
> +function printTimelineRecords(performActions, typeName, formatter)
>  {
> +    if (performActions) {
> +        if (window.layoutTestController)
> +            layoutTestController.setTimelineProfilingEnabled(true);
> +        performActions();
> +    }
> +
>      evaluateInWebInspector("WebInspector.TimelineAgent.RecordType", function(result) {
>          timelineAgentRecordType = result;
>      });
>  
> -    evaluateInWebInspector("frontend_getTimelineResults()", function(timelineRecords) {
> +    evaluateInWebInspector("frontend_getTimelineResults", function(timelineRecords) {

As an ignorant slob, it surprises me that omitting the () actually calls the function?

[Timothy Hatcher]

Comment on attachment 43313 [details]
[PATCH] Same with Win stuff fixed. 

IIRC win/WebInspector.h changes need to go at the end of the class, so they wont break nightly builds. Someone on Windows and GTK should review.

[Timothy Hatcher]

Otherwise looks OK.

[Pavel Feldman]

(In reply to comment #6)
> (From update of attachment 43313 [details])
> IIRC win/WebInspector.h changes need to go at the end of the class, so they
> wont break nightly builds. Someone on Windows and GTK should review.

Will fix. I tested clean build on Windows and it is fine, building GTK now.

[Pavel Feldman]

(In reply to comment #5)
> (From update of attachment 43312 [details])
> > +    // Inject scripts into the frontend on the first pass.  Some other logic may want to
> > +    // use them before the reload.
> comment out of date.
> 

Fixed.

> 
> Is something wrong with the diff?  what happened to the lines:
> 
>     var callId = lastCallId++;
>     callbacks[callId] = callback;
> 

They are not in diff, hence they are still there in the code...

> > +    printTimelineRecords(performActions, "Paint");
> 
> For this parameter, I'm wondering why you chose to pass a string instead of a
> value like timelineAgentRecordType.Paint
> 

By this moment timelineAgentRecordType is not yet available.

> 
> As an ignorant slob, it surprises me that omitting the () actually calls the
> function?

It is a hint in the testing framework (see TestController.js). If passed parameter is a name of a function, it would invoke it with the testController object. Not that we need it here, but calling by name is just as good as invoking...

[Pavel Feldman]

Created attachment 43319 [details]
[PATCH] Review comments addressed

[Gustavo Noronha (kov)]

Comment on attachment 43319 [details]
[PATCH] Review comments addressed

Awesome =)

> +        * webkit/webkitwebinspector.cpp:
> +        (webkit_web_inspector_class_init):
> +        (webkit_web_inspector_set_property):
> +        (webkit_web_inspector_get_property):
> +

> +    /**
> +    * WebKitWebInspector:timeline-profiling-enabled
> +    *
> +    * This is enabling Timeline profiling in the Inspector.
> +    *
> +    * Since: 1.1.1

This should be 1.1.17.

> +    */
> +    g_object_class_install_property(gobject_class,
> +                                    PROP_TIMELINE_PROFILING_ENABLED,
> +                                    g_param_spec_boolean(
> +                                        "timeline-profiling-enabled",
> +                                        _("Enable Timeline profiling"),
> +                                        _("Profile the WebCore instrumentation."),
> +                                        FALSE,
> +                                        WEBKIT_PARAM_READWRITE));

All looks good otherwise. I'll try running the tests with the patch and let you know.

[Gustavo Noronha (kov)]

With the patch applied I get these failures consistently:

inspector/console-dir.html -> failed
inspector/console-dirxml.html -> failed
inspector/console-format-collections.html -> failed
inspector/console-format.html -> failed
inspector/console-tests.html -> failed

All other inspector/ patches pass. Here's one of the diffs:

--- /tmp/layout-test-results/inspector/console-dir-expected.txt	2009-11-16 19:11:48.000000000 -0200
+++ /tmp/layout-test-results/inspector/console-dir-actual.txt	2009-11-16 19:11:48.000000000 -0200
@@ -3,6 +3,7 @@
 CONSOLE MESSAGE: line 11: [object NodeList]
 Tests that console logging dumps proper messages.
 
+Error dispatching: getStyles
 console-dir.html:9HTMLDocument
 console-dir.html:10Array
 console-dir.html:11NodeList

Without the patch, I remember I was getting these errors once in a while. Something wrong on our side? Ideas? =)

[Pavel Feldman]

Ok, at least it builds on GTK ok. Thanks for giving it a try.

> +Error dispatching: getStyles
>  console-dir.html:9HTMLDocument
>  console-dir.html:10Array
>  console-dir.html:11NodeList
> 
> Without the patch, I remember I was getting these errors once in a while.
> Something wrong on our side? Ideas? =)

Error message you are seeing is due to the exception somewhere in

InjectedScript.getStyles (InjectedScript.js:57). This getStyles methods is called from InjectedScript.dispatch (same file). dispatch is invoked from InspectorBackend.cpp:419.

Easiest way of nailing it down would be to surround getStyles with try/catch in InjectedScript.js and dump exception into the console. Replace InjectedScript.dispatch in InjectedScript.js with following:

InjectedScript.dispatch = function(methodName, args, callId)
{
    var argsArray = JSON.parse(args);
    if (callId)
        argsArray.splice(0, 0, callId);  // Methods that run asynchronously have a call back id parameter.
    try {
        var result = InjectedScript[methodName].apply(InjectedScript, argsArray);
        if (typeof result === "undefined") {
            InjectedScript._window().console.error("Web Inspector error: InjectedScript.%s returns undefined", methodName);
            result = null;
        }
        return JSON.stringify(result);
    } catch (e) {
        InjectedScript._window().console.log(e.toString());
        throw e;
    }
}

I'll make it a part of this change...

[Timothy Hatcher]

Comment on attachment 43319 [details]
[PATCH] Review comments addressed

> +    * Since: 1.1.1

This should be 1.1.17.

[Pavel Feldman]

Comment on attachment 43319 [details]
[PATCH] Review comments addressed

(will land later, once investigated gtk issue. i think we are finishing the test while having some active timeouts...)

[Gustavo Noronha (kov)]

Here's what I get with your suggested change:

--- /tmp/layout-test-results/inspector/console-dir-expected.txt	2009-11-17 09:54:52.000000000 -0200
+++ /tmp/layout-test-results/inspector/console-dir-actual.txt	2009-11-17 09:54:52.000000000 -0200
@@ -1,8 +1,11 @@
+CONSOLE MESSAGE: line 0: TypeError: Result of expression 'defaultView' [null] is not an object.
 CONSOLE MESSAGE: line 9: [object HTMLDocument]
 CONSOLE MESSAGE: line 10: test1,test2
 CONSOLE MESSAGE: line 11: [object NodeList]
 Tests that console logging dumps proper messages.
 
+TypeError: Result of expression 'defaultView' [null] is not an object.
+Error dispatching: getStyles
 console-dir.html:9HTMLDocument
 console-dir.html:10Array
 console-dir.html:11NodeList

[Gustavo Noronha (kov)]

(In reply to comment #16)
> +TypeError: Result of expression 'defaultView' [null] is not an object.
> +Error dispatching: getStyles

Adding if (!defaultView) return false; makes all tests pass again, but I am wondering if this is being caused by us lacking something, that causes DOMAgent to not work as expected.

[Pavel Feldman]

(In reply to comment #17)
> (In reply to comment #16)
> > +TypeError: Result of expression 'defaultView' [null] is not an object.
> > +Error dispatching: getStyles
> 
> Adding if (!defaultView) return false; makes all tests pass again, but I am
> wondering if this is being caused by us lacking something, that causes DOMAgent
> to not work as expected.

I've reproduced this on Mac. Something is wrong with frontend being loaded too late. I am investigating. Thanks for your analysis!

[Pavel Feldman]

Created attachment 43355 [details]
[PATCH] I am going to land.

[Pavel Feldman]

Committing to http://svn.webkit.org/repository/webkit/trunk ...
	R	LayoutTests/fast/inspector/cssURLQuotes-expected.txt => LayoutTests/fast/inspector-support/cssURLQuotes-expected.txt
	R	LayoutTests/fast/inspector/cssURLQuotes.html => LayoutTests/fast/inspector-support/cssURLQuotes.html
	R	LayoutTests/fast/inspector/matchedrules.html => LayoutTests/fast/inspector-support/matchedrules.html
	R	LayoutTests/fast/inspector/style.html => LayoutTests/fast/inspector-support/style.html
	R	LayoutTests/inspector/uncaught-dom1-exception-expected.txt => LayoutTests/fast/inspector-support/uncaught-dom1-exception-expected.txt
	R	LayoutTests/inspector/uncaught-dom1-exception.html => LayoutTests/fast/inspector-support/uncaught-dom1-exception.html
	R	LayoutTests/inspector/uncaught-dom3-exception-expected.txt => LayoutTests/fast/inspector-support/uncaught-dom3-exception-expected.txt
	R	LayoutTests/inspector/uncaught-dom3-exception.html => LayoutTests/fast/inspector-support/uncaught-dom3-exception.html
	R	LayoutTests/inspector/uncaught-dom8-exception-expected.txt => LayoutTests/fast/inspector-support/uncaught-dom8-exception-expected.txt
	R	LayoutTests/inspector/uncaught-dom8-exception.html => LayoutTests/fast/inspector-support/uncaught-dom8-exception.html
	M	LayoutTests/ChangeLog
	M	LayoutTests/inspector/inspector-test.js
	A	LayoutTests/inspector/resources/timeline-iframe-data.html
	M	LayoutTests/inspector/timeline-layout-expected.txt
	M	LayoutTests/inspector/timeline-layout.html
	M	LayoutTests/inspector/timeline-mark-timeline.html
	M	LayoutTests/inspector/timeline-paint.html
	M	LayoutTests/inspector/timeline-parse-html-expected.txt
	M	LayoutTests/inspector/timeline-parse-html.html
	M	LayoutTests/inspector/timeline-recalculate-styles-expected.txt
	M	LayoutTests/inspector/timeline-recalculate-styles.html
	M	LayoutTests/inspector/timeline-script-tag-1-expected.txt
	M	LayoutTests/inspector/timeline-script-tag-1.html
	M	LayoutTests/inspector/timeline-script-tag-2-expected.txt
	M	LayoutTests/inspector/timeline-script-tag-2.html
	M	LayoutTests/inspector/timeline-script-tag-2.js
	M	LayoutTests/inspector/timeline-test.js
	M	WebCore/ChangeLog
	M	WebCore/WebCore.Inspector.exp
	M	WebCore/inspector/front-end/TimelinePanel.js
	M	WebKit/gtk/ChangeLog
	M	WebKit/gtk/webkit/webkitwebinspector.cpp
	M	WebKit/mac/ChangeLog
	M	WebKit/mac/WebInspector/WebInspector.h
	M	WebKit/mac/WebInspector/WebInspector.mm
	M	WebKit/win/ChangeLog
	M	WebKit/win/Interfaces/IWebInspector.idl
	M	WebKit/win/WebInspector.cpp
	M	WebKit/win/WebInspector.h
	M	WebKitTools/ChangeLog
	M	WebKitTools/DumpRenderTree/LayoutTestController.cpp
	M	WebKitTools/DumpRenderTree/LayoutTestController.h
	M	WebKitTools/DumpRenderTree/gtk/DumpRenderTree.cpp
	M	WebKitTools/DumpRenderTree/gtk/LayoutTestControllerGtk.cpp
	M	WebKitTools/DumpRenderTree/mac/DumpRenderTree.mm
	M	WebKitTools/DumpRenderTree/mac/LayoutTestControllerMac.mm
	M	WebKitTools/DumpRenderTree/win/DumpRenderTree.cpp
	M	WebKitTools/DumpRenderTree/win/LayoutTestControllerWin.cpp
Committed r51072