Bug 173955 – ResourceLoadObserver clean up

• Status: RESOLVED
• Resolution: FIXED
• Component: WebCore Misc.

[Chris Dumez]

ResourceLoadObserver clean up: Modernize code a bit and get rid of unused variables.

[Chris Dumez]

Created attachment 314098 [details]
Patch

[Sam Weinig]

Comment on attachment 314098 [details]
Patch

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

> Source/WebCore/loader/ResourceLoadObserver.cpp:80
> +    m_queue->dispatch([this] {

Is this safe? What is keeping this alive?  Don't we usually do [protectedThis = makeRef(*this)] for things like this? (Same question for other queue usage.

> Source/WebCore/loader/ResourceLoadObserver.cpp:114
> +    return Settings::resourceLoadStatisticsEnabled() && !page->usesEphemeralSession() && m_store;

Not related to the cleanup, but why is this a static setting?

[Chris Dumez]

Comment on attachment 314098 [details]
Patch

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

>> Source/WebCore/loader/ResourceLoadObserver.cpp:80
>> +    m_queue->dispatch([this] {
> 
> Is this safe? What is keeping this alive?  Don't we usually do [protectedThis = makeRef(*this)] for things like this? (Same question for other queue usage.

I guess this wasn't done in this class because ResourceLoadObserver is a singleton.

>> Source/WebCore/loader/ResourceLoadObserver.cpp:114
>> +    return Settings::resourceLoadStatisticsEnabled() && !page->usesEphemeralSession() && m_store;
> 
> Not related to the cleanup, but why is this a static setting?

question for Brent or John.

[Brent Fulgham]

Comment on attachment 314098 [details]
Patch

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

R=me

>>> Source/WebCore/loader/ResourceLoadObserver.cpp:80
>>> +    m_queue->dispatch([this] {
>> 
>> Is this safe? What is keeping this alive?  Don't we usually do [protectedThis = makeRef(*this)] for things like this? (Same question for other queue usage.
> 
> I guess this wasn't done in this class because ResourceLoadObserver is a singleton.

Yeah, ResourceLoadObserver is NeverDestroyed.

>>> Source/WebCore/loader/ResourceLoadObserver.cpp:114
>>> +    return Settings::resourceLoadStatisticsEnabled() && !page->usesEphemeralSession() && m_store;
>> 
>> Not related to the cleanup, but why is this a static setting?
> 
> question for Brent or John.

It’s certainly a system-wide setting, but it really should be at the same level as cookie accept policy. I’m not tied to it being a static setting if there is a cleaner way to represent it,  It’s certainly not something we control on a page level.

> Source/WebCore/loader/ResourceLoadObserver.cpp:-616
> -    return primaryDomain;

Should we hold onto the comment here? (Use host if there is no TLD)

That’s not obvious to me, and might be surprising to others, too.

[Sam Weinig]

(In reply to Chris Dumez from comment #3)
> Comment on attachment 314098 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=314098&action=review
> 
> >> Source/WebCore/loader/ResourceLoadObserver.cpp:80
> >> +    m_queue->dispatch([this] {
> > 
> > Is this safe? What is keeping this alive?  Don't we usually do [protectedThis = makeRef(*this)] for things like this? (Same question for other queue usage.
> 
> I guess this wasn't done in this class because ResourceLoadObserver is a
> singleton.

Ah. It's quite sad that we added another singleton. I've been working for a couple of years to make things owner (or at least shared ownership) by the page/view so separate users of WebKit in the same address space can not stomp on each other. Non-ephemeral session state is the white whale we haven't gotten to yet, but here lies another.

[Brent Fulgham]

(In reply to Sam Weinig from comment #5)
> (In reply to Chris Dumez from comment #3)
> > Comment on attachment 314098 [details]
> > Patch
> > 
> > View in context:
> > https://bugs.webkit.org/attachment.cgi?id=314098&action=review
> > 
> > >> Source/WebCore/loader/ResourceLoadObserver.cpp:80
> > >> +    m_queue->dispatch([this] {
> > > 
> > > Is this safe? What is keeping this alive?  Don't we usually do [protectedThis = makeRef(*this)] for things like this? (Same question for other queue usage.
> > 
> > I guess this wasn't done in this class because ResourceLoadObserver is a
> > singleton.
> 
> Ah. It's quite sad that we added another singleton. I've been working for a
> couple of years to make things owner (or at least shared ownership) by the
> page/view so separate users of WebKit in the same address space can not
> stomp on each other. Non-ephemeral session state is the white whale we
> haven't gotten to yet, but here lies another.

White whale! :)

I.m totally happy to change this to something better. We think this is a global setting, but maybe there are cases where we want to compartmentalize this more,

[Sam Weinig]

(In reply to Brent Fulgham from comment #4)
> Comment on attachment 314098 [details]
> 
> >>> Source/WebCore/loader/ResourceLoadObserver.cpp:114
> >>> +    return Settings::resourceLoadStatisticsEnabled() && !page->usesEphemeralSession() && m_store;
> >> 
> >> Not related to the cleanup, but why is this a static setting?
> > 
> > question for Brent or John.
> 
> It’s certainly a system-wide setting, but it really should be at the same
> level as cookie accept policy. I’m not tied to it being a static setting if
> there is a cleaner way to represent it,  It’s certainly not something we
> control on a page level.

In WebCore, there really isn't supposed to be anything wider than the Page level. Network and storage policies might be shared among WKWebViews at the WebKit level (via WKPreferences or maybe one day, the WKWebSiteDataStore will hold them), but when you get down to WebCore, the goal is to allow Pages to be maintain that state for all the objects below it. Some of those come through Setting (or properties directly on Page, like SessionID), some though clients registered with the Page at its creation.

[Sam Weinig]

(In reply to Brent Fulgham from comment #6)
> (In reply to Sam Weinig from comment #5)
> > (In reply to Chris Dumez from comment #3)
> > > Comment on attachment 314098 [details]
> > > Patch
> > > 
> > > View in context:
> > > https://bugs.webkit.org/attachment.cgi?id=314098&action=review
> > > 
> > > >> Source/WebCore/loader/ResourceLoadObserver.cpp:80
> > > >> +    m_queue->dispatch([this] {
> > > > 
> > > > Is this safe? What is keeping this alive?  Don't we usually do [protectedThis = makeRef(*this)] for things like this? (Same question for other queue usage.
> > > 
> > > I guess this wasn't done in this class because ResourceLoadObserver is a
> > > singleton.
> > 
> > Ah. It's quite sad that we added another singleton. I've been working for a
> > couple of years to make things owner (or at least shared ownership) by the
> > page/view so separate users of WebKit in the same address space can not
> > stomp on each other. Non-ephemeral session state is the white whale we
> > haven't gotten to yet, but here lies another.
> 
> White whale! :)
> 
> I.m totally happy to change this to something better. We think this is a
> global setting, but maybe there are cases where we want to compartmentalize
> this more,

What exactly do you mean by "we think this is a global setting"?  What makes a setting a good candidate for being global?

[Chris Dumez]

Committed r218914: <http://trac.webkit.org/changeset/218914>