Bug 33096 – [Qt] DRT: Support evaluateInWebInspector(), setTimerProfilingEnabled() and display().

• Component: WebKit Qt
• Status: RESOLVED FIXED

[Robert Hogan]

Support LayoutTestController.evaluateInWebInspector(), setTimerProfiliingEnabled()
and display() in Qt DRT.

This allows the following tests to pass:

inspector/evaluate-in-frontend.html
inspector/elements-panel-structure.html
inspector/styles-iframe.html
inspector/syntax-highlight-css.html
inspector/syntax-highlight-javascript.html
inspector/timeline-enum-stability.html
inspector/timeline-layout.html
inspector/timeline-mark-timeline.html
inspector/timeline-paint.html
inspector/timeline-parse-html.html
inspector/timeline-recalculate-styles.html
inspector/timeline-script-tag-1.html
inspector/timeline-script-tag-2.html
inspector/timeline-trivial.html
inspector/cookie-resource-match.html
inspector/elements-img-tooltip.html
inspector/elements-panel-selection-on-refresh.html
inspector/timeline-event-dispatch.html

[Robert Hogan]

Created attachment 45733 [details]
Patch

The following inspector tests are unresolved by this patch:

 inspector/console-dir.html
 inspector/console-dirxml.html
 inspector/console-format.html
 inspector/console-tests.html
 inspector/console-clear.html
 inspector/console-format-collections.html

They continue to fail because the console message prints an absolute file:// url for the inspected page, rather than a relative one. gdb crashes when loading the DRT for me unless it's a --minimal build so was not able to look into this one for now.

[WebKit Review Bot]

Attachment 45733 [details] did not pass style-queue:

Failed to run "WebKitTools/Scripts/check-webkit-style" exit_code: 1
WebKitTools/DumpRenderTree/qt/LayoutTestControllerQt.cpp:41:  qt_drt_setTimelineProfilingEnabled is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
WebKitTools/DumpRenderTree/qt/LayoutTestControllerQt.cpp:49:  qt_drt_webinspector_execute_script is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
WebKitTools/DumpRenderTree/qt/LayoutTestControllerQt.cpp:402:  Could not find a newline character at the end of the file.  [whitespace/ending_newline] [5]
Total errors found: 3

[Eric Seidel (no email)]

