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

Description Simon Fraser (smfr) 2019-01-25 13:38:12 PST
This is just wasted work.
Comment 1 Wenson Hsieh 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?
Comment 2 Simon Fraser (smfr) 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.
Comment 3 Wenson Hsieh 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?
Comment 4 Wenson Hsieh 2019-01-25 16:04:09 PST
Created attachment 360183 [details]
Fixes the bug
Comment 5 Wenson Hsieh 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.
Comment 6 zalan 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.
Comment 7 Wenson Hsieh 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.
Comment 8 Wenson Hsieh 2019-01-25 16:23:46 PST
Created attachment 360184 [details]
v2
Comment 9 zalan 2019-01-25 16:24:54 PST
Comment on attachment 360184 [details]
v2

r=me but it will conflict with my change.
Comment 10 zalan 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
Comment 11 Wenson Hsieh 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.
Comment 12 Wenson Hsieh 2019-01-25 17:01:25 PST
Created attachment 360193 [details]
Rebase on trunk
Comment 13 WebKit Commit Bot 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>
Comment 14 WebKit Commit Bot 2019-01-25 19:39:05 PST
All reviewed patches have been landed.  Closing bug.
Comment 15 Radar WebKit Bug Importer 2019-01-25 19:40:29 PST
<rdar://problem/47570343>