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

[bugzilla-daemon at webkit.org]

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





--- Comment #7 from pdeng6 <pan.deng at intel.com>  2012-07-12 04:32:02 PST ---
(In reply to comment #3)
> (From update of attachment 151683 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=151683&action=review
> 
> Can you also add FIXMEs to the Performance Timeline functions indicating that they need to return User Timing too? You can fix them in a separate bug and patch.
> 
Yep, https://bugs.webkit.org/show_bug.cgi?id=91072
> >> Source/WebCore/ChangeLog:9
> >> +        No new tests, feature is not enabled in any platform currently.
> > 
> > You should still commit tests for these, but just skip them on all platforms.
> 
> I think the actual reason is that there's no way to observe it working. We haven't hooked it up to getEntries() yet. You can call the functions, but won't be able to see if they're doing the right things. I think we can hold off on tests until we have observable behavior.

agree.
> 
> > Source/WebCore/page/Performance.cpp:206
> > +double Performance::Now() const
> 
> Remove this. Just use webkitNow() for the time being. We'll switch everything over when that becomes un-prefixed. Otherwise, it's just confusing to have two.
> 
yep, removed
> > Source/WebCore/page/Performance.h:43
> > +#include <wtf/HashMap.h>
> 
> This isn't needed here.
> 
removed
> > Source/WebCore/page/Performance.h:51
> > +typedef int ExceptionCode;
> 
> I think you're supposed to include ExceptionCode.h.
> 
> > Source/WebCore/page/Performance.h:91
> > +    double Now() const;
> 
> Remove this.
> 
removed
> > Source/WebCore/page/Performance.idl:63
> > +        //[Custom] Array getMarks(in [Optional] DOMString markName);
> 
> You can remove these now. It's been settled.
> 
yep, removed
> > Source/WebCore/page/PerformanceMark.cpp:37
> > +
> 
> Remove one blank line here.
> 
removed
> > Source/WebCore/page/PerformanceMark.h:33
> > +#include <wtf/RefPtr.h>
> 
> Is this needed here?
> 
No. removed
> > Source/WebCore/page/PerformanceMeasure.cpp:37
> > +
> 
> No blank line here.
> 
removed
> > Source/WebCore/page/PerformanceMeasure.h:33
> > +#include <wtf/RefPtr.h>
> 
> Probably not needed.
> 
Yep, removed
> > Source/WebCore/page/PerformanceUserTiming.cpp:35
> > +#include <wtf/text/CString.h>
> 
> Is this needed?
> 
No removed.
> > Source/WebCore/page/PerformanceUserTiming.cpp:44
> > +      bool active;
> 
> Why do we need this? Isn't it always true?
> 
> > Source/WebCore/page/PerformanceUserTiming.cpp:45
> > +    } default_restrictions[] = {
> 
> Idea: You could store PerformanceTiming function pointers here. That'd simplify the lookup function below a lot.
> 
great, simplify a lot after re writing, thanks.
> > Source/WebCore/page/PerformanceUserTiming.cpp:70
> > +         restriction-default_restrictions <
> 
> If you keep this line, can you put spaces around the - operator?
> 
Done
> > Source/WebCore/page/PerformanceUserTiming.cpp:85
> > +    typedef PerfEntryMap::iterator Iterator;
> 
> This isn't needed. It's only used once on the next line.
> 
removed
> > Source/WebCore/page/PerformanceUserTiming.cpp:110
> > +double UserTiming::lookUpRecentMarks(const String& mark, ExceptionCode& ec, PerformanceTiming* navTiming)
> 
> I'd call this findExistingMarkStartTime().
> 
done
> > Source/WebCore/page/PerformanceUserTiming.cpp:196
> > +    typedef PerfEntryMap::iterator Iterator;
> 
> Not needed.
> 
removed
> > Source/WebCore/page/PerformanceUserTiming.cpp:206
> > +    return;
> 
> Not needed.
> 
removed
> > Source/WebCore/page/PerformanceUserTiming.h:52
> > +    static PassRefPtr<UserTiming> create() { return adoptRef(new UserTiming()); }
> 
> I think you should pass in Performance* to the constructor and keep it as a m_performance member. Then you don't need to pass in the extra parameter on mark(), measure() and lookUpRecentMarks().
> 
yep, done, thanks.
> > Source/WebCore/page/PerformanceUserTiming.h:62
> > +    RestrictiveKeyMap m_restrictiveKeyMap;
> 
> It'd be better to make this static. We only ever need one.
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.