On WinCairo layout tests related to inspector doesn't work at all. This is because lack of CPUProfilerObserver and MemoryObserver which are used in Test.js. (Source/WebInspectorUI/UserInterface/Test/Test.js) In This ticket we aim to fix it by enabling ENABLE_RESOURCE_USAGE.
Created attachment 371142 [details] Patch
Comment on attachment 371142 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=371142&action=review > Source/WebCore/PlatformWinCairo.cmake:16 > + page/win/ResourceUsageThreadWinCairo.cpp Rename *Win.cpp. > Source/WebCore/page/win/ResourceUsageThreadWinCairo.cpp:43 > +uint64_t fileTimeToUint64(FILETIME ft) Mark these file-local functions "static". > Tools/WebKitTestRunner/InjectedBundle/win/TestRunnerWin.cpp:-40 > - return JSStringCreateWithUTF8CString(""); What about using UrlCreateFromPath? https://docs.microsoft.com/en-us/windows/desktop/api/shlwapi/nf-shlwapi-urlcreatefrompatha > Tools/WebKitTestRunner/InjectedBundle/win/TestRunnerWin.cpp:44 > + _wsplitpath(exePath, drive, dir, nullptr, nullptr); Why do you need to split first? > Tools/WebKitTestRunner/InjectedBundle/win/TestRunnerWin.cpp:52 > + char stubPathUTF8[MAX_PATH]; MAX_PATH shouldn't be used for UTF-8 buffer length. How about using JSStringCreateWithCharacters? Then, you don't need to convert to UTF-8. > LayoutTests/platform/wincairo/TestExpectations:1566 > inspector [ Skip ] Can you unskip all inspector tests? How much failure will occur? If you can't unskip inspector tests, I think you should change the summary of this bug.
Comment on attachment 371142 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=371142&action=review > Source/WebCore/page/win/ResourceUsageOverlayWinCairo.cpp:30 > +#if ENABLE(RESOURCE_USAGE) && OS(WINDOWS) You shouldn't need any of these OS checks. > Source/WebCore/page/win/ResourceUsageThreadWinCairo.cpp:31 > +#if ENABLE(RESOURCE_USAGE) && OS(WINDOWS) Ditto
Created attachment 371236 [details] Patch
Created attachment 371237 [details] Patch
(In reply to Fujii Hironori from comment #2) > Comment on attachment 371142 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=371142&action=review > > > Source/WebCore/PlatformWinCairo.cmake:16 > > + page/win/ResourceUsageThreadWinCairo.cpp > > Rename *Win.cpp. Renamed. > > Source/WebCore/page/win/ResourceUsageThreadWinCairo.cpp:43 > > +uint64_t fileTimeToUint64(FILETIME ft) > > Mark these file-local functions "static". Marked. > > Tools/WebKitTestRunner/InjectedBundle/win/TestRunnerWin.cpp:-40 > > - return JSStringCreateWithUTF8CString(""); > > What about using UrlCreateFromPath? > https://docs.microsoft.com/en-us/windows/desktop/api/shlwapi/nf-shlwapi- > urlcreatefrompatha Changed to use UrlCreateFromPathW. > > Tools/WebKitTestRunner/InjectedBundle/win/TestRunnerWin.cpp:44 > > + _wsplitpath(exePath, drive, dir, nullptr, nullptr); > > Why do you need to split first? We need to determine resource directory path. This part makes the path by splitting absolute path of WebKitTestRunner.exe. > > Tools/WebKitTestRunner/InjectedBundle/win/TestRunnerWin.cpp:52 > > + char stubPathUTF8[MAX_PATH]; > > MAX_PATH shouldn't be used for UTF-8 buffer length. How about using > JSStringCreateWithCharacters? Then, you don't need to convert to UTF-8. Changed. > > LayoutTests/platform/wincairo/TestExpectations:1566 > > inspector [ Skip ] > > Can you unskip all inspector tests? How much failure will occur? If you > can't unskip inspector tests, I think you should change the summary of this > bug. Changed.
(In reply to Don Olmstead from comment #3) > Comment on attachment 371142 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=371142&action=review > > > Source/WebCore/page/win/ResourceUsageOverlayWinCairo.cpp:30 > > +#if ENABLE(RESOURCE_USAGE) && OS(WINDOWS) > > You shouldn't need any of these OS checks. > > > Source/WebCore/page/win/ResourceUsageThreadWinCairo.cpp:31 > > +#if ENABLE(RESOURCE_USAGE) && OS(WINDOWS) > > Ditto Fixed.
Comment on attachment 371237 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=371237&action=review Couple nits but LGTM overall. Fujii could you take another pass? > Source/WebCore/page/win/ResourceUsageThreadWin.cpp:116 > + // FIXME: Exclude the ResourceUsage thread. > + // FIXME: Exclude the SamplingProfiler thread. > + // FIXME: Classify usage per thread. > + data.cpuExcludingDebuggerThreads = data.cpu; Could you open up a bug for this work and then link to it? > Source/cmake/OptionsWinCairo.cmake:35 > +set(ENABLE_RESOURCE_USAGE ON) This is a WebKit option so this should be moved to OptionsWin.cmake in the `if (${WTF_PLATFORM_WIN_CAIRO})` block in between WEBKIT_OPTION_BEGIN() and WEBKIT_OPTION_END() ``` WEBKIT_OPTION_DEFAULT_PORT_VALUE(ENABLE_RESOURCE_USAGE PRIVATE OFF) ```
(In reply to Don Olmstead from comment #8) > Comment on attachment 371237 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=371237&action=review > > Source/cmake/OptionsWinCairo.cmake:35 > > +set(ENABLE_RESOURCE_USAGE ON) > > This is a WebKit option so this should be moved to OptionsWin.cmake in the > `if (${WTF_PLATFORM_WIN_CAIRO})` block in between WEBKIT_OPTION_BEGIN() and > WEBKIT_OPTION_END() > > ``` > WEBKIT_OPTION_DEFAULT_PORT_VALUE(ENABLE_RESOURCE_USAGE PRIVATE OFF) > ``` Sorry ON not OFF. Copy paste error...
(In reply to Don Olmstead from comment #9) > (In reply to Don Olmstead from comment #8) > > Comment on attachment 371237 [details] > > Patch > > > > View in context: > > https://bugs.webkit.org/attachment.cgi?id=371237&action=review > > > Source/cmake/OptionsWinCairo.cmake:35 > > > +set(ENABLE_RESOURCE_USAGE ON) > > > > This is a WebKit option so this should be moved to OptionsWin.cmake in the > > `if (${WTF_PLATFORM_WIN_CAIRO})` block in between WEBKIT_OPTION_BEGIN() and > > WEBKIT_OPTION_END() > > > > ``` > > WEBKIT_OPTION_DEFAULT_PORT_VALUE(ENABLE_RESOURCE_USAGE PRIVATE OFF) > > ``` > > Sorry ON not OFF. Copy paste error... Actually can you add it to the experimental features list till we have the full implementation. WEBKIT_OPTION_DEFAULT_PORT_VALUE(ENABLE_RESOURCE_USAGE PRIVATE ${ENABLE_EXPERIMENTAL_FEATURES})
Comment on attachment 371237 [details] Patch r=me Fix the CMake part and this is good to go
Created attachment 371252 [details] Patch
(In reply to Don Olmstead from comment #10) > (In reply to Don Olmstead from comment #9) > > (In reply to Don Olmstead from comment #8) > > > Comment on attachment 371237 [details] > > > Patch > > > > > > View in context: > > > https://bugs.webkit.org/attachment.cgi?id=371237&action=review > > > > Source/cmake/OptionsWinCairo.cmake:35 > > > > +set(ENABLE_RESOURCE_USAGE ON) > > > > > > This is a WebKit option so this should be moved to OptionsWin.cmake in the > > > `if (${WTF_PLATFORM_WIN_CAIRO})` block in between WEBKIT_OPTION_BEGIN() and > > > WEBKIT_OPTION_END() > > > > > > ``` > > > WEBKIT_OPTION_DEFAULT_PORT_VALUE(ENABLE_RESOURCE_USAGE PRIVATE OFF) > > > ``` > > > > Sorry ON not OFF. Copy paste error... > > Actually can you add it to the experimental features list till we have the > full implementation. > > WEBKIT_OPTION_DEFAULT_PORT_VALUE(ENABLE_RESOURCE_USAGE PRIVATE > ${ENABLE_EXPERIMENTAL_FEATURES}) FIXED.
(In reply to Don Olmstead from comment #8) > Comment on attachment 371237 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=371237&action=review > > Couple nits but LGTM overall. Fujii could you take another pass? > > > Source/WebCore/page/win/ResourceUsageThreadWin.cpp:116 > > + // FIXME: Exclude the ResourceUsage thread. > > + // FIXME: Exclude the SamplingProfiler thread. > > + // FIXME: Classify usage per thread. > > + data.cpuExcludingDebuggerThreads = data.cpu; > > Could you open up a bug for this work and then link to it? > Opened https://bugs.webkit.org/show_bug.cgi?id=198519
Comment on attachment 371252 [details] Patch Attachment 371252 [details] did not pass ios-sim-ews (ios-simulator-wk2): Output: https://webkit-queues.webkit.org/results/12370788 New failing tests: http/wpt/service-workers/service-worker-networkprocess-crash.html
Created attachment 371257 [details] Archive of layout-test-results from ews122 for ios-simulator-wk2 The attached test failures were seen while running run-webkit-tests on the ios-sim-ews. Bot: ews122 Port: ios-simulator-wk2 Platform: Mac OS X 10.14.5
Comment on attachment 371252 [details] Patch Clearing flags on attachment: 371252 Committed r246076: <https://trac.webkit.org/changeset/246076>
All reviewed patches have been landed. Closing bug.
<rdar://problem/51409011>