[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 #5 from pdeng6 <pan.deng at intel.com>  2012-07-12 04:13:35 PST ---
(In reply to comment #2)
> (From update of attachment 151683 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=151683&action=review
> 
> Just doing a first pass. Someone more familiar with the performance timing stuff should do a more thorough review.
> 
> > Source/WebCore/ChangeLog:5
> > +        This patch implemented mark(), measure(), clearMarks() and clearMeasures() interface of User Timing. Getters are not exposed by Performance Timeline or getMarks()/getMeasures() yet, will be future patches.
> 
> Nit: there's no official style guide here, but usually people put an extra line break between the bug line and the description.
thanks, done.
> 
> > 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.
same point to James, I will submit test case after performance timeline integrated.
> 
> > Source/WebCore/page/PerformanceUserTiming.cpp:71
> > +         restriction-default_restrictions <
> > +         (int)(sizeof(default_restrictions) / sizeof(struct restrictive_field));
> 
> This line wrapping makes it harder to read IMO. WebKit doesn't have a line length restriction, so you should only wrap if it makes the code more readable.
> 
> Also, I think we have a macro for getting the size of an array somewhere. I can't seem to find it at the moment though.
> 
thanks, done, macros looks better.
> > Source/WebCore/page/PerformanceUserTiming.cpp:81
> > +    if (m_restrictiveKeyMap.contains(markName)
> > +       && m_restrictiveKeyMap.get(markName)) {
> 
> THis would read more easily on one line IMO.
> 
agree, done.
> > Source/WebCore/page/PerformanceUserTiming.cpp:107
> > +    return;
> 
> This return doesn't do anything.
> 
removed
> > Source/WebCore/page/PerformanceUserTiming.cpp:113
> > +    double ret = 0.0;
> 
> You never set this. You can kill the local variable and just return 0.0 at the end of this method.
> 
removed
> > Source/WebCore/page/PerformanceUserTiming.cpp:114
> > +    String markName = mark;
> 
> Why create this local variable? Can't you just use mark everywhere below?
> 
no need, removed
> > Source/WebCore/page/PerformanceUserTiming.cpp:120
> > +        if (!strcmp(markName.ascii().data(), "navigationStart"))
> 
> String overrides operator==. I think you can just == here.
> 
> > Source/WebCore/page/PerformanceUserTiming.cpp:164
> > +    // markName not exist
> 
> This comment doesn't really add value as it states what the code obviously does.
removed
> 
> > Source/WebCore/page/PerformanceUserTiming.cpp:175
> > +        goto measure;
> 
> We don't typically use gotos in WebKit code. I don't think there's an official policy against it, but it's definitely not common practice. I'd prefer if you rewrote this to not use goto.
> 
thanks, this function re wrote
> > Source/WebCore/page/PerformanceUserTiming.cpp:216
> > +    if (measureName.isNull()) {
> > +        m_measuresMap.clear();
> > +        return;
> > +    }
> > +    if (m_measuresMap.contains(measureName))
> > +        m_measuresMap.remove(measureName);
> 
> This code is exactly the same as in clearMarks. Can you make a static helper function that takes a map and a string and does this then call it from clearMarks and clearMeasures?
> 
good idea, re wrote this.
> > Source/WebCore/page/PerformanceUserTiming.cpp:217
> > +    return;
> 
> Not needed.
removed
> 
> > Source/WebCore/page/PerformanceUserTiming.h:47
> > +typedef HashMap<String, bool> RestrictiveKeyMap;
> 
removed
> I don't think this typedef really adds value since it's only used in one place and the variable name is the same as the type name.
> 
removed
> > Source/WebCore/page/PerformanceUserTiming.h:48
> > +typedef HashMap<String, Vector<RefPtr<PerformanceEntry> > > PerfEntryMap;
> 
> PerformanceEntryMap would be better since it's a map of PerformanceEntry's, not PerfEntry's.
thanks, changed.

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