Bug 90963 – [User Timing] Implementation of User Timing W/O Getter Method

• Status: RESOLVED
• Resolution: FIXED
• Component: WebCore Misc.

[pdeng6]

Implement mark(), measure(), clearMarks() and clearMeasures() interface of User Timing.

[pdeng6]

Created attachment 151683 [details]
Patch

[Ojan Vafai]

Comment on attachment 151683 [details]
Patch

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.

[James Simonsen]

Comment on attachment 151683 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=151683&action=review

Can you also add FIXMEs to the Performance Timeline functions indicating that they need to return User Timing too? You can fix them in a separate bug and patch.

>> 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.

I think the actual reason is that there's no way to observe it working. We haven't hooked it up to getEntries() yet. You can call the functions, but won't be able to see if they're doing the right things. I think we can hold off on tests until we have observable behavior.

> Source/WebCore/page/Performance.cpp:206
> +double Performance::Now() const

Remove this. Just use webkitNow() for the time being. We'll switch everything over when that becomes un-prefixed. Otherwise, it's just confusing to have two.

> Source/WebCore/page/Performance.h:43
> +#include <wtf/HashMap.h>

This isn't needed here.

> Source/WebCore/page/Performance.h:51
> +typedef int ExceptionCode;

I think you're supposed to include ExceptionCode.h.

> Source/WebCore/page/Performance.h:91
> +    double Now() const;

Remove this.

> Source/WebCore/page/Performance.idl:63
> +        //[Custom] Array getMarks(in [Optional] DOMString markName);

You can remove these now. It's been settled.

> Source/WebCore/page/PerformanceMark.cpp:37
> +

Remove one blank line here.

> Source/WebCore/page/PerformanceMark.h:33
> +#include <wtf/RefPtr.h>

Is this needed here?

> Source/WebCore/page/PerformanceMeasure.cpp:37
> +

No blank line here.

> Source/WebCore/page/PerformanceMeasure.h:33
> +#include <wtf/RefPtr.h>

Probably not needed.

> Source/WebCore/page/PerformanceUserTiming.cpp:35
> +#include <wtf/text/CString.h>

Is this needed?

> Source/WebCore/page/PerformanceUserTiming.cpp:44
> +      bool active;

Why do we need this? Isn't it always true?

