Bug 193843

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 Flags
Fixes the bug
none
v2
none
Rebase on trunk none

Simon Fraser (smfr)
Reported 2019-01-25 13:38:12 PST
This is just wasted work.
Attachments
Fixes the bug (2.10 KB, patch)
2019-01-25 16:04 PST, Wenson Hsieh 		no flags
DetailsFormatted DiffDiff
v2 (3.00 KB, patch)
2019-01-25 16:23 PST, Wenson Hsieh 		no flags
DetailsFormatted DiffDiff
Rebase on trunk (2.65 KB, patch)
2019-01-25 17:01 PST, Wenson Hsieh 		no flags
DetailsFormatted DiffDiff
Show Obsolete (2) View All Add attachment proposed patch, testcase, etc.
Wenson Hsieh
Comment 1 2019-01-25 13:41:10 PST
That's a good point — we should totally gate this on Page's requestedLayoutMilestones(). Is this showing up in traces?
Simon Fraser (smfr)
Comment 2 2019-01-25 13:42:00 PST
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
Comment 3 2019-01-25 14:51:56 PST
(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
Comment 4 2019-01-25 16:04:09 PST
Created attachment 360183 [details] Fixes the bug
Wenson Hsieh
Comment 5 2019-01-25 16:06:57 PST
(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.
alan
Comment 6 2019-01-25 16:08:44 PST
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
Comment 7 2019-01-25 16:14:55 PST
(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
Comment 8 2019-01-25 16:23:46 PST
Created attachment 360184 [details] v2
alan
Comment 9 2019-01-25 16:24:54 PST
Comment on attachment 360184 [details] v2 r=me but it will conflict with my change.
alan
Comment 10 2019-01-25 16:26:11 PST
(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
Comment 11 2019-01-25 16:54:51 PST
(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
Comment 12 2019-01-25 17:01:25 PST
Created attachment 360193 [details] Rebase on trunk
WebKit Commit Bot
Comment 13 2019-01-25 19:39:03 PST
Comment on attachment 360193 [details] Rebase on trunk Clearing flags on attachment: 360193 Committed r240539: <https://trac.webkit.org/changeset/240539>
WebKit Commit Bot
Comment 14 2019-01-25 19:39:05 PST
All reviewed patches have been landed. Closing bug.
Radar WebKit Bug Importer
Comment 15 2019-01-25 19:40:29 PST
<rdar://problem/47570343>
Note You need to log in before you can comment on or make changes to this bug.