[webkit-reviews] review denied: [Bug 90963] [User Timing] Implementation of User Timing W/O Getter Method : [Attachment 156330] Patch

[bugzilla-daemon at webkit.org]

Tony Gentilcore <tonyg 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 156330: Patch
https://bugs.webkit.org/attachment.cgi?id=156330&action=review

------- Additional Comments from Tony Gentilcore <tonyg at chromium.org>
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-expecte
d.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_clearM
arks.html:28
> +		   var non_retained_entries =
context.getEntriesByName(mark_names[i] ,'mark');

s/ ,/, /

>
LayoutTests/http/tests/w3c/webperf/proposal/user-timing/test_user_timing_clearM
arks.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_clearM
arks.html:45
> +		   var non_retained_entries =
context.getEntriesByName(mark_names[i] ,'mark');

s/ ,/, /

>
LayoutTests/http/tests/w3c/webperf/proposal/user-timing/test_user_timing_clearM
arks.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_clearM
easures.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