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

Thu Aug 2 16:57:29 PDT 2012

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

Ojan Vafai <ojan at chromium.org> changed:

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

--- Comment #16 from Ojan Vafai <ojan at chromium.org>  2012-08-02 16:57:25 PST ---
(From update of attachment 153214)
View in context: https://bugs.webkit.org/attachment.cgi?id=153214&action=review

This will probably need a round or two more of review. Please apply my comments to the one test to all the tests as they apply to all of them.

> Source/WebCore/page/Performance.cpp:196
> +    return;

Don't need this.

> Source/WebCore/page/Performance.cpp:204
> +    return;

Don't need this.

> Source/WebCore/page/Performance.idl:59
> +        // See https://dvcs.w3.org/hg/webperf/raw-file/tip/specs/UserTiming/Overview.html

Typically, we put these links in the ChangeLog description instead as they get out of date.

> Source/WebCore/page/PerformanceMark.cpp:45
> +PassRefPtr<PerformanceMark> PerformanceMark::create(const String& name, double startTime)
> +{
> +    return adoptRef(new PerformanceMark(name, startTime));
> +}
> +
> +PerformanceMark::PerformanceMark(const String& name, double startTime)
> +    : PerformanceEntry(name, "mark", startTime, 0.0)
> +{
> +}
> +
> +PerformanceMark::~PerformanceMark()
> +{
> +}

Will this ever have more code than this? You can just inline all of this into the header.

> Source/WebCore/page/PerformanceMark.idl:28
> +    // See: https://dvcs.w3.org/hg/webperf/raw-file/tip/specs/UserTiming/Overview.html

Ditto.

> Source/WebCore/page/PerformanceMeasure.cpp:45
> +PassRefPtr<PerformanceMeasure> PerformanceMeasure::create(const String& name, double startTime, double duration)
> +{
> +    return adoptRef(new PerformanceMeasure(name, startTime, duration));
> +}
> +
> +PerformanceMeasure::PerformanceMeasure(const String& name, double startTime, double duration)
> +    : PerformanceEntry(name, "measure", startTime, duration)
> +{
> +}
> +
> +PerformanceMeasure::~PerformanceMeasure()
> +{
> +}

Ditto

> Source/WebCore/page/PerformanceUserTiming.cpp:46
> +      String key;
> +      NavigationTimingFunction navigationTimingFunction;

Indent is off.

> Source/WebCore/page/PerformanceUserTiming.cpp:111
> +    if (m_restrictiveKeyMap.contains(markName)) {
> +        ec = SYNTAX_ERR;
> +        return;
> +    }

This seems really strange to me. I realize the spec says to do this, but it seems totally unnecessary to me for this to use the same namespace as the PerformanceTimeline. It will make it hard to add things to PerformanceTimeline in the future because we'll need to make sure it doesn't conflict with any user mark names. Has this been discussed on the mailing list?

