Summary: | Document::updateMainArticleElementAfterLayout() should be a no-op when no client depends on knowing the main article element | ||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Simon Fraser (smfr) <simon.fraser> | ||||||||
Component: | WebCore Misc. | Assignee: | Wenson Hsieh <wenson_hsieh> | ||||||||
Status: | RESOLVED FIXED | ||||||||||
Severity: | Normal | CC: | bdakin, cdumez, commit-queue, dbates, esprehn+autocc, ews-watchlist, kangil.han, rniwa, simon.fraser, thorton, webkit-bug-importer, wenson_hsieh, zalan | ||||||||
Priority: | P2 | Keywords: | InRadar | ||||||||
Version: | WebKit Nightly Build | ||||||||||
Hardware: | Unspecified | ||||||||||
OS: | Unspecified | ||||||||||
See Also: | https://bugs.webkit.org/show_bug.cgi?id=193854 | ||||||||||
Attachments: |
|
Description
Simon Fraser (smfr)
2019-01-25 13:38:12 PST
That's a good point — we should totally gate this on Page's requestedLayoutMilestones(). Is this showing up in traces? No, just reading code. That milestone is so weird, and shouldn't be invasive in Document and FrameView. It needs to be extracted. (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? Created attachment 360183 [details]
Fixes the bug
(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. 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.
(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. Created attachment 360184 [details]
v2
Comment on attachment 360184 [details]
v2
r=me but it will conflict with my change.
(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 (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. Created attachment 360193 [details]
Rebase on trunk
Comment on attachment 360193 [details] Rebase on trunk Clearing flags on attachment: 360193 Committed r240539: <https://trac.webkit.org/changeset/240539> All reviewed patches have been landed. Closing bug. |