[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


Tony Gentilcore <tonyg at chromium.org> changed:

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




--- Comment #21 from Tony Gentilcore <tonyg at chromium.org>  2012-10-15 17:23:52 PST ---
(From update of attachment 156330)
View in context: https://bugs.webkit.org/attachment.cgi?id=156330&action=review

Just a few stylistic nits. This will need a rebase and then everything else looks good to me.

> Source/WebCore/page/Performance.cpp:194
> +

You can remove the extra line break here and on line 202 to be consistent with the above methods.

> Source/WebCore/page/Performance.h:114
> +    mutable RefPtr<UserTiming> m_userTiming;

This doesn't need to be mutable as the methods that touch it aren't const.

> Source/WebCore/page/PerformanceUserTiming.cpp:26
> +

nit: there's an extra line break here.

> Source/WebCore/page/PerformanceUserTiming.cpp:37
> +namespace WebCore {

Please add a line break above namespace

> Source/WebCore/page/PerformanceUserTiming.cpp:44
> +    static const struct RestrictiveField {

This should be named RestrictedField.

> Source/WebCore/page/PerformanceUserTiming.cpp:91
> +     }

Indentation is off

> Source/WebCore/page/PerformanceUserTiming.h:62
> +    static HashMap<String, NavigationTimingFunction> m_restrictiveKeyMap;

This should be named m_restrictedKeyMap, also I recommend moving it up a line and separating it by a line break from the ctor and the first non-static method.

> LayoutTests/ChangeLog:10
> +        * http/tests/w3c/webperf/proposal/user-timing/test_user_timing_clearMarks-expected.txt: Added.

The directory name of these tests should be submission instead of proposal.

> LayoutTests/http/tests/w3c/webperf/proposal/user-timing/test_user_timing_clearMarks.html:28
> +                var non_retained_entries = context.getEntriesByName(mark_names[i] ,'mark');

s/ ,/, /

> LayoutTests/http/tests/w3c/webperf/proposal/user-timing/test_user_timing_clearMarks.html:30
> +                test_equals(non_retained_entries.length, 0,'Marks that we cleared should not exist anymore.');

Nit: space after comma

> LayoutTests/http/tests/w3c/webperf/proposal/user-timing/test_user_timing_clearMarks.html:45
> +                var non_retained_entries = context.getEntriesByName(mark_names[i] ,'mark');

s/ ,/, /

> LayoutTests/http/tests/w3c/webperf/proposal/user-timing/test_user_timing_clearMarks.html:47
> +                test_equals(non_retained_entries.length, 0,'Marks that we cleared should not exist anymore.');

Nit: space after comma

> LayoutTests/http/tests/w3c/webperf/proposal/user-timing/test_user_timing_clearMeasures.html:29
> +                var non_retained_entries = context.getEntriesByName(measures[i][0] ,'measure');

s/ ,/, /

> LayoutTests/http/tests/w3c/webperf/resources/webperftestharnessextension.js:12
> +       '',

indent only 4 spaces

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