Bug 155052 – Add new categories to WKWebsiteData

• Status: NEW
• Component: WebKit2

[John Wilander]

Add new categories to WKWebsiteData – PrevalentResource, UserInteraction, EmbeddedUnderOrigins, and EmbedsOrigins

[John Wilander]

rdar://problem/23845874

[John Wilander]

Created attachment 273058 [details]
Patch

[Brent Fulgham]

Comment on attachment 273058 [details]
Patch

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

This patch is close, but it will bog down the main GUI thread with needless copying and other computations. Please correct that.

> Source/WebKit2/UIProcess/WebsiteData/WebsiteDataStore.cpp:465
> +        

No blank line here, please.

> Source/WebKit2/UIProcess/WebsiteData/WebsiteDataStore.cpp:482
> +        

No blank line here, please.

> Source/WebKit2/UIProcess/WebsiteData/WebsiteDataStore.cpp:491
> +                    websiteData.entries.append(WebsiteData::Entry { WTFMove(origin), WebsiteDataType::WebsiteDataTypeUserInteraction, 0, 0, 0 });

Why are you doing all of this copying on the main thread? I think the copy step should be done outside the RunLoop::main().dispatch so that we don't lock up the GUI process.

> Source/WebKit2/UIProcess/WebsiteData/WebsiteDataStore.cpp:499
> +        

No blank line here, please.

> Source/WebKit2/UIProcess/WebsiteData/WebsiteDataStore.cpp:508
> +                    websiteData.entries.append(WebsiteData::Entry { WTFMove(origin), WebsiteDataType::WebsiteDataTypeEmbeddedUnderOrigins, 0, originsAndEmbeddedUnderOrigins.get(origin), 0 });

Ditto regarding copying on main thread.

> Source/WebKit2/UIProcess/WebsiteData/WebsiteDataStore.cpp:516
> +        

No blank line here.

> Source/WebKit2/UIProcess/WebsiteData/WebsiteDataStore.cpp:525
> +                    websiteData.entries.append(WebsiteData::Entry { WTFMove(origin), WebsiteDataType::WebsiteDataTypeEmbedsOrigins, 0, originsAndOriginsTheyEmbed.get(origin), 0});

Ditto regarding copying on the main thread.

[John Wilander]

Created attachment 273113 [details]
Patch

[John Wilander]

Thanks for the review! All of Brent's comments addressed.

[Brent Fulgham]

Comment on attachment 273113 [details]
Patch

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

> Source/WebKit2/UIProcess/WebsiteData/WebsiteDataStore.cpp:1190
> +    // FIXME: Not implemented

A small nit: we tend to want a Radar or Bugzilla bug number whenever we have a FIXME comment.

> Source/WebKit2/UIProcess/WebsiteData/WebsiteDataStore.cpp:1199
> +    // FIXME: Not implemented

Ditto.

> Source/WebKit2/UIProcess/WebsiteData/WebsiteDataStore.cpp:1208
> +    // FIXME: Not implemented

Ditto.

> Source/WebKit2/UIProcess/WebsiteData/WebsiteDataStore.cpp:1217
> +    // FIXME: Not implemented

Ditto.

[Brent Fulgham]

This patch looks good to me, but we need Andy to give a final WebKit2 approval.

[John Wilander]

Created attachment 273115 [details]
Patch

[John Wilander]

Fixed the FIXMEs with a reference to rdar://problem/24996840.

[Sam Weinig]

Comment on attachment 273115 [details]
Patch

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

> Source/WebKit2/ChangeLog:30
> +        * DatabaseProcess/DatabaseProcess.cpp:
> +        (WebKit::DatabaseProcess::fetchWebsiteData):
> +        * NetworkProcess/NetworkProcess.cpp:
> +        (WebKit::fetchDiskCacheEntries):
> +        * Shared/WebsiteData/WebsiteData.h:
> +        * Shared/WebsiteData/WebsiteDataType.h:
> +        * UIProcess/API/Cocoa/WKWebsiteDataRecord.mm:
> +        (dataTypesToString):
> +        * UIProcess/API/Cocoa/WKWebsiteDataRecordInternal.h:
> +        (WebKit::toWebsiteDataType):
> +        (WebKit::toWKWebsiteDataTypes):
> +        * UIProcess/API/Cocoa/WKWebsiteDataRecordPrivate.h:
> +        * UIProcess/WebsiteData/WebsiteDataStore.cpp:
> +        (WebKit::WebsiteDataStore::fetchData):
> +        (WebKit::WebsiteDataStore::setResourceLoadStatisticsEnabled):
> +        (WebKit::WebsiteDataStore::prevalentResourceOrigins):
> +        (WebKit::WebsiteDataStore::userInteractionOrigins):
> +        (WebKit::WebsiteDataStore::embeddedUnderOrigins):
> +        (WebKit::WebsiteDataStore::embedsOrigins):
> +        * UIProcess/WebsiteData/WebsiteDataStore.h:
> +        * WebProcess/WebProcess.cpp:
> +        (WebKit::WebProcess::fetchWebsiteData):

Please provide detailed changelog comments.

> Source/WebKit2/Shared/WebsiteData/WebsiteData.h:49
> +        unsigned embeddedUnderOrigins;
> +        unsigned embedsOrigins;

What are embeddedUnderOrigins and embedsOrigins? What do these counts represent?

> Source/WebKit2/Shared/WebsiteData/WebsiteDataType.h:50
> +    WebsiteDataTypePrevalentResource = 1 << 14,

What is a PrevalentResource?

> Source/WebKit2/Shared/WebsiteData/WebsiteDataType.h:51
> +    WebsiteDataTypeUserInteraction = 1 << 15,

How can a a piece of data be of type UserInteraction?

> Source/WebKit2/Shared/WebsiteData/WebsiteDataType.h:53
> +    WebsiteDataTypeEmbeddedUnderOrigins = 1 << 16,
> +    WebsiteDataTypeEmbedsOrigins = 1 << 16,

Same question as above.

> Source/WebKit2/UIProcess/WebsiteData/WebsiteDataStore.h:118
> +    static Vector<RefPtr<WebCore::SecurityOrigin>> prevalentResourceOrigins();
> +    static Vector<RefPtr<WebCore::SecurityOrigin>> userInteractionOrigins();
> +    static HashMap<RefPtr<WebCore::SecurityOrigin>, unsigned> embeddedUnderOrigins();
> +    static HashMap<RefPtr<WebCore::SecurityOrigin>, unsigned> embedsOrigins();

Why are these static?

[John Wilander]

Created attachment 273208 [details]
Patch

[John Wilander]

Added changelog comments per file.
Renamed types and associated functions to make them more clear.

The getter functions are static since I believe the data source will be a static single ton. I'll change those if static doesn't turn out to be the correct choice.

[Andy Estes]

(In reply to comment #12)
> Added changelog comments per file.
> Renamed types and associated functions to make them more clear.
> 
> The getter functions are static since I believe the data source will be a
> static single ton. I'll change those if static doesn't turn out to be the
> correct choice.

Won't the data source be WebsiteDataStore::m_resourceLoadStatistics in this case? That's not a singleton.

[John Wilander]

You are absolutely correct. Uploading new patch.

[John Wilander]

Created attachment 273217 [details]
Patch

[Sam Weinig]

I still don't understand why these new categories are needed. If all the data is being stored as part of the resource load statistics, that seems like the only category you need.

[Sam Weinig]

Comment on attachment 273217 [details]
Patch

r-ing because I don't understand why we need these new types.