[Webkit-unassigned] [Bug 90963] [User Timing] Implementation of User Timing W/O Getter Method
bugzilla-daemon at webkit.org
bugzilla-daemon at webkit.org
Fri Aug 3 03:53:31 PDT 2012
https://bugs.webkit.org/show_bug.cgi?id=90963
--- Comment #17 from pdeng6 <pan.deng at intel.com> 2012-08-03 03:53:27 PST ---
(In reply to comment #16)
> (From update of attachment 153214 [details])
> 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.
>
thanks for your comment:-)
> > Source/WebCore/page/Performance.cpp:196
> > + return;
>
> Don't need this.
>
yep, done
> > Source/WebCore/page/Performance.cpp:204
> > + return;
>
> Don't need this.
>
yep, done
> > 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.
>
yep, move this to ChangeLog
> > 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.
>
no, all inlined in .h, and removed this file
> > Source/WebCore/page/PerformanceMark.idl:28
> > + // See: https://dvcs.w3.org/hg/webperf/raw-file/tip/specs/UserTiming/Overview.html
>
> Ditto.
>
done
> > 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
>
done
> > Source/WebCore/page/PerformanceUserTiming.cpp:46
> > + String key;
> > + NavigationTimingFunction navigationTimingFunction;
>
> Indent is off.
>
thanks, done,
> > 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?
>
About mark names no conflict with Navigation Timing interface, I've not seen a thread about this in mailing list.
Navigation Timing spec is stable, and navigation timing2 would have natural different namespace as PerformanceNavigationTiming have different types from “mark”. My personal view is SYNTAX_ERR for conflicts is useful to remind user to avoid mark name confusion.
You can raise this issue to W3C.
> > 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.
>
done
> > 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).
>
yep, removed.
> > 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.
>
I didn't find a official test case guideline of W3C, however, every .html that was adopted as W3C navigation timing include this. so I suggest we keep this, how do you think of it?
> > 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.
>
done for all of them.
> > LayoutTests/http/tests/w3c/webperf/proposal/user-timing/test_user_timing_clearMarks.html:15
> > +<script id="metadata_cache">/*
> > +{
> > +}
> > +*/</script>
>
> What is this?
>
I think we don't need this currently.
> > 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?
>
yes, they always 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?
>
removed this.
> > 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.
>
yep, added comment for this.
> > LayoutTests/http/tests/w3c/webperf/proposal/user-timing/test_user_timing_clearMarks.html:60
> > + markNames.forEach(context.initialMarks);
> > + markNames.forEach(context.initialMarks);
>
> Ditto
>
yep, added comment for this.
> > 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?
>
no need, removed this.
> > 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.
>
removed from 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.
>
yep, done.
> > 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
>
I split this to general measure() test cases, and timing order associate with navigation timing, results is slim now.
> > 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!).
I agree it's short for web performance test, however it comes from w3c webperftestharness.js
>
> > LayoutTests/http/tests/w3c/webperf/resources/webperftestharnessextension.js:83
> > + var msg = 'Entry \"' + entry.name + '\" shoule be one that we have set.';
>
> Typo: shoule
>
thanks, done
> > LayoutTests/http/tests/w3c/webperf/resources/webperftestharnessextension.js:93
> > +
>
> extra line break
>
done
> > LayoutTests/http/tests/w3c/webperf/resources/webperftestharnessextension.js:126
> > +
> > +
>
> Extra ine breeak
>
done
> > 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.
>
yep, done
> > 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.
thanks a lot, done this.
--
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.
More information about the webkit-unassigned
mailing list