> Source/WebCore/page/PerformanceUserTiming.cpp:143
> +    if (startMark.isNull()) {
> +        endTime = m_performance->webkitNow();
> +    } else if (endMark.isNull()) {

No curly's around single-line if-statements.

> LayoutTests/http/tests/w3c/webperf/proposal/user-timing/test_user_timing_clearMarks.html:1
> +ï»¿<!DOCTYPE html>

I don't think you want the BOM here (and in the other tests).

> LayoutTests/http/tests/w3c/webperf/proposal/user-timing/test_user_timing_clearMarks.html:4
> +        <meta charset="utf-8" />

Do w3c tests require this? If not, I'd drop it. The less cruft in tests the better.

> LayoutTests/http/tests/w3c/webperf/proposal/user-timing/test_user_timing_clearMarks.html:7
> +	<link rel="help" href="http://www.w3.org/TR/user-timing/#extensions-performance-interface"/>        

Indentation is off.

> LayoutTests/http/tests/w3c/webperf/proposal/user-timing/test_user_timing_clearMarks.html:15
> +<script id="metadata_cache">/*
> +{
> +}
> +*/</script>

What is this?

> LayoutTests/http/tests/w3c/webperf/proposal/user-timing/test_user_timing_clearMarks.html:18
> +         function onload_test()

Typically we use camel-case js code. Do w3c tests use lowercase?

> LayoutTests/http/tests/w3c/webperf/proposal/user-timing/test_user_timing_clearMarks.html:30
> +            var entryListChecker = new performanceEntryList_checker("mark");

camel-case is perferred.

> LayoutTests/http/tests/w3c/webperf/proposal/user-timing/test_user_timing_clearMarks.html:46
> +/**/

Is this accidental?

> LayoutTests/http/tests/w3c/webperf/proposal/user-timing/test_user_timing_clearMarks.html:48
> +            markNames.forEach(context.initialMarks);
> +            markNames.forEach(context.initialMarks);

Did you mean to repeat this? If so, it could use a comment explaining why.

> LayoutTests/http/tests/w3c/webperf/proposal/user-timing/test_user_timing_clearMarks.html:60
> +            markNames.forEach(context.initialMarks);
> +            markNames.forEach(context.initialMarks);

Ditto

> LayoutTests/http/tests/w3c/webperf/proposal/user-timing/test_user_timing_clearMarks.html:76
> +        <iframe id="frameContext" onload="setTimeout(onload_test(), 0);" src="/w3c/webperf/resources/blank_page_green.htm"></iframe>

Do you need the setTimeout?

> LayoutTests/http/tests/w3c/webperf/proposal/user-timing/test_user_timing_method_exist.html:30
> +        test_namespace();
> +        test_method_exists('webkitMark');
> +        test_method_exists('webkitClearMarks');
> +        test_method_exists('webkitMeasure');
> +        test_method_exists('webkitClearMeasures');
> +        test_method_exists('webkitGetEntries');
> +        test_method_exists('webkitGetEntriesByType');
> +        test_method_exists('webkitGetEntriesByName');

Since you do this here, there's no need to do it in all the other tests.

> LayoutTests/http/tests/w3c/webperf/resources/webperftestharnessextension.js:27
> +       '',
> +       '1',
> +       '2',
> +       '3',
> +       '4',
> +       '5',
> +       '6',
> +       'a',
> +       'bc',
> +       'def',
> +       'ghij',
> +       'M',
> +       false,
> +       true,
> +       undefined,
> +       null,

The expected results for these are quite large and it's hard to verify correctness. Do we really need to test all these cases? Can we just check, empty string, one number, one string? The rest of these don't actually test code paths that you just added. This doesn't need to be a generic test for all bindings code.

> LayoutTests/http/tests/w3c/webperf/resources/webperftestharnessextension.js:56
> +       [''],
> +       ['aa', 1],
> +       ['bb', true],
> +       ['cc', 'navigationStart'],
> +       ['dd', 'fetchStart'],
> +       ['ee', 'domainLookupEnd'],
> +       ['ff', 'domInteractive'],
> +       ['gg', 'loadEventStart'],
> +       ['   ', '', 1],
> +       ['aaa', '1', 2],
> +       ['bbb', 3, 'a'],
> +       ['ccc', 'a', 'false'],
> +       ['ddd', 'true', 'undefined'],
> +       ['eee', 'undefined', 'null'],
> +       ['fff', 'fetchStart', 'domInteractive'],
> +       ['ggg', 'domComplete', 'M'],
> +       ['hhh', '4', 'domLoading'],
> +       ['nav2now', 'navigationStart'],
> +       ['loadTime', 'navigationStart', 'loadEventEnd'],
> +       ['loadEventEnd2a', 'loadEventEnd', 'a'],
> +       ['nav2a', 'navigationStart', 'a'],
> +       ['domComplete2a', 'domComplete', 'a'],
> +       ['onloadStart2a', 'loadEventStart', 'a'],
> +       ['negativeValue', 3, 'navigationStart'],
> +       ['nav2M', 'navigationStart', 'M'],
> +       ['loadStart2M', 'loadEventStart', 'M'],

Ditto: Can we check fewer cases? Most of these don't actually test separate code paths that you just added

> LayoutTests/http/tests/w3c/webperf/resources/webperftestharnessextension.js:62
> +    wp_test(function() { assert_true(typeof performanceNamespace[method_name] === 'function', msg); }, msg, properties);

Where is wp_test defined? Also, we prefer not to use abbreviates where they're not necessary. webPerformanceTest would be preferred (assuming that's what wp stands for!).

> LayoutTests/http/tests/w3c/webperf/resources/webperftestharnessextension.js:83
> +        var msg = 'Entry \"' + entry.name + '\" shoule be one that we have set.';

Typo: shoule

> LayoutTests/http/tests/w3c/webperf/resources/webperftestharnessextension.js:93
> +        

extra line break

> LayoutTests/http/tests/w3c/webperf/resources/webperftestharnessextension.js:126
> +    
> +

Extra ine breeak

> LayoutTests/http/tests/w3c/webperf/resources/webperftestharnessextension.js:194
> +            "mark":mark,
> +            "measure":measure,
> +            "initialMarks":initialMarks,
> +            "initialMeasures":initialMeasures,
> +            "clearMarks":clearMarks,
> +            "clearMeasures":clearMeasures, 
> +            "getEntries":getEntries,
> +            "getEntriesByType":getEntriesByType,
> +            "getEntriesByName":getEntriesByName,
> +           };

This should just be a 4 space ident.

> LayoutTests/platform/chromium/TestExpectations:3714
> +BUGWK90963 SKIP : http/tests/w3c/webperf/proposal/user-timing/test_user_timing_clearMarks.html = PASS TEXT
> +BUGWK90963 SKIP : http/tests/w3c/webperf/proposal/user-timing/test_user_timing_clearMeasures.html = PASS TEXT
> +BUGWK90963 SKIP : http/tests/w3c/webperf/proposal/user-timing/test_user_timing_mark.html = PASS TEXT
> +BUGWK90963 SKIP : http/tests/w3c/webperf/proposal/user-timing/test_user_timing_mark_exception.html = PASS TEXT
> +BUGWK90963 SKIP : http/tests/w3c/webperf/proposal/user-timing/test_user_timing_measure.html = PASS TEXT
> +BUGWK90963 SKIP : http/tests/w3c/webperf/proposal/user-timing/test_user_timing_measure_exception.html = PASS TEXT
> +BUGWK90963 SKIP : http/tests/w3c/webperf/proposal/user-timing/test_user_timing_method_exist.html = PASS TEXT

You don't need to skip these all separately. You can skip the whole directory the same way you do with the Skipped files.

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