[webkit-reviews] review denied: [Bug 90963] [User Timing] Implementation of User Timing W/O Getter Method : [Attachment 151683] Patch
bugzilla-daemon at webkit.org
bugzilla-daemon at webkit.org
Wed Jul 11 09:12:56 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 151683: Patch
https://bugs.webkit.org/attachment.cgi?id=151683&action=review
------- Additional Comments from Ojan Vafai <ojan at chromium.org>
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.
> 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.
> 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.
> Source/WebCore/page/PerformanceUserTiming.cpp:81
> + if (m_restrictiveKeyMap.contains(markName)
> + && m_restrictiveKeyMap.get(markName)) {
THis would read more easily on one line IMO.
> Source/WebCore/page/PerformanceUserTiming.cpp:107
> + return;
This return doesn't do anything.
> 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.
> Source/WebCore/page/PerformanceUserTiming.cpp:114
> + String markName = mark;
Why create this local variable? Can't you just use mark everywhere below?
> 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.
> 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.
> 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?
> Source/WebCore/page/PerformanceUserTiming.cpp:217
> + return;
Not needed.
> Source/WebCore/page/PerformanceUserTiming.h:47
> +typedef HashMap<String, bool> RestrictiveKeyMap;
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.
> 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.
More information about the webkit-reviews
mailing list