Summary: | Drawingarea should only capture painting related milestones | ||||||||
---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | zalan <zalan> | ||||||
Component: | Layout and Rendering | Assignee: | zalan <zalan> | ||||||
Status: | RESOLVED FIXED | ||||||||
Severity: | Normal | CC: | aakash_jain, ap, bfulgham, commit-queue, dbates, ryanhaddad, simon.fraser, thorton, webkit-bot-watchers-bugzilla, zalan | ||||||
Priority: | P2 | Keywords: | InRadar | ||||||
Version: | WebKit Nightly Build | ||||||||
Hardware: | Unspecified | ||||||||
OS: | Unspecified | ||||||||
See Also: | https://bugs.webkit.org/show_bug.cgi?id=196948 | ||||||||
Attachments: |
|
Description
zalan
2019-04-15 12:39:17 PDT
Created attachment 367443 [details]
Patch
Comment on attachment 367443 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=367443&action=review > Source/WebKit/ChangeLog:3 > + Drawingarea should only capture painting related milestones DrawingArea? Drawing Area? Created attachment 367444 [details]
Patch
Comment on attachment 367444 [details] Patch Clearing flags on attachment: 367444 Committed r244291: <https://trac.webkit.org/changeset/244291> All reviewed patches have been landed. Closing bug. Seems like this commit broke API test: AutoLayoutRenderingProgressRelativeOrdering. EWS did indicate this API test failure (on obsolete patch of this bug). See https://ews-build.webkit.org/#/builders/3/builds/578 (In reply to Aakash Jain from comment #6) > Seems like this commit broke API test: > AutoLayoutRenderingProgressRelativeOrdering. > > EWS did indicate this API test failure (on obsolete patch of this bug). See > https://ews-build.webkit.org/#/builders/3/builds/578 This might require some test changes. See https://bugs.webkit.org/show_bug.cgi?id=196948#c3 (In reply to zalan from comment #7) > (In reply to Aakash Jain from comment #6) > > Seems like this commit broke API test: > > AutoLayoutRenderingProgressRelativeOrdering. > > > > EWS did indicate this API test failure (on obsolete patch of this bug). See > > https://ews-build.webkit.org/#/builders/3/builds/578 > This might require some test changes. See > https://bugs.webkit.org/show_bug.cgi?id=196948#c3 It seems like autosizing clients rely on this order. However I am not convinced that updateIntrinsicContentSizeIfNeeded should be tied to flushLayer() as it looks purely layoutish (and surely auto-sizing only). (In reply to zalan from comment #8) > (In reply to zalan from comment #7) > > (In reply to Aakash Jain from comment #6) > > > Seems like this commit broke API test: > > > AutoLayoutRenderingProgressRelativeOrdering. > > > > > > EWS did indicate this API test failure (on obsolete patch of this bug). See > > > https://ews-build.webkit.org/#/builders/3/builds/578 > > This might require some test changes. See > > https://bugs.webkit.org/show_bug.cgi?id=196948#c3 > > It seems like autosizing clients rely on this order. However I am not > convinced that updateIntrinsicContentSizeIfNeeded should be tied to > flushLayer() as it looks purely layoutish (and surely auto-sizing only). It may appear "layoutish" but in platform toolkit land "layout" and "painting" happen synchronously and take effect simultaneously, so it's likely important that autosizing changes and painted-content changes happen at the same time. (In reply to Tim Horton from comment #9) > (In reply to zalan from comment #8) > > (In reply to zalan from comment #7) > > > (In reply to Aakash Jain from comment #6) > > > > Seems like this commit broke API test: > > > > AutoLayoutRenderingProgressRelativeOrdering. > > > > > > > > EWS did indicate this API test failure (on obsolete patch of this bug). See > > > > https://ews-build.webkit.org/#/builders/3/builds/578 > > > This might require some test changes. See > > > https://bugs.webkit.org/show_bug.cgi?id=196948#c3 > > > > It seems like autosizing clients rely on this order. However I am not > > convinced that updateIntrinsicContentSizeIfNeeded should be tied to > > flushLayer() as it looks purely layoutish (and surely auto-sizing only). > > It may appear "layoutish" but in platform toolkit land "layout" and > "painting" happen synchronously and take effect simultaneously, so it's > likely important that autosizing changes and painted-content changes happen > at the same time. The callback is in flushLayers() and not in the commit handler, so not sure what "same time" means in that context. (In reply to zalan from comment #10) > (In reply to Tim Horton from comment #9) > > (In reply to zalan from comment #8) > > > (In reply to zalan from comment #7) > > > > (In reply to Aakash Jain from comment #6) > > > > > Seems like this commit broke API test: > > > > > AutoLayoutRenderingProgressRelativeOrdering. > > > > > > > > > > EWS did indicate this API test failure (on obsolete patch of this bug). See > > > > > https://ews-build.webkit.org/#/builders/3/builds/578 > > > > This might require some test changes. See > > > > https://bugs.webkit.org/show_bug.cgi?id=196948#c3 > > > > > > It seems like autosizing clients rely on this order. However I am not > > > convinced that updateIntrinsicContentSizeIfNeeded should be tied to > > > flushLayer() as it looks purely layoutish (and surely auto-sizing only). > > > > It may appear "layoutish" but in platform toolkit land "layout" and > > "painting" happen synchronously and take effect simultaneously, so it's > > likely important that autosizing changes and painted-content changes happen > > at the same time. > The callback is in flushLayers() and not in the commit handler, so not sure > what "same time" means in that context. Unless "painted-content" changes and the actual painting are unrelated. Comment on attachment 367444 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=367444&action=review > Source/WebKit/WebProcess/WebPage/WebPage.cpp:6229 > + static auto paintMilestones = OptionSet<WebCore::LayoutMilestone> { DidHitRelevantRepaintedObjectsAreaThreshold, DidFirstFlushForHeaderLayer, DidFirstPaintAfterSuppressedIncrementalRendering, DidRenderSignificantAmountOfText, DidFirstMeaningfulPaint }; This can be made ever so slightly more optimal by making this a constexpr and not static. |