[webkit-reviews] review denied: [Bug 90963] [User Timing] Implementation of User Timing W/O Getter Method : [Attachment 153214] Patch

Thu Aug 2 16:57:24 PDT 2012

Ojan Vafai <ojan 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 153214: Patch
https://bugs.webkit.org/attachment.cgi?id=153214&action=review

------- Additional Comments from Ojan Vafai <ojan at chromium.org>
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_clearM
arks.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_clearM
arks.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_clearM
arks.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_clearM
arks.html:15
> +<script id="metadata_cache">/*
> +{
> +}
> +*/</script>

What is this?

>
LayoutTests/http/tests/w3c/webperf/proposal/user-timing/test_user_timing_clearM
arks.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_clearM
arks.html:30
> +	       var entryListChecker = new performanceEntryList_checker("mark");

camel-case is perferred.

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

Is this accidental?

>
LayoutTests/http/tests/w3c/webperf/proposal/user-timing/test_user_timing_clearM
arks.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_clearM
arks.html:60
> +	       markNames.forEach(context.initialMarks);
> +	       markNames.forEach(context.initialMarks);

Ditto

>
LayoutTests/http/tests/w3c/webperf/proposal/user-timing/test_user_timing_clearM
arks.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.htm
l = 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.