WKWebsiteDataStore should handle media keys

• Component: New Bugs
• Status: RESOLVED FIXED

[Anders Carlsson]

WKWebsiteDataStore should handle media keys

[Anders Carlsson]

Created attachment 253754 [details]
Patch

[Darin Adler]

Comment on attachment 253754 [details]
Patch

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

Looks good. Nits only.

> Source/WebKit2/UIProcess/WebsiteData/WebsiteDataStore.cpp:145
> +    struct CallbackAggregator final : public ThreadSafeRefCounted<CallbackAggregator> {

Not sure we need “public” here since struct derivation is public by default. Style question, I guess.

> Source/WebKit2/UIProcess/WebsiteData/WebsiteDataStore.cpp:349
> +        WTF::StringCapture mediaKeysStorageDirectory { m_mediaKeysStorageDirectory };

Should just be StringCapture, right? Why the WTF prefix?

> Source/WebKit2/UIProcess/WebsiteData/WebsiteDataStore.cpp:818
> +    for (const auto& originPath : WebCore::listDirectory(mediaKeysStorageDirectory, "*")) {

Not sure the const here is needed or helpful.

> Source/WebKit2/UIProcess/WebsiteData/WebsiteDataStore.cpp:837
> +    for (const auto& mediaKeyDirectory : WebCore::listDirectory(mediaKeysStorageDirectory, "*")) {

Not sure the const here is needed or helpful.

> Source/WebKit2/UIProcess/WebsiteData/WebsiteDataStore.cpp:859
> +    for (const auto& origin : origins) {

Not sure the const here is needed or helpful.

> Source/WebKit2/UIProcess/WebsiteData/WebsiteDataStore.cpp:862
> +        auto mediaKeyDirectory = WebCore::pathByAppendingComponent(mediaKeysStorageDirectory, origin->databaseIdentifier());
> +
> +        auto mediaKeyFile = WebCore::pathByAppendingComponent(mediaKeyDirectory, "SecureStop.plist");

I’d omit this blank line.

[Anders Carlsson]

(In reply to comment #2)
> Comment on attachment 253754 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=253754&action=review
> 
> Looks good. Nits only.
> 
> > Source/WebKit2/UIProcess/WebsiteData/WebsiteDataStore.cpp:145
> > +    struct CallbackAggregator final : public ThreadSafeRefCounted<CallbackAggregator> {
> 
> Not sure we need “public” here since struct derivation is public by default.
> Style question, I guess.

I got rid of it.

> 
> > Source/WebKit2/UIProcess/WebsiteData/WebsiteDataStore.cpp:349
> > +        WTF::StringCapture mediaKeysStorageDirectory { m_mediaKeysStorageDirectory };
> 
> Should just be StringCapture, right? Why the WTF prefix?

I blame autocomplete!

> 
> > Source/WebKit2/UIProcess/WebsiteData/WebsiteDataStore.cpp:818
> > +    for (const auto& originPath : WebCore::listDirectory(mediaKeysStorageDirectory, "*")) {
> 
> Not sure the const here is needed or helpful.

I think it's helpful because there's no point in allowing mutation of the strings returned.
> > +
> > +        auto mediaKeyFile = WebCore::pathByAppendingComponent(mediaKeyDirectory, "SecureStop.plist");
> 
> I’d omit this blank line.

[Anders Carlsson]

Committed r184912: <http://trac.webkit.org/changeset/184912>

[Darin Adler]

Comment on attachment 253754 [details]
Patch

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

> Source/WebKit2/UIProcess/WebsiteData/WebsiteDataStore.cpp:841
> +        if (!WebCore::fileExists(mediaKeyFile))
> +            continue;

Wait, why do we need this? Won’t getFileModificationTime fail if the file doesn’t exist? I suggest deleting this code.