> Source/WebCore/page/PerformanceUserTiming.cpp:45
> +    } default_restrictions[] = {

Idea: You could store PerformanceTiming function pointers here. That'd simplify the lookup function below a lot.

> Source/WebCore/page/PerformanceUserTiming.cpp:70
> +         restriction-default_restrictions <

If you keep this line, can you put spaces around the - operator?

> Source/WebCore/page/PerformanceUserTiming.cpp:85
> +    typedef PerfEntryMap::iterator Iterator;

This isn't needed. It's only used once on the next line.

> Source/WebCore/page/PerformanceUserTiming.cpp:110
> +double UserTiming::lookUpRecentMarks(const String& mark, ExceptionCode& ec, PerformanceTiming* navTiming)

I'd call this findExistingMarkStartTime().

> Source/WebCore/page/PerformanceUserTiming.cpp:196
> +    typedef PerfEntryMap::iterator Iterator;

Not needed.

> Source/WebCore/page/PerformanceUserTiming.cpp:206
> +    return;

Not needed.

> Source/WebCore/page/PerformanceUserTiming.h:52
> +    static PassRefPtr<UserTiming> create() { return adoptRef(new UserTiming()); }

I think you should pass in Performance* to the constructor and keep it as a m_performance member. Then you don't need to pass in the extra parameter on mark(), measure() and lookUpRecentMarks().

> Source/WebCore/page/PerformanceUserTiming.h:62
> +    RestrictiveKeyMap m_restrictiveKeyMap;

It'd be better to make this static. We only ever need one.

[pdeng6]

Created attachment 151902 [details]
Patch

[pdeng6]

(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.

[pdeng6]

Created attachment 151906 [details]
Patch

[pdeng6]

(In reply to comment #3)
> (From update of attachment 151683 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=151683&action=review
> 
> Can you also add FIXMEs to the Performance Timeline functions indicating that they need to return User Timing too? You can fix them in a separate bug and patch.
> 
Yep, https://bugs.webkit.org/show_bug.cgi?id=91072
> >> 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.
> 
> I think the actual reason is that there's no way to observe it working. We haven't hooked it up to getEntries() yet. You can call the functions, but won't be able to see if they're doing the right things. I think we can hold off on tests until we have observable behavior.

agree.
> 
> > Source/WebCore/page/Performance.cpp:206
> > +double Performance::Now() const
> 
> Remove this. Just use webkitNow() for the time being. We'll switch everything over when that becomes un-prefixed. Otherwise, it's just confusing to have two.
> 
yep, removed
> > Source/WebCore/page/Performance.h:43
> > +#include <wtf/HashMap.h>
> 
> This isn't needed here.
> 
removed
> > Source/WebCore/page/Performance.h:51
> > +typedef int ExceptionCode;
> 
> I think you're supposed to include ExceptionCode.h.
> 
> > Source/WebCore/page/Performance.h:91
> > +    double Now() const;
> 
> Remove this.
> 
removed
> > Source/WebCore/page/Performance.idl:63
> > +        //[Custom] Array getMarks(in [Optional] DOMString markName);
> 
> You can remove these now. It's been settled.
> 
yep, removed
> > Source/WebCore/page/PerformanceMark.cpp:37
> > +
> 
> Remove one blank line here.
> 
removed
> > Source/WebCore/page/PerformanceMark.h:33
> > +#include <wtf/RefPtr.h>
> 
> Is this needed here?
> 
No. removed
> > Source/WebCore/page/PerformanceMeasure.cpp:37
> > +
> 
> No blank line here.
> 
removed
> > Source/WebCore/page/PerformanceMeasure.h:33
> > +#include <wtf/RefPtr.h>
> 
> Probably not needed.
> 
Yep, removed
> > Source/WebCore/page/PerformanceUserTiming.cpp:35
> > +#include <wtf/text/CString.h>
> 
> Is this needed?
> 
No removed.
> > Source/WebCore/page/PerformanceUserTiming.cpp:44
> > +      bool active;
> 
> Why do we need this? Isn't it always true?
> 
> > Source/WebCore/page/PerformanceUserTiming.cpp:45
> > +    } default_restrictions[] = {
> 
> Idea: You could store PerformanceTiming function pointers here. That'd simplify the lookup function below a lot.
> 
great, simplify a lot after re writing, thanks.
> > Source/WebCore/page/PerformanceUserTiming.cpp:70
> > +         restriction-default_restrictions <
> 
> If you keep this line, can you put spaces around the - operator?
> 
Done
> > Source/WebCore/page/PerformanceUserTiming.cpp:85
> > +    typedef PerfEntryMap::iterator Iterator;
> 
> This isn't needed. It's only used once on the next line.
> 
removed
> > Source/WebCore/page/PerformanceUserTiming.cpp:110
> > +double UserTiming::lookUpRecentMarks(const String& mark, ExceptionCode& ec, PerformanceTiming* navTiming)
> 
> I'd call this findExistingMarkStartTime().
> 
done
> > Source/WebCore/page/PerformanceUserTiming.cpp:196
> > +    typedef PerfEntryMap::iterator Iterator;
> 
> Not needed.
> 
removed
> > Source/WebCore/page/PerformanceUserTiming.cpp:206
> > +    return;
> 
> Not needed.
> 
removed
> > Source/WebCore/page/PerformanceUserTiming.h:52
> > +    static PassRefPtr<UserTiming> create() { return adoptRef(new UserTiming()); }
> 
> I think you should pass in Performance* to the constructor and keep it as a m_performance member. Then you don't need to pass in the extra parameter on mark(), measure() and lookUpRecentMarks().
> 
yep, done, thanks.
> > Source/WebCore/page/PerformanceUserTiming.h:62
> > +    RestrictiveKeyMap m_restrictiveKeyMap;
> 
> It'd be better to make this static. We only ever need one.
done this.

[James Simonsen]

Comment on attachment 151906 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=151906&action=review

Thanks for the changes. Just a couple more small changes and this should be ready to commit.

> Source/WebCore/page/Performance.cpp:195
> +        m_timing = PerformanceTiming::create(m_frame);

I don't think this is necessary any more. When you call m_performance->timing() in PerformanceUserTiming, it'll be constructed automatically.

> Source/WebCore/page/Performance.idl:44
> +#if defined(ENABLE_PERFORMANCE_TIMELINE2) && ENABLE_PERFORMANCE_TIMELINE

Remove that 2.

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

I think structs are supposed to be CamelCase.

> Source/WebCore/page/PerformanceUserTiming.cpp:46
> +      functionPointer navFunctionPointer;

In general, abbreviations are discouraged in WebKit. Maybe use navigationTimingFunction?

> Source/WebCore/page/PerformanceUserTiming.cpp:71
> +    for (const struct restrictive_field* restriction = default_restrictions;

You should only do this if m_restrictiveKeyMap is empty. We only need to do it once now.

> Source/WebCore/page/PerformanceUserTiming.cpp:140
> +        return insertPerformanceEntry(m_measuresMap, PerformanceMeasure::create(measureName, startTime, endTime - startTime));

How about we just call insertPerformanceEntry once at the end of the function? So the code would look something like:

if (startMark.isNull()) {
  endTime = ...
} else if (endMark.isNull()) {
  startTime = ...
  endTime = ...
} else {
  startTime = ...
  endTime = ...
}
insertPerformanceEntry(...)

> Source/WebCore/page/PerformanceUserTiming.h:47
> +typedef int ExceptionCode;

You can delete this now. It should come from ExceptionCode.h.

> Source/WebCore/page/PerformanceUserTiming.h:48
> +typedef unsigned long long (PerformanceTiming::*functionPointer)() const;

Maybe call this NavigationTimingFunction? I think types are supposed to start upper case.

[Ojan Vafai]

(In reply to comment #3)
> (From update of attachment 151683 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=151683&action=review
> 
> Can you also add FIXMEs to the Performance Timeline functions indicating that they need to return User Timing too? You can fix them in a separate bug and patch.
> 
> >> 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.
> 
> I think the actual reason is that there's no way to observe it working. We haven't hooked it up to getEntries() yet. You can call the functions, but won't be able to see if they're doing the right things. I think we can hold off on tests until we have observable behavior.

I see. I'd rather we do this in the opposite order if possible, e.g. hook something up to getEntries first.

[pdeng6]

Created attachment 152125 [details]
Patch

[pdeng6]

(In reply to comment #8)
> (From update of attachment 151906 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=151906&action=review
> 
> Thanks for the changes. Just a couple more small changes and this should be ready to commit.
> 
> > Source/WebCore/page/Performance.cpp:195
> > +        m_timing = PerformanceTiming::create(m_frame);
> 
> I don't think this is necessary any more. When you call m_performance->timing() in PerformanceUserTiming, it'll be constructed automatically.

Yep, done.
> 
> > Source/WebCore/page/Performance.idl:44
> > +#if defined(ENABLE_PERFORMANCE_TIMELINE2) && ENABLE_PERFORMANCE_TIMELINE
> 
> Remove that 2.
> 
thanks for catch this, removed.
> > Source/WebCore/page/PerformanceUserTiming.cpp:44
> > +    static const struct restrictive_field {
> 
> I think structs are supposed to be CamelCase.
> 
> > Source/WebCore/page/PerformanceUserTiming.cpp:46
> > +      functionPointer navFunctionPointer;
> 
> In general, abbreviations are discouraged in WebKit. Maybe use navigationTimingFunction?
yes, replaced.
> 
> > Source/WebCore/page/PerformanceUserTiming.cpp:71
> > +    for (const struct restrictive_field* restriction = default_restrictions;
> 
> You should only do this if m_restrictiveKeyMap is empty. We only need to do it once now.
> 
Done
> > Source/WebCore/page/PerformanceUserTiming.cpp:140
> > +        return insertPerformanceEntry(m_measuresMap, PerformanceMeasure::create(measureName, startTime, endTime - startTime));
> 
> How about we just call insertPerformanceEntry once at the end of the function? So the code would look something like:
> 
> if (startMark.isNull()) {
>   endTime = ...
> } else if (endMark.isNull()) {
>   startTime = ...
>   endTime = ...
> } else {
>   startTime = ...
>   endTime = ...
> }
> insertPerformanceEntry(...)
> 
yep, looks better.

> > Source/WebCore/page/PerformanceUserTiming.h:47
> > +typedef int ExceptionCode;
> 
> You can delete this now. It should come from ExceptionCode.h.
> 
removed
> > Source/WebCore/page/PerformanceUserTiming.h:48
> > +typedef unsigned long long (PerformanceTiming::*functionPointer)() const;
> 
> Maybe call this NavigationTimingFunction? I think types are supposed to start upper case.
Yep, thanks.

[James Simonsen]

Comment on attachment 152125 [details]
Patch

Looks good to me. We'll need to figure out what to do about tests and you'll need reviewer approval.

[pdeng6]

(In reply to comment #9)
> (In reply to comment #3)
> > (From update of attachment 151683 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=151683&action=review
> > 
> > Can you also add FIXMEs to the Performance Timeline functions indicating that they need to return User Timing too? You can fix them in a separate bug and patch.
> > 
> > >> 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.
> > 
> > I think the actual reason is that there's no way to observe it working. We haven't hooked it up to getEntries() yet. You can call the functions, but won't be able to see if they're doing the right things. I think we can hold off on tests until we have observable behavior.
> 
> I see. I'd rather we do this in the opposite order if possible, e.g. hook something up to getEntries first.

(In reply to comment #9)
> (In reply to comment #3)
> > (From update of attachment 151683 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=151683&action=review
> > 
> > Can you also add FIXMEs to the Performance Timeline functions indicating that they need to return User Timing too? You can fix them in a separate bug and patch.
> > 
> > >> 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.
> > 
> > I think the actual reason is that there's no way to observe it working. We haven't hooked it up to getEntries() yet. You can call the functions, but won't be able to see if they're doing the right things. I think we can hold off on tests until we have observable behavior.
> 
> I see. I'd rather we do this in the opposite order if possible, e.g. hook something up to getEntries first.

Thanks.
I have test case locally, however, to have a consistent style of w3c and James's test harness, I think it would be better to submit in another patch for https://bugs.webkit.org/show_bug.cgi?id=91072, and it need some more work.
so, could you please have a review this patch ?

[pdeng6]

Created attachment 153214 [details]
Patch

[pdeng6]

Test cases that use W3C test harness were added. 

@tonyg: Could you please review this? thanks.

[Ojan Vafai]

Comment on attachment 153214 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=153214&action=review

This will probably need a round or two more of review. Please apply my comments to the one test to all the tests as they apply to all of them.

> Source/WebCore/page/Performance.cpp:196
> +    return;

Don't need this.

> Source/WebCore/page/Performance.cpp:204
> +    return;

Don't need this.

> Source/WebCore/page/Performance.idl:59
> +        // See https://dvcs.w3.org/hg/webperf/raw-file/tip/specs/UserTiming/Overview.html

Typically, we put these links in the ChangeLog description instead as they get out of date.

> Source/WebCore/page/PerformanceMark.cpp:45
> +PassRefPtr<PerformanceMark> PerformanceMark::create(const String& name, double startTime)
> +{
> +    return adoptRef(new PerformanceMark(name, startTime));
> +}
> +
> +PerformanceMark::PerformanceMark(const String& name, double startTime)
> +    : PerformanceEntry(name, "mark", startTime, 0.0)
> +{
> +}
> +
> +PerformanceMark::~PerformanceMark()
> +{
> +}

Will this ever have more code than this? You can just inline all of this into the header.

> Source/WebCore/page/PerformanceMark.idl:28
> +    // See: https://dvcs.w3.org/hg/webperf/raw-file/tip/specs/UserTiming/Overview.html

Ditto.

> Source/WebCore/page/PerformanceMeasure.cpp:45
> +PassRefPtr<PerformanceMeasure> PerformanceMeasure::create(const String& name, double startTime, double duration)
> +{
> +    return adoptRef(new PerformanceMeasure(name, startTime, duration));
> +}
> +
> +PerformanceMeasure::PerformanceMeasure(const String& name, double startTime, double duration)
> +    : PerformanceEntry(name, "measure", startTime, duration)
> +{
> +}
> +
> +PerformanceMeasure::~PerformanceMeasure()
> +{
> +}

Ditto

> Source/WebCore/page/PerformanceUserTiming.cpp:46
> +      String key;
> +      NavigationTimingFunction navigationTimingFunction;

Indent is off.

> Source/WebCore/page/PerformanceUserTiming.cpp:111
> +    if (m_restrictiveKeyMap.contains(markName)) {
> +        ec = SYNTAX_ERR;
> +        return;
> +    }

This seems really strange to me. I realize the spec says to do this, but it seems totally unnecessary to me for this to use the same namespace as the PerformanceTimeline. It will make it hard to add things to PerformanceTimeline in the future because we'll need to make sure it doesn't conflict with any user mark names. Has this been discussed on the mailing list?

> Source/WebCore/page/PerformanceUserTiming.cpp:143
> +    if (startMark.isNull()) {
> +        endTime = m_performance->webkitNow();
> +    } else if (endMark.isNull()) {

No curly's around single-line if-statements.

> LayoutTests/http/tests/w3c/webperf/proposal/user-timing/test_user_timing_clearMarks.html:1
> +<!DOCTYPE html>

I don't think you want the BOM here (and in the other tests).

> LayoutTests/http/tests/w3c/webperf/proposal/user-timing/test_user_timing_clearMarks.html:4
> +        <meta charset="utf-8" />

Do w3c tests require this? If not, I'd drop it. The less cruft in tests the better.

> LayoutTests/http/tests/w3c/webperf/proposal/user-timing/test_user_timing_clearMarks.html:7
> +	<link rel="help" href="http://www.w3.org/TR/user-timing/#extensions-performance-interface"/>        

Indentation is off.

> LayoutTests/http/tests/w3c/webperf/proposal/user-timing/test_user_timing_clearMarks.html:15
> +<script id="metadata_cache">/*
> +{
> +}
> +*/</script>

What is this?

> LayoutTests/http/tests/w3c/webperf/proposal/user-timing/test_user_timing_clearMarks.html:18
> +         function onload_test()

Typically we use camel-case js code. Do w3c tests use lowercase?

> LayoutTests/http/tests/w3c/webperf/proposal/user-timing/test_user_timing_clearMarks.html:30
> +            var entryListChecker = new performanceEntryList_checker("mark");

camel-case is perferred.

> LayoutTests/http/tests/w3c/webperf/proposal/user-timing/test_user_timing_clearMarks.html:46
> +/**/

Is this accidental?

> LayoutTests/http/tests/w3c/webperf/proposal/user-timing/test_user_timing_clearMarks.html:48
> +            markNames.forEach(context.initialMarks);
> +            markNames.forEach(context.initialMarks);

Did you mean to repeat this? If so, it could use a comment explaining why.

> LayoutTests/http/tests/w3c/webperf/proposal/user-timing/test_user_timing_clearMarks.html:60
> +            markNames.forEach(context.initialMarks);
> +            markNames.forEach(context.initialMarks);

Ditto

> LayoutTests/http/tests/w3c/webperf/proposal/user-timing/test_user_timing_clearMarks.html:76
> +        <iframe id="frameContext" onload="setTimeout(onload_test(), 0);" src="/w3c/webperf/resources/blank_page_green.htm"></iframe>

Do you need the setTimeout?

> LayoutTests/http/tests/w3c/webperf/proposal/user-timing/test_user_timing_method_exist.html:30
> +        test_namespace();
> +        test_method_exists('webkitMark');
> +        test_method_exists('webkitClearMarks');
> +        test_method_exists('webkitMeasure');
> +        test_method_exists('webkitClearMeasures');
> +        test_method_exists('webkitGetEntries');
> +        test_method_exists('webkitGetEntriesByType');
> +        test_method_exists('webkitGetEntriesByName');

Since you do this here, there's no need to do it in all the other tests.

> LayoutTests/http/tests/w3c/webperf/resources/webperftestharnessextension.js:27
> +       '',
> +       '1',
> +       '2',
> +       '3',
> +       '4',
> +       '5',
> +       '6',
> +       'a',
> +       'bc',
> +       'def',
> +       'ghij',
> +       'M',
> +       false,
> +       true,
> +       undefined,
> +       null,

The expected results for these are quite large and it's hard to verify correctness. Do we really need to test all these cases? Can we just check, empty string, one number, one string? The rest of these don't actually test code paths that you just added. This doesn't need to be a generic test for all bindings code.

> LayoutTests/http/tests/w3c/webperf/resources/webperftestharnessextension.js:56
> +       [''],
> +       ['aa', 1],
> +       ['bb', true],
> +       ['cc', 'navigationStart'],
> +       ['dd', 'fetchStart'],
> +       ['ee', 'domainLookupEnd'],
> +       ['ff', 'domInteractive'],
> +       ['gg', 'loadEventStart'],
> +       ['   ', '', 1],
> +       ['aaa', '1', 2],
> +       ['bbb', 3, 'a'],
> +       ['ccc', 'a', 'false'],
> +       ['ddd', 'true', 'undefined'],
> +       ['eee', 'undefined', 'null'],
> +       ['fff', 'fetchStart', 'domInteractive'],
> +       ['ggg', 'domComplete', 'M'],
> +       ['hhh', '4', 'domLoading'],
> +       ['nav2now', 'navigationStart'],
> +       ['loadTime', 'navigationStart', 'loadEventEnd'],
> +       ['loadEventEnd2a', 'loadEventEnd', 'a'],
> +       ['nav2a', 'navigationStart', 'a'],
> +       ['domComplete2a', 'domComplete', 'a'],
> +       ['onloadStart2a', 'loadEventStart', 'a'],
> +       ['negativeValue', 3, 'navigationStart'],
> +       ['nav2M', 'navigationStart', 'M'],
> +       ['loadStart2M', 'loadEventStart', 'M'],

Ditto: Can we check fewer cases? Most of these don't actually test separate code paths that you just added

> LayoutTests/http/tests/w3c/webperf/resources/webperftestharnessextension.js:62
> +    wp_test(function() { assert_true(typeof performanceNamespace[method_name] === 'function', msg); }, msg, properties);

Where is wp_test defined? Also, we prefer not to use abbreviates where they're not necessary. webPerformanceTest would be preferred (assuming that's what wp stands for!).

> LayoutTests/http/tests/w3c/webperf/resources/webperftestharnessextension.js:83
> +        var msg = 'Entry \"' + entry.name + '\" shoule be one that we have set.';

Typo: shoule

> LayoutTests/http/tests/w3c/webperf/resources/webperftestharnessextension.js:93
> +        

extra line break

> LayoutTests/http/tests/w3c/webperf/resources/webperftestharnessextension.js:126
> +    
> +

Extra ine breeak

> LayoutTests/http/tests/w3c/webperf/resources/webperftestharnessextension.js:194
> +            "mark":mark,
> +            "measure":measure,
> +            "initialMarks":initialMarks,
> +            "initialMeasures":initialMeasures,
> +            "clearMarks":clearMarks,
> +            "clearMeasures":clearMeasures, 
> +            "getEntries":getEntries,
> +            "getEntriesByType":getEntriesByType,
> +            "getEntriesByName":getEntriesByName,
> +           };

This should just be a 4 space ident.

> LayoutTests/platform/chromium/TestExpectations:3714
> +BUGWK90963 SKIP : http/tests/w3c/webperf/proposal/user-timing/test_user_timing_clearMarks.html = PASS TEXT
> +BUGWK90963 SKIP : http/tests/w3c/webperf/proposal/user-timing/test_user_timing_clearMeasures.html = PASS TEXT
> +BUGWK90963 SKIP : http/tests/w3c/webperf/proposal/user-timing/test_user_timing_mark.html = PASS TEXT
> +BUGWK90963 SKIP : http/tests/w3c/webperf/proposal/user-timing/test_user_timing_mark_exception.html = PASS TEXT
> +BUGWK90963 SKIP : http/tests/w3c/webperf/proposal/user-timing/test_user_timing_measure.html = PASS TEXT
> +BUGWK90963 SKIP : http/tests/w3c/webperf/proposal/user-timing/test_user_timing_measure_exception.html = PASS TEXT
> +BUGWK90963 SKIP : http/tests/w3c/webperf/proposal/user-timing/test_user_timing_method_exist.html = PASS TEXT

You don't need to skip these all separately. You can skip the whole directory the same way you do with the Skipped files.

[pdeng6]

(In reply to comment #16)
> (From update of attachment 153214 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=153214&action=review
> 
> This will probably need a round or two more of review. Please apply my comments to the one test to all the tests as they apply to all of them.
> 
thanks for your comment:-)
> > Source/WebCore/page/Performance.cpp:196
> > +    return;
> 
> Don't need this.
> 
yep, done
> > Source/WebCore/page/Performance.cpp:204
> > +    return;
> 
> Don't need this.
> 
yep, done
> > Source/WebCore/page/Performance.idl:59
> > +        // See https://dvcs.w3.org/hg/webperf/raw-file/tip/specs/UserTiming/Overview.html
> 
> Typically, we put these links in the ChangeLog description instead as they get out of date.
> 
yep, move this to ChangeLog
> > Source/WebCore/page/PerformanceMark.cpp:45
> > +PassRefPtr<PerformanceMark> PerformanceMark::create(const String& name, double startTime)
> > +{
> > +    return adoptRef(new PerformanceMark(name, startTime));
> > +}
> > +
> > +PerformanceMark::PerformanceMark(const String& name, double startTime)
> > +    : PerformanceEntry(name, "mark", startTime, 0.0)
> > +{
> > +}
> > +
> > +PerformanceMark::~PerformanceMark()
> > +{
> > +}
> 
> Will this ever have more code than this? You can just inline all of this into the header.
> 
no, all inlined in .h, and removed this file
> > Source/WebCore/page/PerformanceMark.idl:28
> > +    // See: https://dvcs.w3.org/hg/webperf/raw-file/tip/specs/UserTiming/Overview.html
> 
> Ditto.
> 
done
> > Source/WebCore/page/PerformanceMeasure.cpp:45
> > +PassRefPtr<PerformanceMeasure> PerformanceMeasure::create(const String& name, double startTime, double duration)
> > +{
> > +    return adoptRef(new PerformanceMeasure(name, startTime, duration));
> > +}
> > +
> > +PerformanceMeasure::PerformanceMeasure(const String& name, double startTime, double duration)
> > +    : PerformanceEntry(name, "measure", startTime, duration)
> > +{
> > +}
> > +
> > +PerformanceMeasure::~PerformanceMeasure()
> > +{
> > +}
> 
> Ditto
> 
done
> > Source/WebCore/page/PerformanceUserTiming.cpp:46
> > +      String key;
> > +      NavigationTimingFunction navigationTimingFunction;
> 
> Indent is off.
> 
thanks, done,
> > Source/WebCore/page/PerformanceUserTiming.cpp:111
> > +    if (m_restrictiveKeyMap.contains(markName)) {
> > +        ec = SYNTAX_ERR;
> > +        return;
> > +    }
> 
> This seems really strange to me. I realize the spec says to do this, but it seems totally unnecessary to me for this to use the same namespace as the PerformanceTimeline. It will make it hard to add things to PerformanceTimeline in the future because we'll need to make sure it doesn't conflict with any user mark names. Has this been discussed on the mailing list?
> 
About mark names no conflict with Navigation Timing interface, I've not seen a thread about this in mailing list.
Navigation Timing spec is stable, and navigation timing2 would have natural different namespace as PerformanceNavigationTiming have different types from “mark”. My personal view is SYNTAX_ERR for conflicts is useful to remind user to avoid mark name confusion.
You can raise this issue to W3C.

> > Source/WebCore/page/PerformanceUserTiming.cpp:143
> > +    if (startMark.isNull()) {
> > +        endTime = m_performance->webkitNow();
> > +    } else if (endMark.isNull()) {
> 
> No curly's around single-line if-statements.
> 
done
> > LayoutTests/http/tests/w3c/webperf/proposal/user-timing/test_user_timing_clearMarks.html:1
> > +<!DOCTYPE html>
> 
> I don't think you want the BOM here (and in the other tests).
> 
yep, removed.
> > LayoutTests/http/tests/w3c/webperf/proposal/user-timing/test_user_timing_clearMarks.html:4
> > +        <meta charset="utf-8" />
> 
> Do w3c tests require this? If not, I'd drop it. The less cruft in tests the better.
> 
I didn't find a official test case guideline of W3C, however, every .html that was adopted as W3C navigation timing include this. so I suggest we keep this, how do you think of it?
> > LayoutTests/http/tests/w3c/webperf/proposal/user-timing/test_user_timing_clearMarks.html:7
> > +	<link rel="help" href="http://www.w3.org/TR/user-timing/#extensions-performance-interface"/>        
> 
> Indentation is off.
> 
done for all of them.
> > LayoutTests/http/tests/w3c/webperf/proposal/user-timing/test_user_timing_clearMarks.html:15
> > +<script id="metadata_cache">/*
> > +{
> > +}
> > +*/</script>
> 
> What is this?
> 
I think we don't need this currently.
> > LayoutTests/http/tests/w3c/webperf/proposal/user-timing/test_user_timing_clearMarks.html:18
> > +         function onload_test()
> 
> Typically we use camel-case js code. Do w3c tests use lowercase?
> 
yes, they always use lowercase.
> > LayoutTests/http/tests/w3c/webperf/proposal/user-timing/test_user_timing_clearMarks.html:30
> > +            var entryListChecker = new performanceEntryList_checker("mark");
> 
> camel-case is perferred.
> 

> > LayoutTests/http/tests/w3c/webperf/proposal/user-timing/test_user_timing_clearMarks.html:46
> > +/**/
> 
> Is this accidental?
> 
removed this.
> > LayoutTests/http/tests/w3c/webperf/proposal/user-timing/test_user_timing_clearMarks.html:48
> > +            markNames.forEach(context.initialMarks);
> > +            markNames.forEach(context.initialMarks);
> 
> Did you mean to repeat this? If so, it could use a comment explaining why.
> 
yep, added comment for this.
> > LayoutTests/http/tests/w3c/webperf/proposal/user-timing/test_user_timing_clearMarks.html:60
> > +            markNames.forEach(context.initialMarks);
> > +            markNames.forEach(context.initialMarks);
> 
> Ditto
> 
yep, added comment for this.
> > LayoutTests/http/tests/w3c/webperf/proposal/user-timing/test_user_timing_clearMarks.html:76
> > +        <iframe id="frameContext" onload="setTimeout(onload_test(), 0);" src="/w3c/webperf/resources/blank_page_green.htm"></iframe>
> 
> Do you need the setTimeout?
> 
no need, removed this.
> > LayoutTests/http/tests/w3c/webperf/proposal/user-timing/test_user_timing_method_exist.html:30
> > +        test_namespace();
> > +        test_method_exists('webkitMark');
> > +        test_method_exists('webkitClearMarks');
> > +        test_method_exists('webkitMeasure');
> > +        test_method_exists('webkitClearMeasures');
> > +        test_method_exists('webkitGetEntries');
> > +        test_method_exists('webkitGetEntriesByType');
> > +        test_method_exists('webkitGetEntriesByName');
> 
> Since you do this here, there's no need to do it in all the other tests.
> 
removed from other tests.

> > LayoutTests/http/tests/w3c/webperf/resources/webperftestharnessextension.js:27
> > +       '',
> > +       '1',
> > +       '2',
> > +       '3',
> > +       '4',
> > +       '5',
> > +       '6',
> > +       'a',
> > +       'bc',
> > +       'def',
> > +       'ghij',
> > +       'M',
> > +       false,
> > +       true,
> > +       undefined,
> > +       null,
> 
> The expected results for these are quite large and it's hard to verify correctness. Do we really need to test all these cases? Can we just check, empty string, one number, one string? The rest of these don't actually test code paths that you just added. This doesn't need to be a generic test for all bindings code.
> 
yep, done.
> > LayoutTests/http/tests/w3c/webperf/resources/webperftestharnessextension.js:56
> > +       [''],
> > +       ['aa', 1],
> > +       ['bb', true],
> > +       ['cc', 'navigationStart'],
> > +       ['dd', 'fetchStart'],
> > +       ['ee', 'domainLookupEnd'],
> > +       ['ff', 'domInteractive'],
> > +       ['gg', 'loadEventStart'],
> > +       ['   ', '', 1],
> > +       ['aaa', '1', 2],
> > +       ['bbb', 3, 'a'],
> > +       ['ccc', 'a', 'false'],
> > +       ['ddd', 'true', 'undefined'],
> > +       ['eee', 'undefined', 'null'],
> > +       ['fff', 'fetchStart', 'domInteractive'],
> > +       ['ggg', 'domComplete', 'M'],
> > +       ['hhh', '4', 'domLoading'],
> > +       ['nav2now', 'navigationStart'],
> > +       ['loadTime', 'navigationStart', 'loadEventEnd'],
> > +       ['loadEventEnd2a', 'loadEventEnd', 'a'],
> > +       ['nav2a', 'navigationStart', 'a'],
> > +       ['domComplete2a', 'domComplete', 'a'],
> > +       ['onloadStart2a', 'loadEventStart', 'a'],
> > +       ['negativeValue', 3, 'navigationStart'],
> > +       ['nav2M', 'navigationStart', 'M'],
> > +       ['loadStart2M', 'loadEventStart', 'M'],
> 
> Ditto: Can we check fewer cases? Most of these don't actually test separate code paths that you just added
> 
I split this to general measure() test cases, and timing order associate with navigation timing, results is slim now.
> > LayoutTests/http/tests/w3c/webperf/resources/webperftestharnessextension.js:62
> > +    wp_test(function() { assert_true(typeof performanceNamespace[method_name] === 'function', msg); }, msg, properties);
> 
> Where is wp_test defined? Also, we prefer not to use abbreviates where they're not necessary. webPerformanceTest would be preferred (assuming that's what wp stands for!).

I agree it's short for web performance test, however it comes from w3c webperftestharness.js
> 
> > LayoutTests/http/tests/w3c/webperf/resources/webperftestharnessextension.js:83
> > +        var msg = 'Entry \"' + entry.name + '\" shoule be one that we have set.';
> 
> Typo: shoule
> 
thanks, done
> > LayoutTests/http/tests/w3c/webperf/resources/webperftestharnessextension.js:93
> > +        
> 
> extra line break
> 
done
> > LayoutTests/http/tests/w3c/webperf/resources/webperftestharnessextension.js:126
> > +    
> > +
> 
> Extra ine breeak
> 
done
> > LayoutTests/http/tests/w3c/webperf/resources/webperftestharnessextension.js:194
> > +            "mark":mark,
> > +            "measure":measure,
> > +            "initialMarks":initialMarks,
> > +            "initialMeasures":initialMeasures,
> > +            "clearMarks":clearMarks,
> > +            "clearMeasures":clearMeasures, 
> > +            "getEntries":getEntries,
> > +            "getEntriesByType":getEntriesByType,
> > +            "getEntriesByName":getEntriesByName,
> > +           };
> 
> This should just be a 4 space ident.
> 
yep, done
> > LayoutTests/platform/chromium/TestExpectations:3714
> > +BUGWK90963 SKIP : http/tests/w3c/webperf/proposal/user-timing/test_user_timing_clearMarks.html = PASS TEXT
> > +BUGWK90963 SKIP : http/tests/w3c/webperf/proposal/user-timing/test_user_timing_clearMeasures.html = PASS TEXT
> > +BUGWK90963 SKIP : http/tests/w3c/webperf/proposal/user-timing/test_user_timing_mark.html = PASS TEXT
> > +BUGWK90963 SKIP : http/tests/w3c/webperf/proposal/user-timing/test_user_timing_mark_exception.html = PASS TEXT
> > +BUGWK90963 SKIP : http/tests/w3c/webperf/proposal/user-timing/test_user_timing_measure.html = PASS TEXT
> > +BUGWK90963 SKIP : http/tests/w3c/webperf/proposal/user-timing/test_user_timing_measure_exception.html = PASS TEXT
> > +BUGWK90963 SKIP : http/tests/w3c/webperf/proposal/user-timing/test_user_timing_method_exist.html = PASS TEXT
> 
> You don't need to skip these all separately. You can skip the whole directory the same way you do with the Skipped files.
thanks a lot, done this.

[pdeng6]

Created attachment 156318 [details]
Patch

[pdeng6]

Created attachment 156330 [details]
Patch

[pdeng6]

@Ojan, could you please review this new patch?
thanks :)

[Tony Gentilcore]

Comment on attachment 156330 [details]
Patch

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

[pdeng6]

Created attachment 168949 [details]
Patch

[pdeng6]

(In reply to comment #21)
> (From update of attachment 156330 [details])
> 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.
rebase done.
> 
> > 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.
yep, done.
> 
> > 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.
yep, done.
> 
> > Source/WebCore/page/PerformanceUserTiming.cpp:26
> > +
> 
> nit: there's an extra line break here.
done
> 
> > Source/WebCore/page/PerformanceUserTiming.cpp:37
> > +namespace WebCore {
> 
> Please add a line break above namespace
done
> 
> > Source/WebCore/page/PerformanceUserTiming.cpp:44
> > +    static const struct RestrictiveField {
> 
> This should be named RestrictedField.
thanks, done
> 
> > Source/WebCore/page/PerformanceUserTiming.cpp:91
> > +     }
> 
> Indentation is off
done
> 
> > 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.
> 
done.
> > 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.
> 
yep, done
> > 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/ ,/, /
done
> 
> > 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
done
> 
> > 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/ ,/, /
done
> 
> > 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.');
> 
done
> 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/ ,/, /
done
> 
> > LayoutTests/http/tests/w3c/webperf/resources/webperftestharnessextension.js:12
> > +       '',
> 
> indent only 4 spaces
thanks, done

[Tony Gentilcore]

Comment on attachment 168949 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=168949&action=review

Just a few more comments.

> Source/WebCore/page/Performance.idl:55
> +#if defined(ENABLE_USER_TIMING) && ENABLE_USER_TIMING

I recommend adding a comment with a link to the spec here.

> Source/WebCore/page/PerformanceEntryList.h:36
> +#include "PerformanceEntry.h"

I think the forward declaration is enough, right? If not and I'm missing something, then please remove the forward declaration in favor of this.

> LayoutTests/http/tests/w3c/webperf/resources/webperftestharnessextension.js:91
> +       arguments.length == 0 ? performanceContext.webkitMark() : performanceContext.webkitMark(arguments[0]);

In order to upstream these tests, we should use polyfill instead of hardcoding the webkit prefixes.

For example:

performance.mark = (function() {
  return performance.mark       ||
         performance.mozMark    ||
         performance.msMark     ||
         performance.oMark      ||
         performance.webkitMark;
})();

Up to you if you want to tackle that in this patch or not.

> LayoutTests/http/tests/w3c/webperf/resources/webperftestharnessextension.js:101
> +       if (arguments.length == 0)

Instead of all this, you can do:
performanceContext.webkitMeasure.apply(this, arguments);

Same for initialMeasures, clearMarks, clearMeasures.

> LayoutTests/http/tests/w3c/webperf/resources/webperftestharnessextension.js:147
> +        "mark":mark,

Looks like it would be better to make this a class.

function PerformanceContext()
{
}

PerformanceContext.prototype.mark = function()
{
...
}

...

> LayoutTests/http/tests/w3c/webperf/submission/user-timing/test_user_timing_clearMarks.html:66
> +        <iframe id="frameContext" onload="onload_test()" src="/w3c/webperf/resources/blank_page_green.htm"></iframe>

These tests all seem to create an iframe for no reasons. Please simplify them and just call onload_test from <body> and remove the iframe.

> LayoutTests/platform/qt-5.0-wk2/TestExpectations:620
> +# https://bugs.webkit.org/show_bug.cgi?id=90963

I'd remove the link to this bug here and in the qt expectations because bug 84893 is already referenced.

[pdeng6]

Created attachment 169156 [details]
Patch

[pdeng6]

> > Source/WebCore/page/Performance.idl:55
> > +#if defined(ENABLE_USER_TIMING) && ENABLE_USER_TIMING
> 
> I recommend adding a comment with a link to the spec here.
done,
> 
> > Source/WebCore/page/PerformanceEntryList.h:36
> > +#include "PerformanceEntry.h"
> 
> I think the forward declaration is enough, right? If not and I'm missing something, then please remove the forward declaration in favor of this.
> 
forward decl is enough here, done

> > LayoutTests/http/tests/w3c/webperf/resources/webperftestharnessextension.js:91
> > +       arguments.length == 0 ? performanceContext.webkitMark() : performanceContext.webkitMark(arguments[0]);
> 
> In order to upstream these tests, we should use polyfill instead of hardcoding the webkit prefixes.
> 
> For example:
> 
> performance.mark = (function() {
>   return performance.mark       ||
>          performance.mozMark    ||
>          performance.msMark     ||
>          performance.oMark      ||
>          performance.webkitMark;
> })();
> 
> Up to you if you want to tackle that in this patch or not.
> 
thanks, done, of course I would love to upstream that. :)
> > LayoutTests/http/tests/w3c/webperf/resources/webperftestharnessextension.js:101
> > +       if (arguments.length == 0)
> 
> Instead of all this, you can do:
> performanceContext.webkitMeasure.apply(this, arguments);
> 
> Same for initialMeasures, clearMarks, clearMeasures.
> 
> > LayoutTests/http/tests/w3c/webperf/resources/webperftestharnessextension.js:147
> > +        "mark":mark,
> 
> Looks like it would be better to make this a class.
> 
Good idea, thanks!
> function PerformanceContext()
> {
> }
> 
> PerformanceContext.prototype.mark = function()
> {
> ...
> }
> 
> ...
> 
> > LayoutTests/http/tests/w3c/webperf/submission/user-timing/test_user_timing_clearMarks.html:66
> > +        <iframe id="frameContext" onload="onload_test()" src="/w3c/webperf/resources/blank_page_green.htm"></iframe>
> 
> These tests all seem to create an iframe for no reasons. Please simplify them and just call onload_test from <body> and remove the iframe.
> 
all iframe removed.
> > LayoutTests/platform/qt-5.0-wk2/TestExpectations:620
> > +# https://bugs.webkit.org/show_bug.cgi?id=90963
> 
> I'd remove the link to this bug here and in the qt expectations because bug 84893 is already referenced.
yep, removed

[WebKit Review Bot]

Comment on attachment 169156 [details]
Patch

Rejecting attachment 169156 [details] from commit-queue.

[e-mail] does not have committer permissions according to http://trac.webkit.org/browser/trunk/Tools/Scripts/webkitpy/common/config/committers.py.

- If you do not have committer rights please read http://webkit.org/coding/contributing.html for instructions on how to use bugzilla flags.

- If you have committer rights please correct the error in Tools/Scripts/webkitpy/common/config/committers.py by adding yourself to the file (no review needed).  The commit-queue restarts itself every 2 hours.  After restart the commit-queue will correctly respect your committer rights.

[WebKit Review Bot]

Comment on attachment 169156 [details]
Patch

Rejecting attachment 169156 [details] from commit-queue.

Failed to run "['/mnt/git/webkit-commit-queue/Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '-..." exit_code: 2

Last 500 characters of output:
platform/mac/TestExpectations
Hunk #1 succeeded at 1311 (offset -25 lines).
patching file LayoutTests/platform/qt-5.0-wk2/TestExpectations
patching file LayoutTests/platform/qt/TestExpectations
Hunk #1 succeeded at 587 (offset 1 line).
patching file LayoutTests/platform/win/TestExpectations
Hunk #1 succeeded at 2342 (offset 1 line).

Failed to run "[u'/mnt/git/webkit-commit-queue/Tools/Scripts/svn-apply', u'--force', u'--reviewer', u'Tony Genti..." exit_code: 1 cwd: /mnt/git/webkit-commit-queue

Full output: http://queues.webkit.org/results/14384774

[pdeng6]

Created attachment 169319 [details]
Patch

[WebKit Review Bot]

Comment on attachment 169319 [details]
Patch

Clearing flags on attachment: 169319

Committed r131693: <http://trac.webkit.org/changeset/131693>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.