Bug 42313 – Enable window.webkitPerformance (Web Timing) for chromium

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Tony Gentilcore]

Enable window.webkitPerformance (Web Timing) for chromium

[Tony Gentilcore]

Created attachment 61600 [details]
Patch

[Darin Fisher (:fishd, Google)]

Comment on attachment 61600 [details]
Patch

LayoutTests/platform/chromium/fast/dom/Window/window-properties-performance-expected.txt:1
 +  This test dumps all of the properties that are reachable from the window.webkitPerformance object, along with their types.
hmm, it seems like we'd want to invert things here so that the expected
results are the shared results, and then just add the test to the Skipped
list for other ports.  at least, this is what we did for webkitNotifications.

[Tony Gentilcore]

(In reply to comment #2)
> (From update of attachment 61600 [details])
> LayoutTests/platform/chromium/fast/dom/Window/window-properties-performance-expected.txt:1
>  +  This test dumps all of the properties that are reachable from the window.webkitPerformance object, along with their types.
> hmm, it seems like we'd want to invert things here so that the expected
> results are the shared results, and then just add the test to the Skipped
> list for other ports.  at least, this is what we did for webkitNotifications.

Just to be sure I understand you, the idea is to invert all expectations, not just for window-properties-performance-expected.txt. Right?

[Tony Gentilcore]

Created attachment 61686 [details]
Patch

[Tony Gentilcore]

(In reply to comment #3)
> (In reply to comment #2)
> > (From update of attachment 61600 [details] [details])
> > LayoutTests/platform/chromium/fast/dom/Window/window-properties-performance-expected.txt:1
> >  +  This test dumps all of the properties that are reachable from the window.webkitPerformance object, along with their types.
> > hmm, it seems like we'd want to invert things here so that the expected
> > results are the shared results, and then just add the test to the Skipped
> > list for other ports.  at least, this is what we did for webkitNotifications.
> 
> Just to be sure I understand you, the idea is to invert all expectations, not just for window-properties-performance-expected.txt. Right?

After thinking about this, that is the only thing that makes sense. Patch updated.

[Tony Gentilcore]

Oh, and there was one strange thing.

When I use run_webkit_tests.bat, it doesn't seem to be using the default expectations even though there are no longer chromium specific expectations. Am I passing it the wrong flags or does it fall back to platform/{mac|win|gtk} before the default?

If the fallback path is chromium-{mac|win|gtk} => chromium => {mac|win|gtk} => default, then I need to copy the default expectations to the chromium port in this patch.

[Tony Gentilcore]

Created attachment 61732 [details]
Add passing expectations for chromium platform

[Tony Gentilcore]

(In reply to comment #6)
> Oh, and there was one strange thing.
> 
> When I use run_webkit_tests.bat, it doesn't seem to be using the default expectations even though there are no longer chromium specific expectations. Am I passing it the wrong flags or does it fall back to platform/{mac|win|gtk} before the default?
> 
> If the fallback path is chromium-{mac|win|gtk} => chromium => {mac|win|gtk} => default, then I need to copy the default expectations to the chromium port in this patch.

Okay, it looks like that was the fallback path. So I copied the default passing expectations to the chromium port. Should be good to go now.

[Tony Gentilcore]

Created attachment 61755 [details]
Patch

[Tony Gentilcore]

I updated this patch per our offline discussion to skip instead of FAIL for other platforms. Man, that is so much better!

I also opened bugs to enable for each port.

[Darin Fisher (:fishd, Google)]

Comment on attachment 61755 [details]
Patch

LayoutTests/platform/chromium/fast/dom/Window/window-properties-performance-expected.txt:1
 +  This test dumps all of the properties that are reachable from the window.webkitPerformance object, along with their types.
this file should be unnecessary given the one here:
LayoutTests/fast/dom/Window/window-properties-performance-expected.txt

LayoutTests/platform/gtk/Skipped:5872
 +  http/tests/misc/webtiming-two-redirects.html
it seems like you should add fast/dom/Window/window-properties-performance.html to
the Skipped files as well.  what point is there in testing that webkitPerformance
is undefined for the ports that don't have webtiming enabled?  i think it is better
to skip the test for those ports.  otherwise, to enable the feature, they have to
delete the expected failure results, which is a bit unusual.

[Tony Gentilcore]

Created attachment 61768 [details]
Skip window-properties-performance.html on disabled ports

[Tony Gentilcore]

(In reply to comment #11)
> (From update of attachment 61755 [details])
> LayoutTests/platform/chromium/fast/dom/Window/window-properties-performance-expected.txt:1
>  +  This test dumps all of the properties that are reachable from the window.webkitPerformance object, along with their types.
> this file should be unnecessary given the one here:
> LayoutTests/fast/dom/Window/window-properties-performance-expected.txt
> 

Just to make sure I wasn't crazy, I tracked this down. layout_tests/port/chromium_win.py::baseline_search_path() defines the search path for Vista as [chromium-win-vista, chromium-win, chromium, win, mac].

This means that if there are platform specific results in win or mac, there need to be platform specific results in chromium.

But this is all moot as, per your comment, I've removed the platform specific results in favor of adding window-properties-performance.html to Skipped.

> LayoutTests/platform/gtk/Skipped:5872
>  +  http/tests/misc/webtiming-two-redirects.html
> it seems like you should add fast/dom/Window/window-properties-performance.html to
> the Skipped files as well.  what point is there in testing that webkitPerformance
> is undefined for the ports that don't have webtiming enabled?  i think it is better
> to skip the test for those ports.  otherwise, to enable the feature, they have to
> delete the expected failure results, which is a bit unusual.

My original thought was that it is valuable to keep that test with differing expectations to verify that window.webkitPerformance isn't exposed when the WEB_TIMING flag is off. But I see your point. Fixed and noted for the future.

[Darin Fisher (:fishd, Google)]

Comment on attachment 61768 [details]
Skip window-properties-performance.html on disabled ports

r=me

(we pick up results from platform/{mac,win} because often times we have similar
 expectations, especially for tests that output the render tree.)

[WebKit Commit Bot]

Comment on attachment 61768 [details]
Skip window-properties-performance.html on disabled ports

Rejecting patch 61768 from commit-queue.

Failed to run "[u'/Users/eseidel/Projects/CommitQueue/WebKitTools/Scripts/svn-apply', u'--reviewer', u'Darin Fisher', u'--force']" exit_code: 1
Last 500 characters of output:
k/Skipped.rej
patching file LayoutTests/platform/mac/Skipped
Hunk #1 succeeded at 287 (offset -3 lines).
patching file LayoutTests/platform/qt/Skipped
Hunk #1 FAILED at 5463.
1 out of 1 hunk FAILED -- saving rejects to file LayoutTests/platform/qt/Skipped.rej
patching file LayoutTests/platform/win/Skipped
Hunk #1 FAILED at 956.
1 out of 1 hunk FAILED -- saving rejects to file LayoutTests/platform/win/Skipped.rej
patching file WebKit/chromium/ChangeLog
patching file WebKit/chromium/features.gypi

Full output: http://webkit-commit-queue.appspot.com/results/3570044

[Tony Gentilcore]

Created attachment 61811 [details]
Patch for landing

[WebKit Commit Bot]

Comment on attachment 61811 [details]
Patch for landing

Rejecting patch 61811 from commit-queue.

Failed to run "['WebKitTools/Scripts/run-webkit-tests', '--no-launch-safari', '--exit-after-n-failures=1', '--ignore-tests', 'compositing', '--quiet']" exit_code: 1
Last 500 characters of output:
LayoutTests
Skipped list contained 'http/tests/misc/webtiming-one-redirect.html', but no file of that name could be found
Skipped list contained 'http/tests/misc/webtiming-two-redirects.html', but no file of that name could be found
Testing 20704 test cases.
http/tests/misc/webtiming-one-redirect.php -> failed

Exiting early after 1 failures. 19980 tests run.
511.18s total testing time

19979 test cases (99%) succeeded
1 test case (<1%) had incorrect layout
27 test cases (<1%) had stderr output

Full output: http://webkit-commit-queue.appspot.com/results/3590056

[Tony Gentilcore]

Committed r63559: <http://trac.webkit.org/changeset/63559>