[Webkit-unassigned] [Bug 90963] [User Timing] Implementation of User Timing W/O Getter Method

Tue Oct 16 12:30:48 PDT 2012

https://bugs.webkit.org/show_bug.cgi?id=90963

Tony Gentilcore <tonyg at chromium.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
 Attachment #168949|review?                     |review-
               Flag|                            |

--- Comment #24 from Tony Gentilcore <tonyg at chromium.org>  2012-10-16 12:31:36 PST ---
(From update of attachment 168949)
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_clearMarks.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.

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.