Bug 196926 – Drawingarea should only capture painting related milestones

• Status: RESOLVED
• Resolution: FIXED
• Component: Layout and Rendering

[alan]

<rdar://problem/48003845>

[alan]

Created attachment 367443 [details]
Patch

[Tim Horton]

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?

[alan]

Created attachment 367444 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 367444 [details]
Patch

Clearing flags on attachment: 367444

Committed r244291: <https://trac.webkit.org/changeset/244291>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Aakash Jain]

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

[alan]

(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

[alan]

(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).

[Tim Horton]

(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.

[alan]

(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.

[alan]

(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.

[Daniel Bates]

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.