as opposed to waiting for 500ms with the initial paint.
waiting 500ms that is.
Created attachment 353403 [details] Patch
Attachment 353403 [details] did not pass style-queue: ERROR: Source/WebCore/page/ChromeClient.h:336: Inline functions should not be in classes annotated with WEBCORE_EXPORT. Remove the macro from the class and apply it to each appropriate method, or move the inline function definition out-of-line. [build/webcore_export] [4] ERROR: Source/WebCore/page/FrameView.cpp:96: Alphabetical sorting problem. [build/include_order] [4] Total errors found: 2 in 17 files If any of these errors are false positives, please file a bug against check-webkit-style.
Comment on attachment 353403 [details] Patch Attachment 353403 [details] did not pass mac-wk2-ews (mac-wk2): Output: https://webkit-queues.webkit.org/results/9783488 New failing tests: tiled-drawing/simple-document-with-margin-tiles.html compositing/tiling/non-visible-window-tile-coverage.html
Created attachment 353408 [details] Archive of layout-test-results from ews107 for mac-sierra-wk2 The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews. Bot: ews107 Port: mac-sierra-wk2 Platform: Mac OS X 10.12.6
Comment on attachment 353403 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=353403&action=review > Source/WebCore/ChangeLog:12 > + 1. Improve visuallyNoneEmpty milestone confidence > + 2. Issue initial paint soon after the milestone fires > + 3. Suspend optional style recalcs and layouts while painting is being throttled. Will you break this into 3 patches? > Source/WebCore/dom/Document.cpp:1810 > + const Seconds styleRecalcDelay = 50_ms; Would be nice to see all the throttling-related code in one place some day. > Source/WebCore/page/FrameView.cpp:4415 > + auto hasPendingScriptExecution = frame().document()->scriptRunner() && frame().document()->scriptRunner()->hasPendingScripts(); > + if (auto* documentLoader = frame().loader().documentLoader()) { > + auto& resourceLoader = documentLoader->cachedResourceLoader(); > + if (!resourceLoader.requestCount()) > + return hasPendingScriptExecution; > + > + auto& resources = resourceLoader.allCachedResources(); > + for (auto& resource : resources) { > + if (resource.value->isLoaded()) > + continue; > + if (resource.value->type() == CachedResource::Type::CSSStyleSheet || resource.value->type() == CachedResource::Type::Script || resource.value->type() == CachedResource::Type::FontResource) > + return false; > + } > + } Maybe pull this into a separate function. > Source/WebCore/page/FrameView.h:396 > + void incrementVisuallyNonEmptyCharacterCount(const String); const String& > Source/WebCore/page/FrameView.h:947 > + auto whitespaceCheck = [](UChar character) { > + return character == ' ' || character == '\t' || character == '\n'; > + }; Can you not share some existing code for this? What about ? > Source/WebKit/WebProcess/WebPage/RemoteLayerTree/RemoteLayerTreeDrawingArea.h:117 > + void initialDeferredPaint(); Hard to figure out what this means. Does it mean do the paint, or something else?
<rdar://problem/45736178>
Created attachment 353624 [details] Patch
Attachment 353624 [details] did not pass style-queue: ERROR: Source/WebCore/page/ChromeClient.h:336: Inline functions should not be in classes annotated with WEBCORE_EXPORT. Remove the macro from the class and apply it to each appropriate method, or move the inline function definition out-of-line. [build/webcore_export] [4] Total errors found: 1 in 17 files If any of these errors are false positives, please file a bug against check-webkit-style.
(In reply to Simon Fraser (smfr) from comment #6) > Comment on attachment 353403 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=353403&action=review > > > Source/WebCore/ChangeLog:12 > > + 1. Improve visuallyNoneEmpty milestone confidence > > + 2. Issue initial paint soon after the milestone fires > > + 3. Suspend optional style recalcs and layouts while painting is being throttled. > > Will you break this into 3 patches? Normally I would, but since the additional paint is certainly a perf regression, I'd rather have it together with the potential progression of suspending optional style recalcs/layouts. > > > Source/WebCore/dom/Document.cpp:1810 > > + const Seconds styleRecalcDelay = 50_ms; > > Would be nice to see all the throttling-related code in one place some day. > > > Source/WebCore/page/FrameView.cpp:4415 > > + auto hasPendingScriptExecution = frame().document()->scriptRunner() && frame().document()->scriptRunner()->hasPendingScripts(); > > + if (auto* documentLoader = frame().loader().documentLoader()) { > > + auto& resourceLoader = documentLoader->cachedResourceLoader(); > > + if (!resourceLoader.requestCount()) > > + return hasPendingScriptExecution; > > + > > + auto& resources = resourceLoader.allCachedResources(); > > + for (auto& resource : resources) { > > + if (resource.value->isLoaded()) > > + continue; > > + if (resource.value->type() == CachedResource::Type::CSSStyleSheet || resource.value->type() == CachedResource::Type::Script || resource.value->type() == CachedResource::Type::FontResource) > > + return false; > > + } > > + } > > Maybe pull this into a separate function. Done. > > > Source/WebCore/page/FrameView.h:396 > > + void incrementVisuallyNonEmptyCharacterCount(const String); > > const String& :( > > > Source/WebCore/page/FrameView.h:947 > > + auto whitespaceCheck = [](UChar character) { > > + return character == ' ' || character == '\t' || character == '\n'; > > + }; > > Can you not share some existing code for this? What about ? I actually did find something similar in parsing code. > > > Source/WebKit/WebProcess/WebPage/RemoteLayerTree/RemoteLayerTreeDrawingArea.h:117 > > + void initialDeferredPaint(); > > Hard to figure out what this means. Does it mean do the paint, or something > else? Good point. Renamed.
Comment on attachment 353624 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=353624&action=review r=me. Interesting to see how the perf bots like this. > Source/WebCore/dom/Document.cpp:1810 > + const auto styleRecalcDelay = 50_ms; I suppose there is some deep science here? > Source/WebCore/dom/Document.cpp:1811 > + m_styleRecalcTimer.startOneShot(throttleStyleRecalc ? styleRecalcDelay : 0_s); Maybe this change could be done separately? > Source/WebCore/rendering/RenderText.cpp:203 > + // FIXME: Find out how to increment the visually non empty character count when the font becomes available. Maybe this stuff needs to move to RenderText::styleDidChange
(In reply to Antti Koivisto from comment #11) > Comment on attachment 353624 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=353624&action=review > > r=me. Interesting to see how the perf bots like this. > > > Source/WebCore/dom/Document.cpp:1810 > > + const auto styleRecalcDelay = 50_ms; > > I suppose there is some deep science here? 49ms seemed too short, while 51ms felt way too long. > > > Source/WebCore/dom/Document.cpp:1811 > > + m_styleRecalcTimer.startOneShot(throttleStyleRecalc ? styleRecalcDelay : 0_s); > > Maybe this change could be done separately? perf bots would see this patch as a net regression without that change. > > > Source/WebCore/rendering/RenderText.cpp:203 > > + // FIXME: Find out how to increment the visually non empty character count when the font becomes available. > > Maybe this stuff needs to move to RenderText::styleDidChange I was thinking about that too. Will probably do it in a a followup patch.
Comment on attachment 353624 [details] Patch Attachment 353624 [details] did not pass mac-wk2-ews (mac-wk2): Output: https://webkit-queues.webkit.org/results/9817559 New failing tests: tiled-drawing/simple-document-with-margin-tiles.html compositing/tiling/non-visible-window-tile-coverage.html
Created attachment 353636 [details] Archive of layout-test-results from ews104 for mac-sierra-wk2 The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews. Bot: ews104 Port: mac-sierra-wk2 Platform: Mac OS X 10.12.6
Also, what's the story on macOS? Do we do similar throttling?
(In reply to Simon Fraser (smfr) from comment #15) > Also, what's the story on macOS? Do we do similar throttling? Geoff and I talked about it and agreed that it would be part of a followup patch.
Committed r237785: <https://trac.webkit.org/changeset/237785>
(In reply to Build Bot from comment #13) > Comment on attachment 353624 [details] > Patch > > Attachment 353624 [details] did not pass mac-wk2-ews (mac-wk2): > Output: https://webkit-queues.webkit.org/results/9817559 > > New failing tests: > tiled-drawing/simple-document-with-margin-tiles.html > compositing/tiling/non-visible-window-tile-coverage.html These tests are failing on the bots after <https://trac.webkit.org/changeset/237785>: https://build.webkit.org/results/Apple%20Sierra%20Release%20WK2%20(Tests)/r237785%20(12559)/results.html
(In reply to Ryan Haddad from comment #18) > (In reply to Build Bot from comment #13) > > Comment on attachment 353624 [details] > > Patch > > > > Attachment 353624 [details] did not pass mac-wk2-ews (mac-wk2): > > Output: https://webkit-queues.webkit.org/results/9817559 > > > > New failing tests: > > tiled-drawing/simple-document-with-margin-tiles.html > > compositing/tiling/non-visible-window-tile-coverage.html > > These tests are failing on the bots after > <https://trac.webkit.org/changeset/237785>: > https://build.webkit.org/results/Apple%20Sierra%20Release%20WK2%20(Tests)/ > r237785%20(12559)/results.html It looks to me a (In reply to Ryan Haddad from comment #18) > (In reply to Build Bot from comment #13) > > Comment on attachment 353624 [details] > > Patch > > > > Attachment 353624 [details] did not pass mac-wk2-ews (mac-wk2): > > Output: https://webkit-queues.webkit.org/results/9817559 > > > > New failing tests: > > tiled-drawing/simple-document-with-margin-tiles.html > > compositing/tiling/non-visible-window-tile-coverage.html > > These tests are failing on the bots after > <https://trac.webkit.org/changeset/237785>: > https://build.webkit.org/results/Apple%20Sierra%20Release%20WK2%20(Tests)/ > r237785%20(12559)/results.html I assume those tests need rebaselining but not 100% sure what values represent in this context. Let me check with Tim/Simon first.
Other issues after this change: These two API tests are timing out on macOS bots TestWebKitAPI.WebKit.LayoutMilestonesWithAllContentInFrame TestWebKitAPI.WebKit.FirstVisuallyNonEmptyLayoutAfterPageCacheRestore https://build.webkit.org/builders/Apple%20Sierra%20Release%20WK2%20%28Tests%29/builds/12559 Layout tests are exiting early and 5 API tests are failing the following assertion on iOS Simulator Debug bots: ASSERTION FAILED: !m_layerFlushTimer.isActive() /Volumes/Data/slave/ios-simulator-12-debug/build/Source/WebKit/WebProcess/WebPage/RemoteLayerTree/RemoteLayerTreeDrawingArea.mm(290) : virtual void WebKit::RemoteLayerTreeDrawingArea::scheduleInitialDeferredPaint() https://build.webkit.org/builders/Apple%20iOS%2012%20Simulator%20Debug%20WK2%20(Tests)/builds/637
Reverted r237785 for reason: Introduced layout test and API test failures on macOS and iOS. Committed r237813: <https://trac.webkit.org/changeset/237813>
(In reply to zalan from comment #19) > (In reply to Ryan Haddad from comment #18) > > > > These tests are failing on the bots after > > <https://trac.webkit.org/changeset/237785>: > > https://build.webkit.org/results/Apple%20Sierra%20Release%20WK2%20(Tests)/ > > r237785%20(12559)/results.html > I assume those tests need rebaselining but not 100% sure what values > represent in this context. Let me check with Tim/Simon first. Those seem like real regressions.
(In reply to Ryan Haddad from comment #21) > Reverted r237785 for reason: > > Introduced layout test and API test failures on macOS and iOS. > > Committed r237813: <https://trac.webkit.org/changeset/237813> Both the API and the iOS failures are addressed now and I can't repro the mac-wk2 only failures locally. :(
Created attachment 353992 [details] Patch
Attachment 353992 [details] did not pass style-queue: ERROR: Source/WebCore/page/ChromeClient.h:336: Inline functions should not be in classes annotated with WEBCORE_EXPORT. Remove the macro from the class and apply it to each appropriate method, or move the inline function definition out-of-line. [build/webcore_export] [4] Total errors found: 1 in 17 files If any of these errors are false positives, please file a bug against check-webkit-style.
Created attachment 354380 [details] Patch
Attachment 354380 [details] did not pass style-queue: ERROR: Source/WebCore/page/ChromeClient.h:336: Inline functions should not be in classes annotated with WEBCORE_EXPORT. Remove the macro from the class and apply it to each appropriate method, or move the inline function definition out-of-line. [build/webcore_export] [4] Total errors found: 1 in 17 files If any of these errors are false positives, please file a bug against check-webkit-style.
Comment on attachment 354380 [details] Patch Attachment 354380 [details] did not pass mac-ews (mac): Output: https://webkit-queues.webkit.org/results/9928487 New failing tests: http/wpt/css/css-animations/start-animation-001.html
Created attachment 354386 [details] Archive of layout-test-results from ews103 for mac-sierra The attached test failures were seen while running run-webkit-tests on the mac-ews. Bot: ews103 Port: mac-sierra Platform: Mac OS X 10.12.6
Created attachment 354394 [details] Patch
Attachment 354394 [details] did not pass style-queue: ERROR: Source/WebCore/page/ChromeClient.h:336: Inline functions should not be in classes annotated with WEBCORE_EXPORT. Remove the macro from the class and apply it to each appropriate method, or move the inline function definition out-of-line. [build/webcore_export] [4] Total errors found: 1 in 17 files If any of these errors are false positives, please file a bug against check-webkit-style.
Comment on attachment 354394 [details] Patch Attachment 354394 [details] did not pass ios-sim-ews (ios-simulator-wk2): Output: https://webkit-queues.webkit.org/results/9932804 New failing tests: accessibility/ios-simulator/form-control-validation-message.html
Created attachment 354430 [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.13.6
Created attachment 354441 [details] Patch
Attachment 354441 [details] did not pass style-queue: ERROR: Source/WebCore/page/ChromeClient.h:336: Inline functions should not be in classes annotated with WEBCORE_EXPORT. Remove the macro from the class and apply it to each appropriate method, or move the inline function definition out-of-line. [build/webcore_export] [4] Total errors found: 1 in 17 files If any of these errors are false positives, please file a bug against check-webkit-style.
Comment on attachment 354441 [details] Patch Clearing flags on attachment: 354441 Committed r238064: <https://trac.webkit.org/changeset/238064>
All reviewed patches have been landed. Closing bug.