Implement mark(), measure(), clearMarks() and clearMeasures() interface of User Timing.
Created attachment 151683 [details] Patch
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.
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.
Created attachment 151902 [details] Patch
(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.
Created attachment 151906 [details] Patch
(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.
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.
(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.
Created attachment 152125 [details] Patch
(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.
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.
(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 ?
Created attachment 153214 [details] Patch
Test cases that use W3C test harness were added. @tonyg: Could you please review this? thanks.
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.
(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.
Created attachment 156318 [details] Patch
Created attachment 156330 [details] Patch
@Ojan, could you please review this new patch? thanks :)
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
Created attachment 168949 [details] Patch
(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
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.
Created attachment 169156 [details] Patch
> > 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
Comment on attachment 169156 [details] Patch Rejecting attachment 169156 [details] from commit-queue. pan.deng@intel.com 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.
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
Created attachment 169319 [details] Patch
Comment on attachment 169319 [details] Patch Clearing flags on attachment: 169319 Committed r131693: <http://trac.webkit.org/changeset/131693>
All reviewed patches have been landed. Closing bug.