Bug 193843 – Document::updateMainArticleElementAfterLayout() should be a no-op when no client depends on knowing the main article element

• Status: RESOLVED
• Resolution: FIXED
• Component: WebCore Misc.

[Simon Fraser (smfr)]

This is just wasted work.

[Wenson Hsieh]

That's a good point — we should totally gate this on Page's requestedLayoutMilestones().

Is this showing up in traces?

[Simon Fraser (smfr)]

No, just reading code. That milestone is so weird, and shouldn't be invasive in Document and FrameView. It needs to be extracted.

[Wenson Hsieh]

(In reply to Simon Fraser (smfr) from comment #2)
> No, just reading code. That milestone is so weird, and shouldn't be invasive
> in Document and FrameView. It needs to be extracted.

Where do you think this code should be extracted to?

[Wenson Hsieh]

Created attachment 360183 [details]
Fixes the bug

[Wenson Hsieh]

(In reply to Simon Fraser (smfr) from comment #2)
> (…) That milestone is so weird, and shouldn't be invasive in Document and FrameView. It needs to be extracted.

Filed https://bugs.webkit.org/show_bug.cgi?id=193854 to track this.

[zalan]

Comment on attachment 360183 [details]
Fixes the bug

Alternatively you could check this at the callsite (there's only one and we don't really expect more callers to have) and assert it here -to not bring in milestone logic to Document.

[Wenson Hsieh]

(In reply to zalan from comment #6)
> Comment on attachment 360183 [details]
> Fixes the bug
> 
> Alternatively you could check this at the callsite (there's only one and we
> don't really expect more callers to have) and assert it here -to not bring
> in milestone logic to Document.

Sounds reasonable.

[Wenson Hsieh]

Created attachment 360184 [details]
v2

[zalan]

Comment on attachment 360184 [details]
v2

r=me but it will conflict with my change.

[zalan]

(In reply to zalan from comment #9)
> Comment on attachment 360184 [details]
> v2
> 
> r=me but it will conflict with my change.

https://trac.webkit.org/changeset/240519/webkit

[Wenson Hsieh]

(In reply to zalan from comment #9)
> Comment on attachment 360184 [details]
> v2
> 
> r=me but it will conflict with my change.

Since the main article update call was moved into FrameView::updateHasReachedSignificantRenderedTextThreshold, we might as well just bail early in updateHasReachedSignificantRenderedTextThreshold if the client doesn't care about DidRenderSignificantAmountOfText.

[Wenson Hsieh]

Created attachment 360193 [details]
Rebase on trunk

[WebKit Commit Bot]

Comment on attachment 360193 [details]
Rebase on trunk

Clearing flags on attachment: 360193

Committed r240539: <https://trac.webkit.org/changeset/240539>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/47570343>