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.
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
Fixing the GTK port is easy, we'll just add width, height properties to the list of properties to ignore.
(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.
I tried running the test under Visual Studio, and it looks like FrameView::inspectorTimelineAgent is returning 0.
(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.
It looks like startTimelineProfiler isn't always getting called before the test completes.
(In reply to comment #6) > It looks like startTimelineProfiler isn't always getting called before the test > completes. But it does sometimes get called.
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).
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.
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.
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?
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
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.
(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!
(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.
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!
Comment on attachment 43153 [details] WebInspector: fixes timeline-paint.html test for Windows. Rejecting patch 43153 from commit-queue. zundel@google.com does not have committer permissions according to http://trac.webkit.org/browser/trunk/WebKitTools/Scripts/modules/committers.py.
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.
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
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.
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.
Comment on attachment 43501 [details] WebInspector: fixes timeline-paint.html test for Windows, GTK+ Do you want to remove test from Skipped in GTK?
(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.
(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?
Created attachment 43506 [details] WebInspector: fixes timeline-paint.html test for Windows, GTK+ Also removes test from gtk/Skipped file
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>
All reviewed patches have been landed. Closing bug.