Bug 155117 – Set AVURLAssetUsesNoPersistentCacheKey on AVAsset to match caching policy.

• Component: WebKit2
• Status: RESOLVED FIXED

[Jeremy Jones]

Set AVURLAssetUsesNoPersistentCacheKey on AVAsset to match caching policy.

[Jeremy Jones]

rdar://problem/6802240

[Jeremy Jones]

Created attachment 273189 [details]
Patch

[Jon Lee]

Comment on attachment 273189 [details]
Patch

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

What about WK1?

> Source/WebCore/ChangeLog:8
> +        No new tests because no new funcitonality was added.

functionality*

> Source/WebCore/html/HTMLMediaElement.h:598
> +    bool mediaPlayerUsePersistentCache() override;

Uses* PersistentCache?

> Source/WebCore/page/ChromeClient.h:473
> +    virtual bool isDataStorePersistent() { return true; }

This will probably cause confusion because on its face it sounds like a generic data store, not one specific to media. Can we make it media-specific?

> Source/WebKit2/Shared/WebPageCreationParameters.cpp:58
> +    encoder << isDataStorePersistent;

ditto.

> Source/WebKit2/Shared/WebPageCreationParameters.cpp:138
> +    if (!decoder.decode(parameters.isDataStorePersistent))

ditto.

> Source/WebKit2/Shared/WebPageCreationParameters.h:92
> +    bool isDataStorePersistent;

ditto.

> Source/WebKit2/UIProcess/WebPageProxy.cpp:5170
> +    parameters.isDataStorePersistent = m_websiteDataStore->isPersistent();

ditto.

> Source/WebKit2/WebProcess/WebCoreSupport/WebChromeClient.cpp:1150
> +bool WebChromeClient::isDataStorePersistent()

ditto.

> Source/WebKit2/WebProcess/WebCoreSupport/WebChromeClient.cpp:1152
> +    return m_page->isDataStorePersistent();

ditto.

> Source/WebKit2/WebProcess/WebCoreSupport/WebChromeClient.h:338
> +    bool isDataStorePersistent() override;

ditto.

> Source/WebKit2/WebProcess/WebPage/WebPage.cpp:368
> +    , m_isDataStorePersistent(parameters.isDataStorePersistent)

ditto.

> Source/WebKit2/WebProcess/WebPage/WebPage.h:932
> +    bool isDataStorePersistent() { return m_isDataStorePersistent; }

ditto.

> Source/WebKit2/WebProcess/WebPage/WebPage.h:1447
> +    bool m_isDataStorePersistent;

ditto.

[Jeremy Jones]

