Bug 217400 - Adjust heuristic for checking whether view reaches visually non-empty state
Summary: Adjust heuristic for checking whether view reaches visually non-empty state
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Sihui Liu
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2020-10-06 12:08 PDT by Sihui Liu
Modified: 2020-10-29 12:15 PDT (History)
5 users (show)

See Also:


Attachments
Patch (2.44 KB, patch)
2020-10-06 12:13 PDT, Sihui Liu
no flags Details | Formatted Diff | Diff
Patch (2.80 KB, patch)
2020-10-07 10:00 PDT, Sihui Liu
no flags Details | Formatted Diff | Diff
Patch (10.45 KB, patch)
2020-10-07 14:37 PDT, Sihui Liu
ews-feeder: commit-queue-
Details | Formatted Diff | Diff
Patch (10.97 KB, patch)
2020-10-07 14:54 PDT, Sihui Liu
no flags Details | Formatted Diff | Diff
Patch for landing (11.09 KB, patch)
2020-10-08 09:06 PDT, Sihui Liu
no flags Details | Formatted Diff | Diff
Patch for landing (10.22 KB, patch)
2020-10-29 11:44 PDT, Sihui Liu
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Sihui Liu 2020-10-06 12:08:21 PDT
We decide that view reaches visually non-empty state and fire corresponding events when main document is parsed and no more content (font or css resources) to be loaded. 
An edge case is when the page has an empty document and depends on async scripts to fill content (like wpt.fyi), then we may fire the events too early.
Comment 1 Sihui Liu 2020-10-06 12:10:09 PDT
rdar://problem/69894849
Comment 2 Sihui Liu 2020-10-06 12:13:17 PDT
Created attachment 410677 [details]
Patch
Comment 3 Darin Adler 2020-10-07 09:13:34 PDT
Comment on attachment 410677 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=410677&action=review

> Source/WebCore/page/FrameView.cpp:4580
> +            bool hasScriptResource = false;

Should be named isLoadingScript or something more like that. This definitely does not answer the question, “Does this have a script resource?”

> Source/WebCore/page/FrameView.cpp:4584
> +                auto resourceType = resource.value->type();

In this narrow context just “type” would be a good variable name.

> Source/WebCore/page/FrameView.cpp:4587
> +                if (!hasScriptResource && resourceType == CachedResource::Type::Script)

Don’t need to check !hasScriptResource here.
Comment 4 Sihui Liu 2020-10-07 10:00:20 PDT
Created attachment 410754 [details]
Patch
Comment 5 Geoffrey Garen 2020-10-07 12:37:58 PDT
Comment on attachment 410754 [details]
Patch

r=me

Please double-check the iOS-wk2 EWS failure.
Comment 6 zalan 2020-10-07 12:42:01 PDT
I hope we eventually send out the non-empty milestone even when the script does not generate any content.
Comment 7 Geoffrey Garen 2020-10-07 12:46:50 PDT
(In reply to zalan from comment #6)
> I hope we eventually send out the non-empty milestone even when the script
> does not generate any content.

Good point. I think you can add an API test for this by using an empty document with a <script src="..."> that points to an empty script, and then verifying that the visually non-empty milestone fires. Seems worth doing (but probably doesn't need to block landing the patch).
Comment 8 Geoffrey Garen 2020-10-07 12:50:22 PDT
Now that I think about it, the test should also verify this new behavior: that the mouth stone does not fire before the script has loaded. You can detect when the script loads by using the api testing url scheme. Or by having the script signal the api test somehow.
Comment 9 Sihui Liu 2020-10-07 14:37:13 PDT
Created attachment 410781 [details]
Patch
Comment 10 Sihui Liu 2020-10-07 14:54:30 PDT
Created attachment 410786 [details]
Patch
Comment 11 EWS 2020-10-07 17:53:21 PDT
ChangeLog entry in Tools/ChangeLog contains OOPS!.
Comment 12 Sihui Liu 2020-10-08 09:06:41 PDT
Created attachment 410849 [details]
Patch for landing
Comment 13 EWS 2020-10-08 10:59:54 PDT
Committed r268192: <https://trac.webkit.org/changeset/268192>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 410849 [details].
Comment 14 Sihui Liu 2020-10-29 11:44:21 PDT
Reopening to attach new patch.
Comment 15 Sihui Liu 2020-10-29 11:44:22 PDT
Created attachment 412670 [details]
Patch for landing
Comment 16 EWS 2020-10-29 12:15:54 PDT
Committed r269160: <https://trac.webkit.org/changeset/269160>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 412670 [details].