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

Wed Oct 17 04:49:25 PDT 2012

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


--- Comment #26 from pdeng6 <pan.deng at intel.com>  2012-10-17 04:50:14 PST ---

> > 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.
done,
> 
> > 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.
> 
forward decl is enough here, done

> > 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.
> 
thanks, done, of course I would love to upstream that. :)
> > 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.
> 
Good idea, thanks!
> 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.
> 
all iframe removed.
> > 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.
yep, removed

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