(In reply to comment #3)
> Comment on attachment 273189 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=273189&action=review
> 
> What about WK1?

For WebKit1, caches are disabled by setting [[NSURLCache sharedURLCache] setDiskCapacity:0];

I'll add a WK1 implementation that checks this value.

> 
> > Source/WebCore/ChangeLog:8
> > +        No new tests because no new funcitonality was added.
> 
> functionality*

Done.

> 
> > Source/WebCore/html/HTMLMediaElement.h:598
> > +    bool mediaPlayerUsePersistentCache() override;
> 
> Uses* PersistentCache?

Sure.

> 
> > Source/WebCore/page/ChromeClient.h:473
> > +    virtual bool isDataStorePersistent() { return true; }
> 
> This will probably cause confusion because on its face it sounds like a
> generic data store, not one specific to media. Can we make it media-specific?

mediaUsesPersistentCache, match the MediaPlayer version.

> 
> > Source/WebKit2/Shared/WebPageCreationParameters.cpp:58
> > +    encoder << isDataStorePersistent;
> 
> ditto.
ditto....

[Jeremy Jones]

Created attachment 273206 [details]
Patch

[Simon Fraser (smfr)]

Comment on attachment 273206 [details]
Patch

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

> Source/WebCore/ChangeLog:10
> +        This will prevent persistent media caches when webkit is using in memory caching.

in-memory

> Source/WebCore/page/ChromeClient.h:473
> +    virtual bool mediaUsesPersistentCache() { return true; }

Can this be a const method?

> Source/WebCore/platform/graphics/MediaPlayer.h:238
> +    virtual bool mediaPlayerUsesPersistentCache() { return true; }

Can this be const?

> Source/WebKit/mac/WebCoreSupport/WebChromeClient.mm:1060
> +bool WebChromeClient::mediaUsesPersistentCache()
> +{
> +    return [[NSURLCache sharedURLCache] diskCapacity] > 0;
> +}

Why make this about media at all? It seems like what you expose to WebCore is "disk cache enabled".

Also, we have our own disk cache in the network process, so I'm not sure the sharedURLCache capacity is at all relevant. Talk to Antti.

[Jeremy Jones]

Comment on attachment 273206 [details]
Patch

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

>> Source/WebCore/ChangeLog:10
>> +        This will prevent persistent media caches when webkit is using in memory caching.
> 
> in-memory

Done.

>> Source/WebCore/page/ChromeClient.h:473
>> +    virtual bool mediaUsesPersistentCache() { return true; }
> 
> Can this be a const method?

Probably. I'll change if possible.

>> Source/WebCore/platform/graphics/MediaPlayer.h:238
>> +    virtual bool mediaPlayerUsesPersistentCache() { return true; }
> 
> Can this be const?

Probably. I'll change if possible.

>> Source/WebKit/mac/WebCoreSupport/WebChromeClient.mm:1060
>> +}
> 
> Why make this about media at all? It seems like what you expose to WebCore is "disk cache enabled".
> 
> Also, we have our own disk cache in the network process, so I'm not sure the sharedURLCache capacity is at all relevant. Talk to Antti.

My previous patch was not media specific. I received feedback that I should make it media specific.

This is the WK1 implementation.

[Jeremy Jones]

Created attachment 273620 [details]
Patch

[Jeremy Jones]

(In reply to comment #6)
> Comment on attachment 273206 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=273206&action=review
> 
> > Source/WebCore/ChangeLog:10
> > +        This will prevent persistent media caches when webkit is using in memory caching.
> 
> in-memory

Done.

> 
> > Source/WebCore/page/ChromeClient.h:473
> > +    virtual bool mediaUsesPersistentCache() { return true; }
> 
> Can this be a const method?

Yes. Done.

> 
> > Source/WebCore/platform/graphics/MediaPlayer.h:238
> > +    virtual bool mediaPlayerUsesPersistentCache() { return true; }
> 
> Can this be const?

Yes. Done.

> 
> > Source/WebKit/mac/WebCoreSupport/WebChromeClient.mm:1060
> > +bool WebChromeClient::mediaUsesPersistentCache()
> > +{
> > +    return [[NSURLCache sharedURLCache] diskCapacity] > 0;
> > +}
> 
> Why make this about media at all? It seems like what you expose to WebCore
> is "disk cache enabled".

This is about configuring MediaPlayer i.e. AVFoundation to use a persistent cache or not. I don't want this to be used more generally for other caches. So, I've renamed these to "mediaShouldUsePersistentCache" to make it more explicit that you ask this to find out if media should use a cache. It is not telling you if media uses a cache.

WebKit 1 has no cache model for no disk cache. So, this implementation is not a requirement. It is nice to have, since there is a develop menu option to disable cache, which sets -diskCapacity to 0.

> 
> Also, we have our own disk cache in the network process, so I'm not sure the
> sharedURLCache capacity is at all relevant. Talk to Antti.

Not relevant. This implementation is for WK1. The WK2 implementation gets is values from the persistentDataStore.

[Simon Fraser (smfr)]

Comment on attachment 273620 [details]
Patch

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

> Source/WebCore/ChangeLog:5
> +

Radar.

[Jeremy Jones]

Created attachment 273660 [details]
Patch

[Jer Noble]

Comment on attachment 273660 [details]
Patch

Clearing flags on attachment: 273660

Committed r197990: <http://trac.webkit.org/changeset/197990>

[Jer Noble]

All reviewed patches have been landed.  Closing bug.