Bug 190055 - Resource Load Statistics: Non-redirected top frame navigation should not get captured in statistics
Summary: Resource Load Statistics: Non-redirected top frame navigation should not get ...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit Misc. (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: John Wilander
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2018-09-27 14:17 PDT by John Wilander
Modified: 2018-09-28 15:41 PDT (History)
6 users (show)

See Also:


Attachments
Patch (8.50 KB, patch)
2018-09-27 14:27 PDT, John Wilander
no flags Details | Formatted Diff | Diff
Patch (8.60 KB, patch)
2018-09-27 14:41 PDT, John Wilander
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description John Wilander 2018-09-27 14:17:17 PDT
We should skip statistics collection of regular top frame navigational redirects.
Comment 1 John Wilander 2018-09-27 14:18:08 PDT
Sorry, we should skip statistics collection of regular top frame navigational loads. Redirects should still be captured.
Comment 2 Radar WebKit Bug Importer 2018-09-27 14:18:19 PDT
<rdar://problem/44843460>
Comment 3 John Wilander 2018-09-27 14:27:21 PDT
Created attachment 351002 [details]
Patch
Comment 4 Chris Dumez 2018-09-27 14:34:01 PDT
Comment on attachment 351002 [details]
Patch

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

> LayoutTests/http/tests/resourceLoadStatistics/do-not-capture-statistics-for-simple-top-navigations-expected.txt:4
> +PASS testRunner.isStatisticsRegisteredAsSubFrameUnder("http://localhost", "http://127.0.0.1") is false

Having PASS lines *after* the "TEST COMPLETE" line indicates the test is written wrong and that it will likely be flaky.

> LayoutTests/http/tests/resourceLoadStatistics/do-not-capture-statistics-for-simple-top-navigations.html:5
> +    <title>Test that a non-redirected top frame navigation doesn't get captured in statistics</title>

Would be nice to use description("Test that a non-redirected top frame navigation doesn't get captured in statistics"); instead of that it shows in the output.

> LayoutTests/http/tests/resourceLoadStatistics/do-not-capture-statistics-for-simple-top-navigations.html:15
> +            testRunner.notifyDone();

Should be:
finishJSTest();

> LayoutTests/http/tests/resourceLoadStatistics/do-not-capture-statistics-for-simple-top-navigations.html:23
> +                    testRunner.waitUntilDone();

Please use:
jsTestIsAsync = true;

right after the description();
Comment 5 John Wilander 2018-09-27 14:41:48 PDT
Created attachment 351005 [details]
Patch
Comment 6 Chris Dumez 2018-09-27 14:51:05 PDT
Comment on attachment 351005 [details]
Patch

r=me if the bots are happy.
Comment 7 John Wilander 2018-09-27 15:06:59 PDT
Thank you, Chris!
Comment 8 WebKit Commit Bot 2018-09-27 17:23:44 PDT
Comment on attachment 351005 [details]
Patch

Clearing flags on attachment: 351005

Committed r236578: <https://trac.webkit.org/changeset/236578>
Comment 9 WebKit Commit Bot 2018-09-27 17:23:46 PDT
All reviewed patches have been landed.  Closing bug.
Comment 10 John Wilander 2018-09-28 15:41:51 PDT
Follow-up change tracked in https://bugs.webkit.org/show_bug.cgi?id=190097.