Bug 171949 - Resource Load Statistics: Enable configuration through preferences
Summary: Resource Load Statistics: Enable configuration through preferences
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit2 (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: John Wilander
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2017-05-10 15:42 PDT by John Wilander
Modified: 2017-05-26 09:09 PDT (History)
10 users (show)

See Also:


Attachments
Patch (25.67 KB, patch)
2017-05-10 15:49 PDT, John Wilander
no flags Details | Formatted Diff | Diff
Patch (12.88 KB, patch)
2017-05-10 17:43 PDT, John Wilander
no flags Details | Formatted Diff | Diff
Patch for landing (12.92 KB, patch)
2017-05-11 12:11 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 2017-05-10 15:42:24 PDT
We should enable configuration of time constants to allow testing with short time spans.
Comment 1 John Wilander 2017-05-10 15:44:16 PDT
<rdar://problem/31894518>
Comment 2 John Wilander 2017-05-10 15:49:35 PDT
Created attachment 309650 [details]
Patch
Comment 3 John Wilander 2017-05-10 17:43:30 PDT
Created attachment 309664 [details]
Patch
Comment 4 Alex Christensen 2017-05-11 10:36:00 PDT
Comment on attachment 309664 [details]
Patch

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

> Source/WebCore/loader/ResourceLoadObserver.cpp:437
> +    if (seconds > 0)

Should the callee check this?  Maybe an assertion?  We have checks in the caller.

> Source/WebKit2/UIProcess/Cocoa/WebResourceLoadStatisticsManagerCocoa.mm:43
> +        // Max 30 days in seconds.
> +        if (timeToLiveUserInteraction > 0 && timeToLiveUserInteraction <= 2592000)

const size_t secondsPerDay = 24 * 3600;
... && timeToLiveUserInteraction <= 30 * secondsPerDay)

> Source/WebKit2/UIProcess/Cocoa/WebResourceLoadStatisticsManagerCocoa.mm:48
> +        // Max 1 day in seconds.
> +        if (timeToLiveCookiePartitionFree > 0 && timeToLiveCookiePartitionFree <= 86400)

... timeToLiveCookiePartitionFree <= 1 * secondsPerDay)
Comment 5 John Wilander 2017-05-11 10:45:48 PDT
(In reply to Alex Christensen from comment #4)
> Comment on attachment 309664 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=309664&action=review
> 
> > Source/WebCore/loader/ResourceLoadObserver.cpp:437
> > +    if (seconds > 0)
> 
> Should the callee check this?  Maybe an assertion?  We have checks in the
> caller.

Since I use it to divide I'd like to be really sure. We may use this in layout tests too if we get bugs that are easier to reproduce with short timeouts.

> > Source/WebKit2/UIProcess/Cocoa/WebResourceLoadStatisticsManagerCocoa.mm:43
> > +        // Max 30 days in seconds.
> > +        if (timeToLiveUserInteraction > 0 && timeToLiveUserInteraction <= 2592000)
> 
> const size_t secondsPerDay = 24 * 3600;
> ... && timeToLiveUserInteraction <= 30 * secondsPerDay)

Good suggestion, will fix.

> > Source/WebKit2/UIProcess/Cocoa/WebResourceLoadStatisticsManagerCocoa.mm:48
> > +        // Max 1 day in seconds.
> > +        if (timeToLiveCookiePartitionFree > 0 && timeToLiveCookiePartitionFree <= 86400)
> 
> ... timeToLiveCookiePartitionFree <= 1 * secondsPerDay)

Will fix.

Thanks, Alex!
Comment 6 John Wilander 2017-05-11 12:11:23 PDT
Created attachment 309748 [details]
Patch for landing
Comment 7 WebKit Commit Bot 2017-05-11 13:03:35 PDT
The commit-queue encountered the following flaky tests while processing attachment 309748 [details]:

workers/bomb.html bug 171985 (author: fpizlo@apple.com)
The commit-queue is continuing to process your patch.
Comment 8 WebKit Commit Bot 2017-05-11 13:04:07 PDT
Comment on attachment 309748 [details]
Patch for landing

Clearing flags on attachment: 309748

