Added dumping function for testing resource load statistics
Created attachment 348679 [details] Patch
Created attachment 348683 [details] Patch
Created attachment 348690 [details] Patch
Comment on attachment 348690 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=348690&action=review > Source/WebCore/ChangeLog:3 > + Added dumping function for testing resource load statistics Maybe a better title would be: Add infrastructure to dump resource load statistics > Source/WebKit/UIProcess/API/C/WKWebsiteDataStoreRef.cpp:155 > + store->dumpResourceLoadStatistics([context, callback] (String dumpResourceLoadStatistics) { This is ref'ing the passed string and this function does not need to take ownership of the string. I suggest the callback take a "const String&" as the parameter type. Can we come up with a better name for the parameter? Maybe resourceLoadStatistics? > Source/WebKit/UIProcess/API/C/WKWebsiteDataStoreRef.cpp:156 > + callback(WKStringCreateWithUTF8CString(dumpResourceLoadStatistics.utf8().data()), context); Is there a better way to convert from WTF::String to WKStringRef. I am surprised that this problem has not already been solved. Does it make sense to to use toCopiedAPI()? > Source/WebKit/UIProcess/ResourceLoadStatisticsMemoryStore.cpp:678 > + Please remove the leading whitespace from this line. > Source/WebKit/UIProcess/ResourceLoadStatisticsMemoryStore.cpp:679 > + StringBuilder dumpResourceLoadStatisticsMapString; Can we come up with a better name for this variable? Maybe result? > Source/WebKit/UIProcess/WebResourceLoadStatisticsStore.cpp:500 > + Please remove the leading whitespace from this line. > Source/WebKit/UIProcess/WebResourceLoadStatisticsStore.cpp:502 > + String dumpResourceLoadStatistics = m_memoryStore ? m_memoryStore->dumpResourceLoadStatistics() : emptyString(); Can we come up with a better name for name of the local variable? Maybe result? or resourceLoadStatistics? > Tools/WebKitTestRunner/InjectedBundle/Bindings/TestRunner.idl:271 > + void setDumpResourceLoadStatistics(boolean shouldDump); I suggest that we take a similar approach as the other dumping function is this file and call this function dumpResourceLoadStatistics() and it should not take any arguments. We should also group this declaration with the declarations of the other dumping functions at the top of this file. > Tools/WebKitTestRunner/TestController.cpp:2808 > + WKStringRef dump; Can we come up with a more descriptive name for this?
(In reply to Daniel Bates from comment #4) > Comment on attachment 348690 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=348690&action=review > > > Source/WebCore/ChangeLog:3 > > + Added dumping function for testing resource load statistics > > Maybe a better title would be: Add infrastructure to dump resource load > statistics Agreed, changing accordingly. > > > Source/WebKit/UIProcess/API/C/WKWebsiteDataStoreRef.cpp:155 > > + store->dumpResourceLoadStatistics([context, callback] (String dumpResourceLoadStatistics) { > > This is ref'ing the passed string and this function does not need to take > ownership of the string. I suggest the callback take a "const String&" as > the parameter type. Can we come up with a better name for the parameter? > Maybe resourceLoadStatistics? Fixed. > > > Source/WebKit/UIProcess/API/C/WKWebsiteDataStoreRef.cpp:156 > > + callback(WKStringCreateWithUTF8CString(dumpResourceLoadStatistics.utf8().data()), context); > > Is there a better way to convert from WTF::String to WKStringRef. I am > surprised that this problem has not already been solved. Does it make sense > to to use toCopiedAPI()? Yes, toCopiedAPI() is a utility function we can use. > > > Source/WebKit/UIProcess/ResourceLoadStatisticsMemoryStore.cpp:678 > > + > > Please remove the leading whitespace from this line. Done. > > > Source/WebKit/UIProcess/ResourceLoadStatisticsMemoryStore.cpp:679 > > + StringBuilder dumpResourceLoadStatisticsMapString; > > Can we come up with a better name for this variable? Maybe result? Changed. > > > Source/WebKit/UIProcess/WebResourceLoadStatisticsStore.cpp:500 > > + > > Please remove the leading whitespace from this line. Done. > > > Source/WebKit/UIProcess/WebResourceLoadStatisticsStore.cpp:502 > > + String dumpResourceLoadStatistics = m_memoryStore ? m_memoryStore->dumpResourceLoadStatistics() : emptyString(); > > Can we come up with a better name for name of the local variable? Maybe > result? or resourceLoadStatistics? Changed to resourceLoadStatistics. > > > Tools/WebKitTestRunner/InjectedBundle/Bindings/TestRunner.idl:271 > > + void setDumpResourceLoadStatistics(boolean shouldDump); > > I suggest that we take a similar approach as the other dumping function is > this file and call this function dumpResourceLoadStatistics() and it should > not take any arguments. We should also group this declaration with the > declarations of the other dumping functions at the top of this file. > Done. > > Tools/WebKitTestRunner/TestController.cpp:2808 > > + WKStringRef dump; > > Can we come up with a more descriptive name for this? Changed to resourceLoadStatisticsRepresentation.
Created attachment 348859 [details] Patch
Comment on attachment 348859 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=348859&action=review > Source/WebKit/UIProcess/API/C/WKWebsiteDataStoreRef.cpp:151 > + callback(WKStringCreateWithUTF8CString(""), context); It seems odd to me that we're transferring ownership of the WKString to the callback here. I think we usually do not transfer ownership of such string to callback functions. > Source/WebKit/UIProcess/API/C/WKWebsiteDataStoreRef.cpp:156 > + callback(WebKit::toCopiedAPI(resourceLoadStatistics), context); Ditto. I would have used: toAPI(resourceLoadStatistics.impl()) > Source/WebKit/UIProcess/WebResourceLoadStatisticsStore.cpp:497 > +void WebResourceLoadStatisticsStore::dumpResourceLoadStatistics(CompletionHandler<void(String)>&& completionHandler) Why String and not const String& ? > Source/WebKit/UIProcess/WebResourceLoadStatisticsStore.cpp:503 > + postTaskReply([result, completionHandler = WTFMove(completionHandler)] () mutable { you are failing to isolate copy result before passing it to the other thread here. You want "result = result.isolatedCopy()". > Tools/WebKitTestRunner/TestInvocation.cpp:1363 > + m_savedResourceLoadStatistics = toWTFString(TestController::singleton().dumpResourceLoadStatistics()); It looks to me that the value returned by dumpResourceLoadStatistics() was leaked here.
Comment on attachment 348859 [details] Patch r- per comment #7.
(In reply to Chris Dumez from comment #7) > Comment on attachment 348859 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=348859&action=review Thanks for the tips/help! > > > Source/WebKit/UIProcess/API/C/WKWebsiteDataStoreRef.cpp:151 > > + callback(WKStringCreateWithUTF8CString(""), context); > > It seems odd to me that we're transferring ownership of the WKString to the > callback here. I think we usually do not transfer ownership of such string > to callback functions. > > > Source/WebKit/UIProcess/API/C/WKWebsiteDataStoreRef.cpp:156 > > + callback(WebKit::toCopiedAPI(resourceLoadStatistics), context); > > Ditto. I would have used: > toAPI(resourceLoadStatistics.impl()) Fixed to use toAPI instead of toCopiedAPI > > > Source/WebKit/UIProcess/WebResourceLoadStatisticsStore.cpp:497 > > +void WebResourceLoadStatisticsStore::dumpResourceLoadStatistics(CompletionHandler<void(String)>&& completionHandler) > > Why String and not const String& ? Changed to use const String& > > > Source/WebKit/UIProcess/WebResourceLoadStatisticsStore.cpp:503 > > + postTaskReply([result, completionHandler = WTFMove(completionHandler)] () mutable { > > you are failing to isolate copy result before passing it to the other thread > here. You want "result = result.isolatedCopy()". Thanks for pointing this out! > > > Tools/WebKitTestRunner/TestInvocation.cpp:1363 > > + m_savedResourceLoadStatistics = toWTFString(TestController::singleton().dumpResourceLoadStatistics()); > > It looks to me that the value returned by dumpResourceLoadStatistics() was > leaked here.
Created attachment 348864 [details] Patch
Created attachment 348871 [details] Patch
Comment on attachment 348864 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=348864&action=review > Tools/WebKitTestRunner/TestController.cpp:2808 > + WKStringRef resourceLoadStatisticsRepresentation; This should probably be a WKRetainPtr<WKStringRef> so that your string stays alive. > Tools/WebKitTestRunner/TestController.cpp:2879 > +WKStringRef TestController::dumpResourceLoadStatistics() Maybe this could this return a String instead of a WKStringRef ? > Tools/WebKitTestRunner/TestController.cpp:2885 > + return context.resourceLoadStatisticsRepresentation; return context.resourceLoadStatisticsRepresentation.get(); if you make the change I suggest above. > Tools/WebKitTestRunner/TestInvocation.cpp:1363 > + m_savedResourceLoadStatistics = toWTFString(TestController::singleton().dumpResourceLoadStatistics()); Is this really the right place to dump the statistics? Seems unrelated to resetting state?
(In reply to Chris Dumez from comment #12) > Comment on attachment 348864 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=348864&action=review > > > Tools/WebKitTestRunner/TestController.cpp:2808 > > + WKStringRef resourceLoadStatisticsRepresentation; > > This should probably be a WKRetainPtr<WKStringRef> so that your string stays > alive. Fixed! > > > Tools/WebKitTestRunner/TestController.cpp:2879 > > +WKStringRef TestController::dumpResourceLoadStatistics() > > Maybe this could this return a String instead of a WKStringRef ? Agreed that it seems cleaner this way. Modified here and call sites accordingly. > > > Tools/WebKitTestRunner/TestController.cpp:2885 > > + return context.resourceLoadStatisticsRepresentation; > > return context.resourceLoadStatisticsRepresentation.get(); > > if you make the change I suggest above. > > > Tools/WebKitTestRunner/TestInvocation.cpp:1363 > > + m_savedResourceLoadStatistics = toWTFString(TestController::singleton().dumpResourceLoadStatistics()); > > Is this really the right place to dump the statistics? Seems unrelated to > resetting state? The design of this dumping function only allows and intends for the resource load statistics map to be dumped once at the conclusion of a test. At that point in the test, however, if the test calls resetStatisticsToConsistentState, the resource load statistics map will be cleared, and so the dumped string will be empty. Thus, right now we can simply record the state of the resource load statistics map at the time of resetting the statistics and store it in the instance variable m_savedResourceLoadStatistics.
(In reply to Chris Dumez from comment #12) > Comment on attachment 348864 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=348864&action=review > > > Tools/WebKitTestRunner/TestController.cpp:2808 > > + WKStringRef resourceLoadStatisticsRepresentation; > > This should probably be a WKRetainPtr<WKStringRef> so that your string stays > alive. > > > Tools/WebKitTestRunner/TestController.cpp:2879 > > +WKStringRef TestController::dumpResourceLoadStatistics() > > Maybe this could this return a String instead of a WKStringRef ? Is there a reason we should return a String over a WKStringRef? It seems like in the existing code that the precedence is to use WKStringRefs in the TestController. > > > Tools/WebKitTestRunner/TestController.cpp:2885 > > + return context.resourceLoadStatisticsRepresentation; > > return context.resourceLoadStatisticsRepresentation.get(); > > if you make the change I suggest above. > > > Tools/WebKitTestRunner/TestInvocation.cpp:1363 > > + m_savedResourceLoadStatistics = toWTFString(TestController::singleton().dumpResourceLoadStatistics()); > > Is this really the right place to dump the statistics? Seems unrelated to > resetting state?
Created attachment 348877 [details] Patch
(In reply to Woodrow Wang from comment #14) > (In reply to Chris Dumez from comment #12) > > Comment on attachment 348864 [details] > > Patch > > > > View in context: > > https://bugs.webkit.org/attachment.cgi?id=348864&action=review > > > > > Tools/WebKitTestRunner/TestController.cpp:2808 > > > + WKStringRef resourceLoadStatisticsRepresentation; > > > > This should probably be a WKRetainPtr<WKStringRef> so that your string stays > > alive. > > > > > Tools/WebKitTestRunner/TestController.cpp:2879 > > > +WKStringRef TestController::dumpResourceLoadStatistics() > > > > Maybe this could this return a String instead of a WKStringRef ? > > Is there a reason we should return a String over a WKStringRef? It seems > like in the existing code that the precedence is to use WKStringRefs in the > TestController. I do not feel strongly but it seems your call site seems to want a String anyway. Also a String is much nicer to deal with than a WKStringRef because you do not have to worry about lifetime. > > > > > Tools/WebKitTestRunner/TestController.cpp:2885 > > > + return context.resourceLoadStatisticsRepresentation; > > > > return context.resourceLoadStatisticsRepresentation.get(); > > > > if you make the change I suggest above. > > > > > Tools/WebKitTestRunner/TestInvocation.cpp:1363 > > > + m_savedResourceLoadStatistics = toWTFString(TestController::singleton().dumpResourceLoadStatistics()); > > > > Is this really the right place to dump the statistics? Seems unrelated to > > resetting state?
Comment on attachment 348877 [details] Patch Attachment 348877 [details] did not pass win-ews (win): Output: https://webkit-queues.webkit.org/results/9096872 New failing tests: fast/workers/worker-exception-during-navigation.html
Created attachment 348895 [details] Archive of layout-test-results from ews203 for win-future The attached test failures were seen while running run-webkit-tests on the win-ews. Bot: ews203 Port: win-future Platform: CYGWIN_NT-6.1-2.9.0-0.318-5-3-x86_64-64bit
Comment on attachment 348877 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=348877&action=review LGTM with a couple of comments. > Tools/WebKitTestRunner/TestController.cpp:2879 > +WKStringRef TestController::dumpResourceLoadStatistics() nit: I think it would look nicer and be less error-prone in terms of lifetime management if this returned a String. Both your call sites need a String anyway. > Tools/WebKitTestRunner/TestInvocation.cpp:254 > + m_textOutput.append(m_savedResourceLoadStatistics.isEmpty() ? toWTFString(TestController::singleton().dumpResourceLoadStatistics()) : m_savedResourceLoadStatistics); AFAICT, the empty string is a valid value that can be returned by dumpResourceLoadStatistics(). Therefore, I think the check for the ternary should be m_savedResourceLoadStatistics.isNull(). > Tools/WebKitTestRunner/TestInvocation.cpp:1361 > if (WKStringIsEqualToUTF8CString(messageName, "StatisticsResetToConsistentState")) { FYI, longer term, I think we should probably drop this TestRunner method. We should not rely on the test to go back to a consistent state, TestController should reset to a consistent state between tests by itself.
(In reply to Chris Dumez from comment #19) > Comment on attachment 348877 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=348877&action=review > > LGTM with a couple of comments. > > > Tools/WebKitTestRunner/TestController.cpp:2879 > > +WKStringRef TestController::dumpResourceLoadStatistics() > > nit: I think it would look nicer and be less error-prone in terms of > lifetime management if this returned a String. Both your call sites need a > String anyway. Thanks for the help with avoiding memory leaks with the WKStringRef! > > > Tools/WebKitTestRunner/TestInvocation.cpp:254 > > + m_textOutput.append(m_savedResourceLoadStatistics.isEmpty() ? toWTFString(TestController::singleton().dumpResourceLoadStatistics()) : m_savedResourceLoadStatistics); > > AFAICT, the empty string is a valid value that can be returned by > dumpResourceLoadStatistics(). Therefore, I think the check for the ternary > should be m_savedResourceLoadStatistics.isNull(). This makes sense, so I'll change it. > > > Tools/WebKitTestRunner/TestInvocation.cpp:1361 > > if (WKStringIsEqualToUTF8CString(messageName, "StatisticsResetToConsistentState")) { > > FYI, longer term, I think we should probably drop this TestRunner method. We > should not rely on the test to go back to a consistent state, TestController > should reset to a consistent state between tests by itself. Agreed, this call to dumpResourceLoadStatistics() is replicated in the dumpResults() function, as the latter call site is much more logical. As an artifact of the resource load statistics' state being cleared by this function, we have this slightly more complex logic to save that state for dumping at the end of a test. Longer term, we can let the TestController handle the resetting of state by itself, making this code slightly simpler.
Created attachment 348930 [details] Patch
Comment on attachment 348930 [details] Patch LGTM
(In reply to Chris Dumez from comment #22) > Comment on attachment 348930 [details] > Patch > > LGTM r=me
Comment on attachment 348930 [details] Patch Attachment 348930 [details] did not pass mac-wk2-ews (mac-wk2): Output: https://webkit-queues.webkit.org/results/9103565 New failing tests: accessibility/smart-invert-reference.html
Created attachment 348942 [details] Archive of layout-test-results from ews106 for mac-sierra-wk2 The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews. Bot: ews106 Port: mac-sierra-wk2 Platform: Mac OS X 10.12.6
Created attachment 348949 [details] Patch
Comment on attachment 348949 [details] Patch Clearing flags on attachment: 348949 Committed r235690: <https://trac.webkit.org/changeset/235690>
All reviewed patches have been landed. Closing bug.
<rdar://problem/44154196>