Bug 31402 – WebInspector: timeline-paint.html test fails on Windows and GTK+ platforms

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

[Eric Ayers]

The timeline API test isn't finding a Paint event in the timeline data.  See error in:

http://build.webkit.org/results/Windows%20Release%20%28Tests%29/r50871%20%286093%29.zip

the instrumentation point being tested is in WebCore/frame/FrameView.cpp  FrameView::paintContents().

I will create a windows and/or gtk build this morning to repro this.

[Eric Ayers]

It looks like the current timeline instrumentation point for Paint might be unintentionally mac specific.  Although FrameView::paintContents() isn't in a mac specific dir, it is invoked from a long list of calls originating from WebKit/mac/WebView/WebHTMLView.mm

[Eric Ayers]

Fixing the GTK port is easy, we'll just add width, height properties to the list of properties to ignore.

[Adam Roben (:aroben)]

(In reply to comment #1)
> It looks like the current timeline instrumentation point for Paint might be
> unintentionally mac specific.  Although FrameView::paintContents() isn't in a
> mac specific dir, it is invoked from a long list of calls originating from
> WebKit/mac/WebView/WebHTMLView.mm

On Windows and GTK+, it should be getting called from ScrollView::paint.

[Adam Roben (:aroben)]

I tried running the test under Visual Studio, and it looks like FrameView::inspectorTimelineAgent is returning 0.

[Eric Ayers]

(In reply to comment #4)
> I tried running the test under Visual Studio, and it looks like
> FrameView::inspectorTimelineAgent is returning 0.

You have to enable the timeline agent first.  The way I do it manually is to inspect the inspector, open up the JavaScript console, and type:

InspectorController.startTimelineProfiler()
WebInspector.addRecordToTimeline = function(arg) {console.log(JSON.stringify(arg));}

I've run Safari on Windows and used the Inspector console to verify that paint records are indeed being generated on Windows in that case.  But dumping the records from DumpRenderTree does not show any paint records coming out of the timeline at all.  I'm wondering if they are being optimized away.

[Adam Roben (:aroben)]

It looks like startTimelineProfiler isn't always getting called before the test completes.

[Adam Roben (:aroben)]

(In reply to comment #6)
> It looks like startTimelineProfiler isn't always getting called before the test
> completes.

But it does sometimes get called.

[Adam Roben (:aroben)]

In the cases where it does get called, the page doesn't get a chance to paint before stopTimelineProfiler() is called (in response to the Web Inspector being closed).

[Eric Ayers]

Created attachment 43117 [details]
WebInspector: fixes timeline-paint.html test for Windows.

I would appreciate someone with a known good Windows setup to run the unit tests.  All the fast/repaint tests fail for me even before I applied my patch.  I am probably missing a font setup.

[Brian Weinstein]

This passes all fast/repaint tests and the inspector tests, but I'm not sure who the best person to review Windows DRT patches is.

[Adam Roben (:aroben)]

Comment on attachment 43117 [details]
WebInspector: fixes timeline-paint.html test for Windows.

>  void LayoutTestController::display()
>  {
>      displayWebView();
> +
> +    // Forces a PAINT to exercise that code
> +    COMPtr<IWebView> webView;
> +    if (FAILED(frame->webView(&webView)))
> +        return;
> +
> +    COMPtr<IWebViewPrivate> viewPrivate;
> +    if (FAILED(webView->QueryInterface(&viewPrivate)))
> +        return;
> +
> +    HWND webViewWindow;
> +    if (FAILED(viewPrivate->viewWindow((OLE_HANDLE*)&webViewWindow)))
> +        return;
> +    ::SendMessage(webViewWindow, WM_PAINT, 0, 0);
>  }

I don't really understand why we need to send a WM_PAINT message here. displayWebView calls InvalidateRect/UpdateWindow, which should cause a WM_PAINT message to be sent. Is that not working?

[Eric Ayers]

Comment on attachment 43117 [details]
WebInspector: fixes timeline-paint.html test for Windows.


>          var record = timelineRecords[i];
> -        if (!isTimelineOverheadRecord(record))
>              dumpTimelineRecord(record, 0);
Formatting glitch

[Eric Ayers]

Created attachment 43153 [details]
WebInspector: fixes timeline-paint.html test for Windows.

Moved the paint message into displayWebView()  It appears that UpdateWindow() was not generating a WM_PAINT message in this case.

From reading the MSDN docs, the UpdateWindow() call is pretty much just a way to conditionally send a WM_PAINT.  In this case, we really do want to force a paint, so I just swapped out the call with SendMessage(...,WM_PAINT).  This is consistent with the way dump() works.

[Adam Roben (:aroben)]

(In reply to comment #13)
> From reading the MSDN docs, the UpdateWindow() call is pretty much just a way
> to conditionally send a WM_PAINT.  In this case, we really do want to force a
> paint, so I just swapped out the call with SendMessage(...,WM_PAINT).  This is
> consistent with the way dump() works.

http://msdn.microsoft.com/en-us/library/dd145167(VS.85).aspx says:

The UpdateWindow function updates the client area of the specified window by sending a WM_PAINT message to the window if the window's update region is not empty.

But the window's update region is *not* empty in this case, since we just called InvalidateRect. Very fishy!

[Eric Ayers]

(In reply to comment #14)
> The UpdateWindow function updates the client area of the specified window by
> sending a WM_PAINT message to the window if the window's update region is not
> empty.
> 
> But the window's update region is *not* empty in this case, since we just
> called InvalidateRect. Very fishy!

I did a test of the client's rectangle with ::RectVisible() which returns 0 for this window.  I'm guessing that's why UpdateWindow() doesn't issue a WM_PAINT message.

[Adam Roben (:aroben)]

Comment on attachment 43153 [details]
WebInspector: fixes timeline-paint.html test for Windows.

> +++ WebKitTools/DumpRenderTree/win/DumpRenderTree.cpp	(working copy)
> @@ -286,7 +286,7 @@ static void initialize()
>  void displayWebView()
>  {
>      ::InvalidateRect(webViewWindow, 0, TRUE);
> -    ::UpdateWindow(webViewWindow);
> +    ::SendMessage(webViewWindow, WM_PAINT, 0, 0);
>  }

It would be good to add a comment here about why UpdateWindow doesn't work. It would also be good to explain this in the ChangeLog.

In general I think you should explain all the changes you made in your ChangeLog (e.g., the change to timelineNonDeterministicProps, the new call to layoutTestController.display(), the removal of isTimelineOverheadRecord(), etc.)

r=me, though, even if you don't add those comments. But I really think you should!

[WebKit Commit Bot]

Comment on attachment 43153 [details]
WebInspector: fixes timeline-paint.html test for Windows.

Rejecting patch 43153 from commit-queue.

[e-mail] does not have committer permissions according to http://trac.webkit.org/browser/trunk/WebKitTools/Scripts/modules/committers.py.

[Eric Seidel (no email)]

Comment on attachment 43153 [details]
WebInspector: fixes timeline-paint.html test for Windows.

I think Eric meant to set cq=?  http://webkit.org/coding/contributing.html and http://trac.webkit.org/wiki/CommitQueue have more information about using the commit queue.

[WebKit Commit Bot]

Comment on attachment 43153 [details]
WebInspector: fixes timeline-paint.html test for Windows.

Rejecting patch 43153 from commit-queue.

Failed to run "['/Users/eseidel/Projects/CommitQueue/WebKitTools/Scripts/svn-apply', '--reviewer', 'Adam Roben', '--force']" exit_code: 1
Last 500 characters of output:
html
Hunk #1 FAILED at 22.
1 out of 1 hunk FAILED -- saving rejects to file LayoutTests/inspector/timeline-paint.html.rej
patching file LayoutTests/inspector/timeline-test.js
Hunk #1 succeeded at 4 with fuzz 1 (offset -4 lines).
Hunk #2 FAILED at 52.
1 out of 2 hunks FAILED -- saving rejects to file LayoutTests/inspector/timeline-test.js.rej
patching file LayoutTests/platform/win/Skipped
Hunk #1 FAILED at 692.
1 out of 1 hunk FAILED -- saving rejects to file LayoutTests/platform/win/Skipped.rej

[Eric Seidel (no email)]

It looks like this patch no longer applies.  We'll need an updated one so we can land it.  Sorry this took so long to land, the commit-queue was down for 6 days due to bug 31461.

[Eric Ayers]

Created attachment 43501 [details]
WebInspector: fixes timeline-paint.html test for Windows, GTK+

re-merged patch, dropped some changes to timeline-test.js that aren't necessary after an update by pfeldman.

[Pavel Feldman]

Comment on attachment 43501 [details]
WebInspector: fixes timeline-paint.html test for Windows, GTK+

Do you want to remove test from Skipped in GTK?

[Eric Ayers]

(In reply to comment #22)
> (From update of attachment 43501 [details])
> Do you want to remove test from Skipped in GTK?
Yes, its safe to remove the test from skipped.  The blacklisting of 'width' and 'height' properties allows it to pass.

[Pavel Feldman]

(In reply to comment #23)
> (In reply to comment #22)
> > (From update of attachment 43501 [details] [details])
> > Do you want to remove test from Skipped in GTK?
> Yes, its safe to remove the test from skipped.  The blacklisting of 'width' and
> 'height' properties allows it to pass.

I mean do you want to make it a part of this patch?

[Eric Ayers]

Created attachment 43506 [details]
WebInspector: fixes timeline-paint.html test for Windows, GTK+

Also removes test from gtk/Skipped file

[WebKit Commit Bot]

Comment on attachment 43501 [details]
WebInspector: fixes timeline-paint.html test for Windows, GTK+

Clearing flags on attachment: 43501

Committed r51196: <http://trac.webkit.org/changeset/51196>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.