Bug 193843 - Document::updateMainArticleElementAfterLayout() should be a no-op when no client depends on knowing the main article element
Summary: Document::updateMainArticleElementAfterLayout() should be a no-op when no cli...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebCore Misc. (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Wenson Hsieh
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2019-01-25 13:38 PST by Simon Fraser (smfr)
Modified: 2019-01-25 19:40 PST (History)
13 users (show)

See Also:


Attachments
Fixes the bug (2.10 KB, patch)
2019-01-25 16:04 PST, Wenson Hsieh
no flags Details | Formatted Diff | Diff
v2 (3.00 KB, patch)
2019-01-25 16:23 PST, Wenson Hsieh
no flags Details | Formatted Diff | Diff
Rebase on trunk (2.65 KB, patch)
2019-01-25 17:01 PST, Wenson Hsieh
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
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>