Committed r216690: <http://trac.webkit.org/changeset/216690>
Comment 9 WebKit Commit Bot 2017-05-11 13:04:09 PDT
All reviewed patches have been landed.  Closing bug.
Comment 10 Csaba Osztrogonác 2017-05-14 06:27:47 PDT
(In reply to WebKit Commit Bot from comment #8)
> Comment on attachment 309748 [details]
> Patch for landing
> 
> Clearing flags on attachment: 309748
> 
> Committed r216690: <http://trac.webkit.org/changeset/216690>

It broke the Apple Mac cmake build:
Undefined symbols for architecture x86_64:
  "WebKit::WebResourceLoadStatisticsManager::registerUserDefaultsIfNeeded()", referenced from:
      WebKit::WebResourceLoadStatisticsStore::registerSharedResourceLoadObserver(std::__1::function<void (WTF::Vector<WTF::String, 0ul, WTF::CrashOnOverflow, 16ul> const&, WTF::Vector<WTF::String, 0ul, WTF::CrashOnOverflow, 16ul> const&, bool)>&&) in WebResourceLoadStatisticsStore.cpp.o
ld: symbol(s) not found for architecture x86_64
clang: error: linker command failed with exit code 1 (use -v to see invocation)
make[2]: *** [lib/WebKit.framework/Versions/A/WebKit] Error 1
make[1]: *** [Source/WebKit2/CMakeFiles/WebKit2.dir/all] Error 2
make: *** [all] Error 2
Comment 11 Csaba Osztrogonác 2017-05-14 06:28:27 PDT
fix landed in https://trac.webkit.org/changeset/216844
Comment 12 Csaba Osztrogonác 2017-05-14 06:33:12 PDT
https://lists.webkit.org/pipermail/webkit-dev/2015-October/027752.html

Apple Mac has cmake build system and buildbot 1.5 years ago. Is it still 
Alex's hobby project or is there any plan to maintain officially by Apple?
(and not breaking this build day by day)
Comment 13 Csaba Osztrogonác 2017-05-17 01:45:28 PDT
(In reply to Csaba Osztrogonác from comment #12)
> https://lists.webkit.org/pipermail/webkit-dev/2015-October/027752.html
> 
> Apple Mac has cmake build system and buildbot 1.5 years ago. Is it still 
> Alex's hobby project or is there any plan to maintain officially by Apple?
> (and not breaking this build day by day)

ping?
Comment 14 Csaba Osztrogonác 2017-05-22 03:42:54 PDT
(In reply to Csaba Osztrogonác from comment #12)
> https://lists.webkit.org/pipermail/webkit-dev/2015-October/027752.html
> 
> Apple Mac has cmake build system and buildbot 1.5 years ago. Is it still 
> Alex's hobby project or is there any plan to maintain officially by Apple?
> (and not breaking this build day by day)

ping?
Comment 15 John Wilander 2017-05-22 09:00:36 PDT
(In reply to Csaba Osztrogonác from comment #14)
> (In reply to Csaba Osztrogonác from comment #12)
> > https://lists.webkit.org/pipermail/webkit-dev/2015-October/027752.html
> > 
> > Apple Mac has cmake build system and buildbot 1.5 years ago. Is it still 
> > Alex's hobby project or is there any plan to maintain officially by Apple?
> > (and not breaking this build day by day)
> 
> ping?

Hi Csaba! Thanks for fixing the cmake build. My bad. The cmake build bot is not on EWS, right? Was there ever any talk about that?
Comment 16 Alex Christensen 2017-05-22 10:36:01 PDT
Windows and Linux use exclusively CMake.  The Mac CMake build is just my little project on the back burner right now.  We have a bot because we don't want to lose my progress, but it isn't critical for anything right now.  Thanks for the fix.
Comment 17 Csaba Osztrogonác 2017-05-26 02:59:43 PDT
(In reply to Alex Christensen from comment #16)
> Windows and Linux use exclusively CMake.  The Mac CMake build is just my
> little project on the back burner right now.  We have a bot because we don't
> want to lose my progress, but it isn't critical for anything right now. 
> Thanks for the fix.

Isn't there any intention to try to maintain the build as 
you asked in the cited webkit-dev mail 1.5 years ago?

Because I see that it is broken by Apple employees all the time,
when the fix is so trivial. It's hard to believe that adding a
new file to the cmake build system is so big deal. Not to mention
that nobody fixes after I notice and comment the bug.
Comment 18 Alex Christensen 2017-05-26 09:09:59 PDT
You have correctly noticed that the Mac CMake build is not a high priority right now.