Bug 171949 – Resource Load Statistics: Enable configuration through preferences

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKit2

[John Wilander]

We should enable configuration of time constants to allow testing with short time spans.

[John Wilander]

<rdar://problem/31894518>

[John Wilander]

Created attachment 309650 [details]
Patch

[John Wilander]

Created attachment 309664 [details]
Patch

[Alex Christensen]

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)

[John Wilander]

(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!

[John Wilander]

Created attachment 309748 [details]
Patch for landing

[WebKit Commit Bot]

The commit-queue encountered the following flaky tests while processing attachment 309748 [details]:

workers/bomb.html bug 171985 (author: [e-mail])
The commit-queue is continuing to process your patch.

[WebKit Commit Bot]

Comment on attachment 309748 [details]
Patch for landing

Clearing flags on attachment: 309748

Committed r216690: <http://trac.webkit.org/changeset/216690>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Csaba Osztrogonác]

(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

[Csaba Osztrogonác]

fix landed in https://trac.webkit.org/changeset/216844

[Csaba Osztrogonác]

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)

[Csaba Osztrogonác]

(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?

[Csaba Osztrogonác]

(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?

[John Wilander]

(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?

[Alex Christensen]

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.

[Csaba Osztrogonác]

(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.

[Alex Christensen]

You have correctly noticed that the Mac CMake build is not a high priority right now.