[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


Ojan Vafai <ojan at chromium.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
 Attachment #151683|review?                     |review-
               Flag|                            |




--- Comment #2 from Ojan Vafai <ojan at chromium.org>  2012-07-11 09:12:56 PST ---
(From update of attachment 151683)
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.

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