(In reply to comment #2)
> Attachment 45733 [details] did not pass style-queue:
> 
> Failed to run "WebKitTools/Scripts/check-webkit-style" exit_code: 1
> WebKitTools/DumpRenderTree/qt/LayoutTestControllerQt.cpp:41: 
> qt_drt_setTimelineProfilingEnabled is incorrectly named. Don't use underscores
> in your identifier names.  [readability/naming] [4]
> WebKitTools/DumpRenderTree/qt/LayoutTestControllerQt.cpp:49: 
> qt_drt_webinspector_execute_script is incorrectly named. Don't use underscores
> in your identifier names.  [readability/naming] [4]
> WebKitTools/DumpRenderTree/qt/LayoutTestControllerQt.cpp:402:  Could not find a
> newline character at the end of the file.  [whitespace/ending_newline] [5]
> Total errors found: 3

If these are false positives we should definitely file a bug about them so we can get them fixed.

[Eric Seidel (no email)]

Comment on attachment 45733 [details]
Patch

How does this interact with the central LayoutTestController code?
http://trac.webkit.org/browser/trunk/WebKitTools/DumpRenderTree/LayoutTestController.cpp#L865

Does Qt not use that code?

Also, it seems that the x-platform LayoutTestController turns off profiling after every test, which it seems you might need to do too.  I think this patch is wrong, or at least incomplete judging by my quick scan of LayoutTestController.cpp.  Feel free to re-mark this r? if I'm misunderstanding.

[Robert Hogan]

(In reply to comment #4)
> (From update of attachment 45733 [details])
> How does this interact with the central LayoutTestController code?
> http://trac.webkit.org/browser/trunk/WebKitTools/DumpRenderTree/LayoutTestController.cpp#L865
> 
> Does Qt not use that code?
> 

No, it's completely independent of it. The call stack leading up to a function in Qt's LayoutTestController is typically:

#0 LayoutTestController::waitForPolicyDelegate (this=0x81a4ef8) at /home/robert/Development/webkit/WebKitTools/DumpRenderTree/qt/LayoutTestControllerQt.cpp:364
#1 0x0806c20e in LayoutTestController::qt_metacall (this=0x81a4ef8, _c=QMetaObject::InvokeMetaMethod, _id=53, _a=0xbfffe3ac) at moc_LayoutTestControllerQt.cpp:239
#2 0x01d24637 in JSC::Bindings::QtRuntimeMetaMethod::call (exec=0xb7aa70f0, functionObject=0xb7a42dc0, args=...) at ../../../../WebCore/bridge/qt/qt_runtime.cpp:1359
#3 0x0158f2e9 in cti_op_call_NotJSFunction () from /home/robert/Development/webkit/WebKitBuild/selectedrange/Debug/lib/libQtWebKit.so.4
#4 0x0158b721 in JSC::JITThunks::tryCacheGetByID(JSC::ExecState*, JSC::CodeBlock*, JSC::ReturnAddressPtr, JSC::JSValue, JSC::Identifier const&, JSC::PropertySlot const&, JSC::StructureStubInfo*) () from /home/robert/Development/webkit/WebKitBuild/selectedrange/Debug/lib/libQtWebKit.so.4
#5 0x01548157 in JSC::JITCode::execute (this=0x8205498, registerFile=0x81afddc, callFrame=0xb7aa7050, globalData=0x81ae208, exception=0x81aecc4) at ../../../../JavaScriptCore/jit/JITCode.h:79
#6 0x0155014c in JSC::Interpreter::execute (this=0x81afdd0, functionExecutable=0x8205488, callFrame=0x81f1c5c, function=0xb7a42c00, thisObj=0xb7a40000, args=..., scopeChain=0x81e7098, exception=0x81aecc4) at ../../../../JavaScriptCore/interpreter/Interpreter.cpp:685
#7 0x015e6cb9 in JSC::JSFunction::call (this=0xb7a42c00, exec=0x81f1c5c, thisValue=..., args=...) at ../../../../JavaScriptCore/runtime/JSFunction.cpp:120
#8 0x015b96a5 in JSC::call (exec=0x81f1c5c, functionObject=..., callType=JSC::CallTypeJS, callData=..., thisValue=..., args=...) at ../../../../JavaScriptCore/runtime/CallData.cpp:39
#9 0x016ec8a6 in WebCore::JSEventListener::handleEvent (this=0x81b0e30, scriptExecutionContext=0x81aa190, event=0x81ec3f0) at ../../../../WebCore/bindings/js/JSEventListener.cpp:113
#10 0x0186a1af in WebCore::EventTarget::fireEventListeners (this=0x81a4a48, event=0x81ec3f0) at ../../../../WebCore/dom/EventTarget.cpp:297
#11 0x01ae7691 in WebCore::DOMWindow::dispatchEvent (this=0x81a4a48, prpEvent=..., prpTarget=...) at ../../../../WebCore/page/DOMWindow.cpp:1337


> Also, it seems that the x-platform LayoutTestController turns off profiling
> after every test, which it seems you might need to do too.  I think this patch
> is wrong, or at least incomplete judging by my quick scan of
> LayoutTestController.cpp.  Feel free to re-mark this r? if I'm
> misunderstanding.

The Qt QWebInspector is created at the start of each new test and deleted once it's finished - it's managed in the DRT's subclass of qwebpage's constructor and destructor, so I think closewebinspector() is redundant as is settimelineprofilingenabled(false).

[Robert Hogan]

Created attachment 45753 [details]
Updated Patch per style bot

[WebKit Review Bot]

Attachment 45753 [details] did not pass style-queue:

Failed to run "WebKitTools/Scripts/check-webkit-style" exit_code: 1
WebKitTools/DumpRenderTree/qt/LayoutTestControllerQt.cpp:41:  qt_drt_setTimelineProfilingEnabled is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
WebKitTools/DumpRenderTree/qt/LayoutTestControllerQt.cpp:49:  qt_drt_webinspector_execute_script is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
Total errors found: 2

[Robert Hogan]

(In reply to comment #3)
> 
> If these are false positives we should definitely file a bug about them so we
> can get them fixed.

Have fixed the last one and raised https://bugs.webkit.org/show_bug.cgi?id=33116 for the other two.

[Robert Hogan]

(In reply to comment #4)
> The Qt QWebInspector is created at the start of each new test and deleted once
> it's finished - 

Actually, this isn't true. The inspector is created by the page, but the page persists across layout tests, along with the inspector. I think something like closewebinspector() is actually required because I managed to crash an unrelated test when running a sequence of tests with this patch.

Removing r flag for now.

[Eric Seidel (no email)]

Need one more skipped test due to this missing feature:
https://bugs.webkit.org/show_bug.cgi?id=33024#c8

[Kent Hansen]

Why is the call to showWebInspector() needed in the evaluateInWebInspector() implementation? I don't see this call in the implementation for other platforms. Is it because QtWebKit initializes the inspector more lazily than the other platforms? As it stands it looks like a bit of a hack.

Specifically I'm curious whether other platforms are also hitting the m_frontend == 0 case in InspectorController::evaluateForTestInFrontend() (which is what happens if one does not call showWebInspector() in the Qt port), or if there is actually a way to have it evaluate stuff without showing it.

[Robert Hogan]

Created attachment 46490 [details]
Updated Patch

[WebKit Review Bot]

Attachment 46490 [details] did not pass style-queue:

Failed to run "WebKitTools/Scripts/check-webkit-style" exit_code: 1
WebKitTools/DumpRenderTree/qt/DumpRenderTreeQt.cpp:80:  qt_drt_webinspector_close is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
WebKitTools/DumpRenderTree/qt/LayoutTestControllerQt.cpp:42:  qt_drt_setTimelineProfilingEnabled is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
WebKitTools/DumpRenderTree/qt/LayoutTestControllerQt.cpp:50:  qt_drt_webinspector_execute_script is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
WebKitTools/DumpRenderTree/qt/LayoutTestControllerQt.cpp:51:  qt_drt_webinspector_show is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
Total errors found: 4

[Robert Hogan]

(In reply to comment #11)
> Why is the call to showWebInspector() needed in the evaluateInWebInspector()
> implementation? I don't see this call in the implementation for other
> platforms. Is it because QtWebKit initializes the inspector more lazily than
> the other platforms? As it stands it looks like a bit of a hack.
> 
> Specifically I'm curious whether other platforms are also hitting the
> m_frontend == 0 case in InspectorController::evaluateForTestInFrontend() (which
> is what happens if one does not call showWebInspector() in the Qt port), or if
> there is actually a way to have it evaluate stuff without showing it.

Hi Kent - you've hit the nail on the head here. As well as being a hack, the lazy init was causing a lot of flakiness when running the tests. Bypassing QWebInspector for initializing and closing the controller has allowed the tests to pass every time for me now.

There's also no need to actually display the inspector (as you suggested) though there is still a need to display the webview for the timeline-paint.html test.

[Robert Hogan]

Created attachment 46511 [details]
Updated Patch

I think this is an improvement - only call closewebinspector() when necessary. Also clean up some whitespace.

[WebKit Review Bot]

Attachment 46511 [details] did not pass style-queue:

Failed to run "WebKitTools/Scripts/check-webkit-style" exit_code: 1
WebKitTools/DumpRenderTree/qt/DumpRenderTreeQt.cpp:80:  qt_drt_webinspector_close is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
WebKitTools/DumpRenderTree/qt/LayoutTestControllerQt.cpp:42:  qt_drt_setTimelineProfilingEnabled is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
WebKitTools/DumpRenderTree/qt/LayoutTestControllerQt.cpp:50:  qt_drt_webinspector_execute_script is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
WebKitTools/DumpRenderTree/qt/LayoutTestControllerQt.cpp:51:  qt_drt_webinspector_show is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
Total errors found: 4

[Kent Hansen]

(In reply to comment #15)
> Created an attachment (id=46511) [details]
> Updated Patch
> 
> I think this is an improvement - only call closewebinspector() when necessary.
> Also clean up some whitespace.

Cool, looking better!
I'm not a reviewer but here are some comments:
- The link to bugzilla should go right below the summary, that way I don't have to scan the rest of the changelog for it.
- Spelling error in changelog (setTimerProfiilingEnabled).
- In general I'd like to see the inspector-related part of the patch split from the rest (display(), setTimelineProfilingEnabled()), if that makes sense. Unless they are tightly related.
- I don't think LayoutController::hideWebInspector() should be removed, it should rather be renamed to closeWebInspector() (looks like it was just given the wrong name in the original impl).
- qt_drt_webinspector_execute_script --> qt_drt_webinspector_executeScript (following the naming convention of other qt_drt functions, e.g. qt_drt_overwritePluginDirectories).
- closeWebInspector() should set the DeveloperExtrasEnabled attribute to false after calling close(), as per the Mac and GTK implementations.
- Should closeWebInspector() really check m_webInspector? Since showWebInspector() bypasses the QWebInspector API, it doesn't look symmetric.
- Rather than doing the URL acrobatics in DumpRenderTree::open(), I recommend having a shouldOpenWebInspector() helper function (see Mac and GTK port), much more readable.

Looking forward to being able to run those inspector tests!

[Robert Hogan]

(In reply to comment #17)
> (In reply to comment #15)
> > Created an attachment (id=46511) [details] [details]
> > Updated Patch
> > 
> > I think this is an improvement - only call closewebinspector() when necessary.
> > Also clean up some whitespace.
> 
> Cool, looking better!
> I'm not a reviewer but here are some comments:
> - The link to bugzilla should go right below the summary, that way I don't have
> to scan the rest of the changelog for it.
> - Spelling error in changelog (setTimerProfiilingEnabled).
> - qt_drt_webinspector_execute_script --> qt_drt_webinspector_executeScript
> (following the naming convention of other qt_drt functions, e.g.
> qt_drt_overwritePluginDirectories).
> - Rather than doing the URL acrobatics in DumpRenderTree::open(), I recommend
> having a shouldOpenWebInspector() helper function (see Mac and GTK port), much
> more readable.
> 
> - I don't think LayoutController::hideWebInspector() should be removed, it
> should rather be renamed to closeWebInspector() (looks like it was just given
> the wrong name in the original impl).
> - closeWebInspector() should set the DeveloperExtrasEnabled attribute to false
> after calling close(), as per the Mac and GTK implementations.
> - Should closeWebInspector() really check m_webInspector? Since
> showWebInspector() bypasses the QWebInspector API, it doesn't look symmetric.

No problem!

> - In general I'd like to see the inspector-related part of the patch split from
> the rest (display(), setTimelineProfilingEnabled()), if that makes sense.
> Unless they are tightly related.

They are, in the sense that they're required for all twenty tests to pass - timelineprofiling for about six or seven, display() for just one.


> Looking forward to being able to run those inspector tests!

[Robert Hogan]

Created attachment 46592 [details]
Updated Patch

[WebKit Review Bot]

Attachment 46592 [details] did not pass style-queue:

Failed to run "WebKitTools/Scripts/check-webkit-style" exit_code: 1
WebKitTools/DumpRenderTree/qt/LayoutTestControllerQt.cpp:42:  qt_drt_setTimelineProfilingEnabled is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
WebKitTools/DumpRenderTree/qt/LayoutTestControllerQt.cpp:50:  qt_drt_webinspector_executeScript is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
WebKitTools/DumpRenderTree/qt/LayoutTestControllerQt.cpp:51:  qt_drt_webinspector_show is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
WebKitTools/DumpRenderTree/qt/LayoutTestControllerQt.cpp:52:  qt_drt_webinspector_close is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
Total errors found: 4

[WebKit Review Bot]

Attachment 46592 [details] did not build on qt:
Build output: http://webkit-commit-queue.appspot.com/results/187578

[Robert Hogan]

Created attachment 46599 [details]
Updated Patch per EWS

[WebKit Review Bot]

Attachment 46599 [details] did not pass style-queue:

Failed to run "WebKitTools/Scripts/check-webkit-style" exit_code: 1
WebKitTools/DumpRenderTree/qt/LayoutTestControllerQt.cpp:42:  qt_drt_setTimelineProfilingEnabled is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
WebKitTools/DumpRenderTree/qt/LayoutTestControllerQt.cpp:50:  qt_drt_webinspector_executeScript is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
WebKitTools/DumpRenderTree/qt/LayoutTestControllerQt.cpp:51:  qt_drt_webinspector_show is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
WebKitTools/DumpRenderTree/qt/LayoutTestControllerQt.cpp:52:  qt_drt_webinspector_close is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
Total errors found: 4

[Csaba Osztrogonác]

(In reply to comment #21)
> Attachment 46592 [details] did not build on qt:
> Build output: http://webkit-commit-queue.appspot.com/results/187578

Robert, you missed the closing '}' of LayoutTestController::setUserStyleSheetEnabled(). Aside of this minor error, the build and layout test finished.

I got only one failing test which should be skipped and file a bug on it:
--- /tmp/layout-test-results/inspector/timeline-paint-expected.txt	2010-01-14 12:33:10.000000000 -0800
+++ /tmp/layout-test-results/inspector/timeline-paint-actual.txt	2010-01-14 12:33:10.000000000 -0800
@@ -1,14 +1,3 @@
 Tests the Timeline API instrumentation of a paint event
 
-Paint Properties:
-+ startTime : * DEFINED *
-+ data : {
-+- x : 0
-+- y : 0
-+- width : * DEFINED *
-+- height : * DEFINED *
-+ }
-+ children : * DEFINED *
-+ endTime : * DEFINED *
-+ type : 3

Unfortunately I got a nasty feature of this patch. Regularly a new window is appeared and then it is disappeared. Could you solve it? I think it is a minor request that we can fix later. (after land) As far as I remember András fixed a similar window appearing problem previously. András, could you help us?

[Robert Hogan]

(In reply to comment #24)
> (In reply to comment #21)
> > Attachment 46592 [details] [details] did not build on qt:
> > Build output: http://webkit-commit-queue.appspot.com/results/187578
> 
> Robert, you missed the closing '}' of
> LayoutTestController::setUserStyleSheetEnabled(). Aside of this minor error,
> the build and layout test finished.
> 
> I got only one failing test which should be skipped and file a bug on it:
> --- /tmp/layout-test-results/inspector/timeline-paint-expected.txt   
> 2010-01-14 12:33:10.000000000 -0800
> +++ /tmp/layout-test-results/inspector/timeline-paint-actual.txt    2010-01-14
> 12:33:10.000000000 -0800
> @@ -1,14 +1,3 @@
>  Tests the Timeline API instrumentation of a paint event
> 
> -Paint Properties:
> -+ startTime : * DEFINED *
> -+ data : {
> -+- x : 0
> -+- y : 0
> -+- width : * DEFINED *
> -+- height : * DEFINED *
> -+ }
> -+ children : * DEFINED *
> -+ endTime : * DEFINED *
> -+ type : 3
> 

I had exactly that behaviour (intermittently) in previous patches. However with the version for review it has passed for me without fail. It is the same test that displays the webview as you describe below - the test relies on timelining a paint event so displaying the window is necessary. Why is displaying the window a problem?

> Unfortunately I got a nasty feature of this patch. Regularly a new window is
> appeared and then it is disappeared. Could you solve it? I think it is a minor
> request that we can fix later. (after land) As far as I remember András fixed a
> similar window appearing problem previously. András, could you help us?

[Csaba Osztrogonác]

I have no idea what caused it, it needs more investigation.
WebKitTools/Scripts/run-webkit-tests inspector/timeline-paint.html --iterations 100

58 test cases (58%) succeeded
42 test cases (42%) had incorrect layout

---
inspector/timeline-paint.html -> failed
.
inspector/timeline-paint.html -> failed
..........................
inspector/timeline-paint.html -> failed
.
inspector/timeline-paint.html -> failed
.
inspector/timeline-paint.html -> failed
---


Displaying window isn't a major problem, but it is unkindly if you would like to work during test is running in background.

[Csaba Osztrogonác]

It is a more complicant than I thought previously.

I ran "WebKitTools/Scripts/run-webkit-tests inspector --iterations 10".
181 test cases (90%) succeeded
18 test cases (9%) had incorrect layout
1 test case (<1%) crashed

inspector/timeline-paint.html failed 9 times
inspector/elements-panel-selection-on-refresh.html failed 9 times
inspector/elements-panel-structure.html crashed once.

[Eric Seidel (no email)]

We've been seeing inspector and profiler crashes on other platforms.  If you're unskipping tests and they're starting to fail/crash they might be related to those other bugs.  Search "inspector crash" and "profiler crash" to see other bugs.

[Robert Hogan]

Comment on attachment 46599 [details]
Updated Patch per EWS

If I can reproduce these crashes/failures when running --iterations on inspector with a gtk build I'll resubmit this with the timeline-paint.html test skipped.

[Andras Becsi]

I did not look thoroughly into the related code yet, so I do not see how the display() function should work correctly (oher platforms call displayWebView() in display()), but the Macs do not show any windows during the testing process, so I think showing one is not a correct behaviour. DumpRenderTree is supposed to be a console application, except it needs some X stuff to work correctly, but it should by no means show a widget during testing.

[Robert Hogan]

Created attachment 46741 [details]
Updated Patch

OK, this seems to be robust!

robert@mwenge:~/Development/webkit$ WebKitTools/Scripts/run-webkit-tests --qt inspector --iterations 10
Running build-dumprendertree
Running tests from /home/robert/Development/webkit/LayoutTests
Testing 19 test cases 10 times.
inspector ..............................................................................................................................................................................................
70.22s total testing time

all 190 test cases succeeded

timeline-paint.html still doesn't work consistently so I'm skipping it for now.

[WebKit Review Bot]

Attachment 46741 [details] did not pass style-queue:

Failed to run "WebKitTools/Scripts/check-webkit-style" exit_code: 1
WebKit/qt/WebCoreSupport/InspectorClientQt.cpp:123:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
WebKit/qt/WebCoreSupport/InspectorClientQt.cpp:124:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
WebKit/qt/WebCoreSupport/InspectorClientQt.cpp:125:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
WebKit/qt/WebCoreSupport/InspectorClientQt.cpp:126:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
WebKitTools/DumpRenderTree/qt/LayoutTestControllerQt.cpp:42:  qt_drt_setTimelineProfilingEnabled is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
WebKitTools/DumpRenderTree/qt/LayoutTestControllerQt.cpp:50:  qt_drt_webinspector_executeScript is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
WebKitTools/DumpRenderTree/qt/LayoutTestControllerQt.cpp:51:  qt_drt_webinspector_show is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
WebKitTools/DumpRenderTree/qt/LayoutTestControllerQt.cpp:52:  qt_drt_webinspector_close is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
Total errors found: 8

[Andras Becsi]

(In reply to comment #31)
> Created an attachment (id=46741) [details]
> Updated Patch
Great work!
There are some occasional crashes on the test bot (1 out of 100 runs) but these may be related to other bugs Eric mentioned.
Please update the changelog text now that you did not implement display().
And also, please use 4-space indent (not 3) as the style-bot pointed out:

(In reply to comment #32)
> Attachment 46741 [details] did not pass style-queue:
> 
> Failed to run "WebKitTools/Scripts/check-webkit-style" exit_code: 1
> WebKit/qt/WebCoreSupport/InspectorClientQt.cpp:123:  Weird number of spaces at
> line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
> WebKit/qt/WebCoreSupport/InspectorClientQt.cpp:124:  Weird number of spaces at
> line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
> WebKit/qt/WebCoreSupport/InspectorClientQt.cpp:125:  Weird number of spaces at
> line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
> WebKit/qt/WebCoreSupport/InspectorClientQt.cpp:126:  Weird number of spaces at
> line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]

[Robert Hogan]

Created attachment 46743 [details]
Updated Patch

Thanks for spotting those Andras!

[WebKit Review Bot]

Attachment 46743 [details] did not pass style-queue:

Failed to run "WebKitTools/Scripts/check-webkit-style" exit_code: 1
WebKitTools/DumpRenderTree/qt/LayoutTestControllerQt.cpp:42:  qt_drt_setTimelineProfilingEnabled is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
WebKitTools/DumpRenderTree/qt/LayoutTestControllerQt.cpp:50:  qt_drt_webinspector_executeScript is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
WebKitTools/DumpRenderTree/qt/LayoutTestControllerQt.cpp:51:  qt_drt_webinspector_show is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
WebKitTools/DumpRenderTree/qt/LayoutTestControllerQt.cpp:52:  qt_drt_webinspector_close is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
Total errors found: 4

[Simon Hausmann]

Comment on attachment 46743 [details]
Updated Patch

In general this looks like a really good patch! Excellent work with improving our test coverage.



> +void QWEBKIT_EXPORT qt_drt_webinspector_executeScript(QWebPage *page, long callId, const QString &script)
> +{
> +    if (!page->handle()->page->inspectorController())
> +        return;
> +    page->handle()->page->inspectorController()->evaluateForTestInFrontend(callId, script);
> +}
> +
> +void QWEBKIT_EXPORT qt_drt_webinspector_close(QWebPage *page)
> +{
> +    if (!page->handle()->page->inspectorController())
> +        return;
> +    page->handle()->page->inspectorController()->close();
> +}
> +
> +void QWEBKIT_EXPORT qt_drt_webinspector_show(QWebPage *page)
> +{
> +    if (!page->handle()->page->inspectorController())
> +        return;
> +    page->handle()->page->inspectorController()->show();
> +}

This approach is fine with me, but the '*' position is incorrect (coding style nitpick :)


> diff --git a/WebKit/qt/WebCoreSupport/InspectorClientQt.cpp b/WebKit/qt/WebCoreSupport/InspectorClientQt.cpp
> index 1caf96d..6e3bd5c 100644
> --- a/WebKit/qt/WebCoreSupport/InspectorClientQt.cpp
> +++ b/WebKit/qt/WebCoreSupport/InspectorClientQt.cpp
> @@ -120,6 +120,9 @@ void InspectorClientQt::showWindow()
>  void InspectorClientQt::closeWindow()
>  {
>  #if ENABLE(INSPECTOR)
> +    // QWebInspector::frontEnd is a copy of inspectorView which will be left dangling
> +    // by m_frontend.set(0) in InspectorController::close() if we do not clear it here.
> +    m_inspectedWebPage->d->getOrCreateInspector()->d->setFrontend(0);
>      m_inspectedWebPage->d->inspectorController()->setWindowVisible(false);
>  #endif

Jocelyn, does that look okay to you? (you have a better idea of the inspector workings than I do :)

>
> +static bool isWebInspectorTest(const QUrl& url)
> +{
> +    if (url.toString().contains("inspector/"))

Is there an easier way to do this? Maybe just url.path().contains() instead of the expensive toString() conversion?

r- because of the style bit, and I'd like to hear Jocelyn's opinion on the InspectorClientQt.cpp change.

[Jocelyn Turcotte]

> > diff --git a/WebKit/qt/WebCoreSupport/InspectorClientQt.cpp b/WebKit/qt/WebCoreSupport/InspectorClientQt.cpp
> > index 1caf96d..6e3bd5c 100644
> > --- a/WebKit/qt/WebCoreSupport/InspectorClientQt.cpp
> > +++ b/WebKit/qt/WebCoreSupport/InspectorClientQt.cpp
> > @@ -120,6 +120,9 @@ void InspectorClientQt::showWindow()
> >  void InspectorClientQt::closeWindow()
> >  {
> >  #if ENABLE(INSPECTOR)
> > +    // QWebInspector::frontEnd is a copy of inspectorView which will be left dangling
> > +    // by m_frontend.set(0) in InspectorController::close() if we do not clear it here.
> > +    m_inspectedWebPage->d->getOrCreateInspector()->d->setFrontend(0);
> >      m_inspectedWebPage->d->inspectorController()->setWindowVisible(false);
> >  #endif
> 
> Jocelyn, does that look okay to you? (you have a better idea of the inspector
> workings than I do :)
> 

From my understanding, calling m_frontend.set(0) should have no effect on the life of InspectorClientQt::m_inspectorView (and QWebInspector's frontend).
InspectorController::m_frontend is a proxy object to call javascript functions and forward data in the inspector web page.

Were you able to make it crash without this line just by running the tests?

[Robert Hogan]

(In reply to comment #37)
> From my understanding, calling m_frontend.set(0) should have no effect on the
> life of InspectorClientQt::m_inspectorView (and QWebInspector's frontend).
> InspectorController::m_frontend is a proxy object to call javascript functions
> and forward data in the inspector web page.
> 
> Were you able to make it crash without this line just by running the tests?

Yes, exactly. The DRT closes the webinspector directly using inspectorController()->close() - which is what the other DRTs do too. The same QWebPage persists between tests so that's all that's required, it also means that the DRT doesn't have to worry about displaying the webinspector.

You're right that my comment is incorrect - m_frontend is a red herring.

The correct explanation is:

Because Qt keeps a copy of the inspectorView value in QWebInspector::frontend, it means that when you call inspectorController()->close() inspectorView is going to get deleted by:

    if (m_page) {
        if (!m_page->mainFrame() || !m_page->mainFrame()->loader() || !m_page->mainFrame()->loader()->isLoading()) {
            m_page->setParentInspectorController(0);
            m_page = 0;
        }
    }

leaving the reference stored in QWebInspectorPrivate::frontend dangling. This comes back to bite when the InspectorController goes to create the inspector page for the next test in the DRT run when we call InspectorController::show(): a new m_page gets created(m_page = m_client->createPage();) and when setFrontEnd() gets called there it hits:

if (frontend)
        frontend->setParent(0);

and since 'frontend' (i.e. m_page) was actually deleted by the InspectorController it crashes when attempting to setParent.

[Robert Hogan]

(In reply to comment #38)
> 
> and since 'frontend' (i.e. m_page) was actually deleted by the
> InspectorController it crashes when attempting to setParent.

Hah, another red herring I'm afraid - the actual culprit is:

Page* InspectorClientQt::createPage()
{
<..>
    m_inspectorView.set(inspectorView);
<..>
    m_inspectedWebPage->d->getOrCreateInspector()->d->setFrontend(inspectorView);

 m_inspectorView.set(inspectorView) deletes the inspectorView for which setFrontEnd has a dangling pointer, and so crashes at:

void QWebInspectorPrivate::setFrontend(QWidget* newFrontend)
{
    if (frontend)
        frontend->setParent(0);

[Robert Hogan]

Created attachment 47366 [details]
Updated Patch

[Jocelyn Turcotte]

Created attachment 47394 [details]
Suggested fix for the inspector page recreation crash.

(In reply to comment #39)
> Page* InspectorClientQt::createPage()
> {
> <..>
>     m_inspectorView.set(inspectorView);
> <..>
>    
> m_inspectedWebPage->d->getOrCreateInspector()->d->setFrontend(inspectorView);
> 
>  m_inspectorView.set(inspectorView) deletes the inspectorView for which
> setFrontEnd has a dangling pointer, and so crashes at:

Ahhh, I see!

Then it might be even better not to recreate the page each time.
What do you think about fixing it this way instead?

[Robert Hogan]

(In reply to comment #41)
> 
> Ahhh, I see!
> 
> Then it might be even better not to recreate the page each time.
> What do you think about fixing it this way instead?

That decision is made by WebCore in show() and close() so seems difficult to work around:

void InspectorController::show()
{
<..>
    if (!m_page) {
        if (m_frontend)
            return;  // We are using custom frontend - no need to create page.

        m_page = m_client->createPage();
        if (!m_page)
            return;
        m_page->setParentInspectorController(this);

        // showWindow() will be called after the page loads in scriptObjectReady()
        return;
    }

    showWindow();
}


and

void InspectorController::close()
{
<..>
    closeWindow();
<..>
    if (m_page) {
        if (!m_page->mainFrame() || !m_page->mainFrame()->loader() || !m_page->mainFrame()->loader()->isLoading()) {
            m_page->setParentInspectorController(0);
            m_page = 0;
        }
    }
}

I think the best thing to do might be:

Page* InspectorClientQt::createPage()
{
    QWebView* inspectorView = new QWebView;
    InspectorClientWebPage* inspectorPage = new InspectorClientWebPage(inspectorView);
    inspectorView->setPage(inspectorPage);

    // QWebInspector::frontEnd is a copy of inspectorView which will be left dangling
    // by m_inspectorView.set(inspectorView) if we do not clear it here. This can
    // happen when the inspector is managed through repeated calls to
    // inspectorController()->show() and inspectorController()->close()
    // on the same QWebPage such as by the Qt DRT.
    m_inspectedWebPage->d->getOrCreateInspector()->d->setFrontend(0);
    m_inspectorView.set(inspectorView);
    <..>
}

After all, if we're going to delete an object, best to ensure that all other references to it are cleared first!

[Robert Hogan]

(In reply to comment #42)
> 
> After all, if we're going to delete an object, best to ensure that all other
> references to it are cleared first!

Ah sorry, didn't see your patch when I commented - see what you mean now.

[Robert Hogan]

Created attachment 47437 [details]
Updated Patch

Updated with Jocelyn's crash fix - also unskip more tests. 26 in total now pass:

webkit$ WebKitTools/Scripts/run-webkit-tests --qt --repeat-each 4  inspector
Running build-dumprendertree
Running tests from /home/robert/Development/webkit/LayoutTests
Testing 26 test cases, repeating each test 4 times.
inspector ........................................................................................................
35.41s total testing time

all 104 test cases succeeded

[Simon Hausmann]

Comment on attachment 47437 [details]
Updated Patch


> +void QWEBKIT_EXPORT qt_drt_webinspector_executeScript(QWebPage* page, long callId, const QString &script)

Coding style nitpick :) (& placement)

>  void DumpRenderTree::open(const QUrl& aurl)
>  {
> +    static QUrl url(aurl);
> +
>      resetToConsistentStateBeforeTesting();
>  
> -    QUrl url = aurl;
> +    if (isWebInspectorTest(url))
> +        layoutTestController()->closeWebInspector();
> +
> +    url = aurl;
> +
> +    if (isWebInspectorTest(url))
> +        layoutTestController()->showWebInspector();
> +

Please don't use a global QUrl object to maintain whether the inspector should be closed on the next run or not.

I suggest either a boolean (where the name indicates what's happening), or see if I can tie it to some other property,
for example to whether DeveloperExtras are enabled or not. That is unless you'd like to show it once the inspector tests being, keep it "visible" and then close it once the last inspector test has finished.


It's a bit sad that we can't use the public API to properly hide/show the inspector but have to resort to private drt hooks :(

[Robert Hogan]

Created attachment 47552 [details]
Updated Patch

> It's a bit sad that we can't use the public API to properly hide/show the
> inspector but have to resort to private drt hooks :(

Fixed everything but this one I think!

[Simon Hausmann]

Comment on attachment 47552 [details]
Updated Patch

>  void DumpRenderTree::open(const QUrl& aurl)
>  {
> +    static bool previousTestUsedWebInspector = false;
> +

A boolean is better, but my main complaint was the fact that you're using a global object to represent state that can be stored in a simple member variable of the DumpRenderTree class :)

Please make it a member variable.

Come to think of it: Is a boolean needed at all? Can't you simply check the currently loaded URL, i.e.

    if (isWebInspectorTest(m_page->mainFrame()->url()))
        layoutTestController()->closeWebInspector();

>      resetToConsistentStateBeforeTesting();
>  
>      QUrl url = aurl;
> +    if (previousTestUsedWebInspector)
> +        layoutTestController()->closeWebInspector();
> +
> +    if (isWebInspectorTest(url)) {
> +        layoutTestController()->showWebInspector();
> +        previousTestUsedWebInspector = true;
> +    }
> +
>      m_expectedHash = QString();
>      if (m_dumpPixels) {

[Robert Hogan]

Created attachment 48717 [details]
Updated Patch

[Csaba Osztrogonác]

(In reply to comment #48)
> Created an attachment (id=48717) [details]
> Updated Patch

$ WebKitTools/Scripts/run-webkit-tests inspector --iterations 100
Testing 28 test cases 100 times.
inspector .......
1682.29s total testing time
all 2800 test cases succeeded

Cool, it seems it works without crashes. ;)

[Simon Hausmann]

Comment on attachment 48717 [details]
Updated Patch

LGTM. Thanks! :)

Robert, do you want to land it yourself or go through the commit queue?

[Csaba Osztrogonác]

Comment on attachment 48717 [details]
Updated Patch

AFAIK Robert isn't a commiter yet, so I cq+-ed it.

[WebKit Commit Bot]

Comment on attachment 48717 [details]
Updated Patch

Clearing flags on attachment: 48717

Committed r54772: <http://trac.webkit.org/changeset/54772>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.