[webkit-reviews] review denied: [Bug 90963] [User Timing] Implementation of User Timing W/O Getter Method : [Attachment 168949] Patch
bugzilla-daemon at webkit.org
bugzilla-daemon at webkit.org
Tue Oct 16 12:30:45 PDT 2012
Tony Gentilcore <tonyg at chromium.org> has denied pdeng6 <pan.deng at intel.com>'s
request for review:
Bug 90963: [User Timing] Implementation of User Timing W/O Getter Method
https://bugs.webkit.org/show_bug.cgi?id=90963
Attachment 168949: Patch
https://bugs.webkit.org/attachment.cgi?id=168949&action=review
------- Additional Comments from Tony Gentilcore <tonyg at chromium.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=168949&action=review
Just a few more comments.
> Source/WebCore/page/Performance.idl:55
> +#if defined(ENABLE_USER_TIMING) && ENABLE_USER_TIMING
I recommend adding a comment with a link to the spec here.
> Source/WebCore/page/PerformanceEntryList.h:36
> +#include "PerformanceEntry.h"
I think the forward declaration is enough, right? If not and I'm missing
something, then please remove the forward declaration in favor of this.
>
LayoutTests/http/tests/w3c/webperf/resources/webperftestharnessextension.js:91
> + arguments.length == 0 ? performanceContext.webkitMark() :
performanceContext.webkitMark(arguments[0]);
In order to upstream these tests, we should use polyfill instead of hardcoding
the webkit prefixes.
For example:
performance.mark = (function() {
return performance.mark ||
performance.mozMark ||
performance.msMark ||
performance.oMark ||
performance.webkitMark;
})();
Up to you if you want to tackle that in this patch or not.
>
LayoutTests/http/tests/w3c/webperf/resources/webperftestharnessextension.js:101
> + if (arguments.length == 0)
Instead of all this, you can do:
performanceContext.webkitMeasure.apply(this, arguments);
Same for initialMeasures, clearMarks, clearMeasures.
>
LayoutTests/http/tests/w3c/webperf/resources/webperftestharnessextension.js:147
> + "mark":mark,
Looks like it would be better to make this a class.
function PerformanceContext()
{
}
PerformanceContext.prototype.mark = function()
{
...
}
...
>
LayoutTests/http/tests/w3c/webperf/submission/user-timing/test_user_timing_clea
rMarks.html:66
> + <iframe id="frameContext" onload="onload_test()"
src="/w3c/webperf/resources/blank_page_green.htm"></iframe>
These tests all seem to create an iframe for no reasons. Please simplify them
and just call onload_test from <body> and remove the iframe.
> LayoutTests/platform/qt-5.0-wk2/TestExpectations:620
> +# https://bugs.webkit.org/show_bug.cgi?id=90963
I'd remove the link to this bug here and in the qt expectations because bug
84893 is already referenced.
More information about the webkit-reviews
mailing list