Bug 155783 – When clearing cache, also clear AVFoundation cache.

• Component: WebKit2
• Status: RESOLVED FIXED

[Jeremy Jones]

When clearing cache, also clear AVFoundation cache.

[Jeremy Jones]

Created attachment 274729 [details]
Patch

[Jeremy Jones]

rdar://problem/25252541

[Darin Adler]

Comment on attachment 274729 [details]
Patch

Might also be good to make this caching respect the Page::isResourceCachingDisabled, which is supposed to bypass caching for developer convenience. Ideally nothing gets found in the cache and also nothing gets written into it either when the flag is set.

[Eric Carlson]

Comment on attachment 274729 [details]
Patch

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

> Source/WebCore/platform/graphics/avfoundation/objc/MediaPlayerPrivateAVFoundationObjC.mm:487
> +    NSArray<String *>* directoryContents = [fileManager contentsOfDirectoryAtPath:sharedAVAssetCachePath() error:nil];
> +    for (NSString *itemName in directoryContents) {

Why declare the array to have "String *", but enumerate "NSString*"?

> Source/WebCore/platform/graphics/avfoundation/objc/MediaPlayerPrivateAVFoundationObjC.mm:493
> +        if (attributes == nil)

"if (!attributes)"

> Source/WebCore/platform/graphics/avfoundation/objc/MediaPlayerPrivateAVFoundationObjC.mm:500
> +        if (modificationDate == nil)

Ditto.

[Jon Lee]

Comment on attachment 274729 [details]
Patch

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

Please rebaseline since the patch doesn't apply.

> Source/WebCore/platform/graphics/avfoundation/objc/MediaPlayerPrivateAVFoundationObjC.mm:475
> +            [sharedAVAssetCache() removeEntryForKey:key];

What is the bug # that tracks testing proper deletion of these files?

> Source/WebCore/platform/graphics/avfoundation/objc/MediaPlayerPrivateAVFoundationObjC.mm:486
> +    NSArray<String *>* directoryContents = [fileManager contentsOfDirectoryAtPath:sharedAVAssetCachePath() error:nil];

NSArray<String *> *directoryContents

> Source/WebCore/platform/graphics/avfoundation/objc/MediaPlayerPrivateAVFoundationObjC.mm:488
> +        if (![itemName hasPrefix:@"CachedMedia-"])

Why test for this prefix? If something more recent has been loaded, presumably the plist would also have its modification timestamp updated as well.

> Source/WebCore/platform/graphics/avfoundation/objc/MediaPlayerPrivateAVFoundationObjC.mm:513
> +    for (NSString* key in [sharedAVAssetCache() allKeys]) {

NSString *key

[Jeremy Jones]

(In reply to comment #3)
> Comment on attachment 274729 [details]
> Patch
> 
> Might also be good to make this caching respect the
> Page::isResourceCachingDisabled, which is supposed to bypass caching for
> developer convenience. Ideally nothing gets found in the cache and also
> nothing gets written into it either when the flag is set.

I don't see Page::isResourceCachingDisabled. But we handle this case by setting AVURLAssetUsesNoPersistentCacheKey based on chrome client's mediaShouldUsePersistentCache().
 
I'll also make sure creating an AVAsset without a cache does not cause the singleton AVAssetCache to be created, to prevent the plist manifest from being written.

[Jeremy Jones]

(In reply to comment #4)
> Comment on attachment 274729 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=274729&action=review
> 
> > Source/WebCore/platform/graphics/avfoundation/objc/MediaPlayerPrivateAVFoundationObjC.mm:487
> > +    NSArray<String *>* directoryContents = [fileManager contentsOfDirectoryAtPath:sharedAVAssetCachePath() error:nil];
> > +    for (NSString *itemName in directoryContents) {
> 
> Why declare the array to have "String *", but enumerate "NSString*"?
> 

Oops. Fixed.

> > Source/WebCore/platform/graphics/avfoundation/objc/MediaPlayerPrivateAVFoundationObjC.mm:493
> > +        if (attributes == nil)
> 
> "if (!attributes)"

Fixed.

> 
> > Source/WebCore/platform/graphics/avfoundation/objc/MediaPlayerPrivateAVFoundationObjC.mm:500
> > +        if (modificationDate == nil)
> 
> Ditto.

Dito.

[Jeremy Jones]

(In reply to comment #5)
> Comment on attachment 274729 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=274729&action=review
> 
> Please rebaseline since the patch doesn't apply.

Will do.

> 
> > Source/WebCore/platform/graphics/avfoundation/objc/MediaPlayerPrivateAVFoundationObjC.mm:475
> > +            [sharedAVAssetCache() removeEntryForKey:key];
> 
> What is the bug # that tracks testing proper deletion of these files?
> 
> > Source/WebCore/platform/graphics/avfoundation/objc/MediaPlayerPrivateAVFoundationObjC.mm:486
> > +    NSArray<String *>* directoryContents = [fileManager contentsOfDirectoryAtPath:sharedAVAssetCachePath() error:nil];
> 
> NSArray<String *> *directoryContents

Fixed. Audited and fixed spacing for "*" for other obj-c declarations.
> 
> > Source/WebCore/platform/graphics/avfoundation/objc/MediaPlayerPrivateAVFoundationObjC.mm:488
> > +        if (![itemName hasPrefix:@"CachedMedia-"])
> 
> Why test for this prefix? If something more recent has been loaded,
> presumably the plist would also have its modification timestamp updated as
> well.

This check is to prevent the plist from being deleted, and to make sure we don't delete files we don't know about, especially when bounded by modifiedSince.

When completely clearing the cache, the entire directory is removed. See:
    if (modifiedSince == std::chrono::system_clock::time_point { }) {
        [fileManager removeItemAtPath:basePath error:nil];

> 
> > Source/WebCore/platform/graphics/avfoundation/objc/MediaPlayerPrivateAVFoundationObjC.mm:513
> > +    for (NSString* key in [sharedAVAssetCache() allKeys]) {
> 
> NSString *key

Fixed this and other.

[Jeremy Jones]

Created attachment 274770 [details]
Patch

[Jeremy Jones]

Created attachment 274772 [details]
Patch

[Jeremy Jones]

(In reply to comment #3)
> Comment on attachment 274729 [details]
> Patch
> 
> Might also be good to make this caching respect the
> Page::isResourceCachingDisabled, which is supposed to bypass caching for
> developer convenience. Ideally nothing gets found in the cache and also
> nothing gets written into it either when the flag is set.

Oh, isResourceCachingDisabled is new. I'll investigate using that.

[Anders Carlsson]

Comment on attachment 274772 [details]
Patch

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

Would it be possible to do this in the ui process, like we do for all other data types? 

It's unfortunate that we'd have to spin up a web process to do work (Which this patch doesn't do either, which means that emptying the cache only works if there's a web process running. It also means that if you have multiple web processes running they are all going to try to delete the same data).

> Source/WebKit2/WebProcess/WebProcess.cpp:1142
> +            websiteData.entries.append(WebsiteData::Entry { origin, WebsiteDataType::MemoryCache, 0 });

I don't think the type of these entries should be MemoryCache.

[Darin Adler]

Comment on attachment 274772 [details]
Patch

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

> Source/WebCore/html/HTMLMediaElement.h:386
> +    WEBCORE_EXPORT static void clearMediaCache(std::chrono::system_clock::time_point modifiedSince = std::chrono::system_clock::time_point { });

I believe this could just be:

    std::chrono::system_clock::time_point modifiedSince = { });

> Source/WebCore/platform/graphics/MediaPlayer.cpp:1043
> +        HashSet<RefPtr<SecurityOrigin>> engineOrigins = engine.originsInMediaCache();

Could use auto here instead.

> Source/WebCore/platform/graphics/MediaPlayer.cpp:1044
> +        origins.add(engineOrigins.begin(), engineOrigins.end());

Might want to add one special case:

    if (origins.empty())
        origins = WTFMove(engineOrigins);
    else
        origins.add(engineOrigins.begin(), engineOrigins.end());

One way to do that would be to make a helper function for adding one hash to another with that optimization.

> Source/WebCore/platform/graphics/MediaPlayerPrivate.h:217
> +    HashSet<RefPtr<SecurityOrigin>> originsInMediaCache() { return HashSet<RefPtr<SecurityOrigin>> { }; }

Can omit the type name:

    HashSet<RefPtr<SecurityOrigin>> originsInMediaCache() { return { }; }

> Source/WebCore/platform/graphics/avfoundation/objc/MediaPlayerPrivateAVFoundationObjC.mm:437
> +    char *pTMPDIR = getenv("TMPDIR");

In WebKit coding style this should be char* with out the space. Also, we almost never use this kind of "p" prefix.

> Source/WebCore/platform/graphics/avfoundation/objc/MediaPlayerPrivateAVFoundationObjC.mm:444
> +    static AVAssetCache *gAVAssetCache = NULL;

In WebKit coding style we don’t use a "g" for something like this. Also, for a static, there is no need to initialize to null. Also, since it’s an Objective-C object pointer it should be nil, not NULL. Also, nothing in the code relies on this being initialized to null.

> Source/WebCore/platform/graphics/avfoundation/objc/MediaPlayerPrivateAVFoundationObjC.mm:457
> +        origins.add(SecurityOrigin::create(URL(ParsedURLString, String::fromUTF8([key UTF8String]))));

It is not correct to use ParsedURLString here. That asserts that this is a URL that WebKIt would find valid, but we have no guarantee of that.

There is no value to converting the key to UTF-8 and then back to UTF-16.

Code should be more like this:

    for (...) {
        URL keyAsURL = URL(URL(), key);
        if (keyAsURL.isValid())
            origins.add(SecurityOrigin::create(keyAsURL);
    }

> Source/WebCore/platform/graphics/avfoundation/objc/MediaPlayerPrivateAVFoundationObjC.mm:486
> +    NSArray<NSString *> *directoryContents = [fileManager contentsOfDirectoryAtPath:sharedAVAssetCachePath() error:nil];

Should use basePath here.

The more modern contentsOfDirectoryAtURL:includingPropertiesForKeys:options: method lets you get the file names and also the file types and modification dates all at once.

I also suggest consider using enumeratorAtURL:includingPropertiesForKeys:options:errorHandler: instead, building up an array of files to delete rather than deleting inside the loop.

> Source/WebCore/platform/graphics/avfoundation/objc/MediaPlayerPrivateAVFoundationObjC.mm:501
> +        NSString *itemPath = [basePath stringByAppendingPathComponent:itemName];
> +        NSDictionary *attributes = [fileManager attributesOfItemAtPath:itemPath error:nil];
> +        if (!attributes)
> +            continue;
> +
> +        if (![NSFileTypeRegular isEqual:[attributes objectForKey:NSFileType]])
> +            continue;
> +
> +        NSDate *modificationDate = [attributes objectForKey:NSFileModificationDate];
> +        if (!modificationDate)
> +            continue;

I believe the more modern way to write this is with the NSURL property methods, which does exactly the I/O needed, and no extra I/O.

> Source/WebCore/platform/graphics/avfoundation/objc/MediaPlayerPrivateAVFoundationObjC.mm:514
> +        if (origins.contains(SecurityOrigin::create(URL(ParsedURLString, String::fromUTF8([key UTF8String])))))

Same comment here about this ParsedURLString and UTF-8 not being the correct way to do this.

[Jeremy Jones]

(In reply to comment #12)
> Comment on attachment 274772 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=274772&action=review
> 
> Would it be possible to do this in the ui process, like we do for all other
> data types? 

Difficult. The media cache is stored in the WebContent tmp dir, which is not accessible to the UIProcess.

Where to put it that both webprocess and uiprocess can access.

Candidates:
1) ApplicationCacheDirectory. This isn't where this sort of data is usually stored, but it is accessible from web process and uiprocess.
2) NetworkCacheDirectory. This is the right place for this sort of data, but isn't (and should not be made) available to WebProcess.
3) Create a new MediaCacheDirectory for just this purpose. Seems like the right solution, but seems unfortunate to create a new sandbox extension just for media.

Also: Moving the location will orphan the existing cache and will require it to be migrated.

I'm posting a change that does option (1) as an intermediate solution. And moves the media cache manipulation from WebProcess.cpp to WebsiteDataStore.cpp

> 
> It's unfortunate that we'd have to spin up a web process to do work (Which
> this patch doesn't do either, which means that emptying the cache only works
> if there's a web process running. It also means that if you have multiple
> web processes running they are all going to try to delete the same data).

I didn't catch this because the cases I tested were clearing multiple cache types and so, the processes were being spun-up anyway. But, I did see multiple delete attempts.

I see that would require a change to computeWebProcessAccessTypeForDataFetch.

> 
> > Source/WebKit2/WebProcess/WebProcess.cpp:1142
> > +            websiteData.entries.append(WebsiteData::Entry { origin, WebsiteDataType::MemoryCache, 0 });
> 
> I don't think the type of these entries should be MemoryCache.

Fixed.

[Jeremy Jones]

(In reply to comment #13)
> Comment on attachment 274772 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=274772&action=review
> 
> > Source/WebCore/html/HTMLMediaElement.h:386
> > +    WEBCORE_EXPORT static void clearMediaCache(std::chrono::system_clock::time_point modifiedSince = std::chrono::system_clock::time_point { });
> 
> I believe this could just be:
> 
>     std::chrono::system_clock::time_point modifiedSince = { });

Done.

> 
> > Source/WebCore/platform/graphics/MediaPlayer.cpp:1043
> > +        HashSet<RefPtr<SecurityOrigin>> engineOrigins = engine.originsInMediaCache();
> 
> Could use auto here instead.

Done.

> 
> > Source/WebCore/platform/graphics/MediaPlayer.cpp:1044
> > +        origins.add(engineOrigins.begin(), engineOrigins.end());
> 
> Might want to add one special case:
> 
>     if (origins.empty())
>         origins = WTFMove(engineOrigins);
>     else
>         origins.add(engineOrigins.begin(), engineOrigins.end());
> 
> One way to do that would be to make a helper function for adding one hash to
> another with that optimization.

Done. Added

template<typename T>
static void addToHash(HashSet<T>& toHash, HashSet<T>&& fromHash) {


> 
> > Source/WebCore/platform/graphics/MediaPlayerPrivate.h:217
> > +    HashSet<RefPtr<SecurityOrigin>> originsInMediaCache() { return HashSet<RefPtr<SecurityOrigin>> { }; }
> 
> Can omit the type name:
> 
>     HashSet<RefPtr<SecurityOrigin>> originsInMediaCache() { return { }; }

Done.

> 
> > Source/WebCore/platform/graphics/avfoundation/objc/MediaPlayerPrivateAVFoundationObjC.mm:437
> > +    char *pTMPDIR = getenv("TMPDIR");
> 
> In WebKit coding style this should be char* with out the space. Also, we
> almost never use this kind of "p" prefix.

Done.

> 
> > Source/WebCore/platform/graphics/avfoundation/objc/MediaPlayerPrivateAVFoundationObjC.mm:444
> > +    static AVAssetCache *gAVAssetCache = NULL;
> 
> In WebKit coding style we don’t use a "g" for something like this. Also, for
> a static, there is no need to initialize to null. Also, since it’s an
> Objective-C object pointer it should be nil, not NULL. Also, nothing in the
> code relies on this being initialized to null.

Done.

> 
> > Source/WebCore/platform/graphics/avfoundation/objc/MediaPlayerPrivateAVFoundationObjC.mm:457
> > +        origins.add(SecurityOrigin::create(URL(ParsedURLString, String::fromUTF8([key UTF8String]))));
> 
> It is not correct to use ParsedURLString here. That asserts that this is a
> URL that WebKIt would find valid, but we have no guarantee of that.
> 
> There is no value to converting the key to UTF-8 and then back to UTF-16.
> 
> Code should be more like this:
> 
>     for (...) {
>         URL keyAsURL = URL(URL(), key);
>         if (keyAsURL.isValid())
>             origins.add(SecurityOrigin::create(keyAsURL);
>     }

Done.

> 
> > Source/WebCore/platform/graphics/avfoundation/objc/MediaPlayerPrivateAVFoundationObjC.mm:486
> > +    NSArray<NSString *> *directoryContents = [fileManager contentsOfDirectoryAtPath:sharedAVAssetCachePath() error:nil];
> 
> Should use basePath here.

Done.

> 
> The more modern contentsOfDirectoryAtURL:includingPropertiesForKeys:options:
> method lets you get the file names and also the file types and modification
> dates all at once.
> 
> I also suggest consider using
> enumeratorAtURL:includingPropertiesForKeys:options:errorHandler: instead,
> building up an array of files to delete rather than deleting inside the loop.

Done.

> 
> > Source/WebCore/platform/graphics/avfoundation/objc/MediaPlayerPrivateAVFoundationObjC.mm:501
> > +        NSString *itemPath = [basePath stringByAppendingPathComponent:itemName];
> > +        NSDictionary *attributes = [fileManager attributesOfItemAtPath:itemPath error:nil];
> > +        if (!attributes)
> > +            continue;
> > +
> > +        if (![NSFileTypeRegular isEqual:[attributes objectForKey:NSFileType]])
> > +            continue;
> > +
> > +        NSDate *modificationDate = [attributes objectForKey:NSFileModificationDate];
> > +        if (!modificationDate)
> > +            continue;
> 
> I believe the more modern way to write this is with the NSURL property
> methods, which does exactly the I/O needed, and no extra I/O.

The NSDirectoryEnumerator provides access to the requested properties.

> 
> > Source/WebCore/platform/graphics/avfoundation/objc/MediaPlayerPrivateAVFoundationObjC.mm:514
> > +        if (origins.contains(SecurityOrigin::create(URL(ParsedURLString, String::fromUTF8([key UTF8String])))))
> 
> Same comment here about this ParsedURLString and UTF-8 not being the correct
> way to do this.

Done.

[Jeremy Jones]

Created attachment 274982 [details]
Patch

[Jeremy Jones]

(In reply to comment #3)
> Comment on attachment 274729 [details]
> Patch
> 
> Might also be good to make this caching respect the
> Page::isResourceCachingDisabled, which is supposed to bypass caching for
> developer convenience. Ideally nothing gets found in the cache and also
> nothing gets written into it either when the flag is set.

I've implemented this in the patch for https://bugs.webkit.org/show_bug.cgi?id=155924

[Jeremy Jones]

Created attachment 275706 [details]
Patch

[Eric Carlson]

Comment on attachment 275706 [details]
Patch

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

> Source/WebKit2/UIProcess/API/APIProcessPoolConfiguration.h:69
> +    void setMeidaCacheDirectory(const WTF::String& mediaCacheDirectory) { m_mediaCacheDirectory = mediaCacheDirectory; }

typo: setMeidaCacheDirectory -> setMediaCacheDirectory

> Source/WebKit2/UIProcess/API/APIWebsiteDataStore.cpp:123
> +    // FIXME: Implement.

FIXMEs should have a bug number (even if the existing FIXME just below doesn't have one :-/ ).

[Jon Lee]

Comment on attachment 275706 [details]
Patch

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

>> Source/WebKit2/UIProcess/API/APIWebsiteDataStore.cpp:123
>> +    // FIXME: Implement.
> 
> FIXMEs should have a bug number (even if the existing FIXME just below doesn't have one :-/ ).

All of the default directories here have the same useless comment.

[Jeremy Jones]

(In reply to comment #19)
> Comment on attachment 275706 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=275706&action=review
> 
> > Source/WebKit2/UIProcess/API/APIProcessPoolConfiguration.h:69
> > +    void setMeidaCacheDirectory(const WTF::String& mediaCacheDirectory) { m_mediaCacheDirectory = mediaCacheDirectory; }
> 
> typo: setMeidaCacheDirectory -> setMediaCacheDirectory

Done.

> 
> > Source/WebKit2/UIProcess/API/APIWebsiteDataStore.cpp:123
> > +    // FIXME: Implement.
> 
> FIXMEs should have a bug number (even if the existing FIXME just below
> doesn't have one :-/ ).

Removed useless comment.

[Jeremy Jones]

Created attachment 275799 [details]
Patch

[Jeremy Jones]

Created attachment 275900 [details]
Patch

[Jeremy Jones]

Created attachment 275945 [details]
Patch

[Jeremy Jones]

Created attachment 275950 [details]
Patch

[Darin Adler]

Comment on attachment 275950 [details]
Patch

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

> Source/WebCore/platform/graphics/MediaPlayer.cpp:1053
> +        auto engineOrigins = engine.originsInMediaCache(path);
> +        addToHash(origins, WTFMove(engineOrigins));

Reads better as a one-liner:

    addToHash(origins, engine.originsInMediaCache(path));

Obviates the need for WTFMove, even.

> Source/WebCore/platform/graphics/avfoundation/objc/MediaPlayerPrivateAVFoundationObjC.mm:486
> +    NSMutableArray<NSURL *> *urlsToDelete = [NSMutableArray array];

In the past we mostly did manual retain/release rather than autorelease. So we would have done [[NSMutableArray alloc] init] here and added a [urlsToDelete release] below. Or used RetainPtr<NSMutableArray<NSURL *>> and adoptNS.

> Source/WebCore/platform/graphics/avfoundation/objc/MediaPlayerPrivateAVFoundationObjC.mm:487
> +    while (NSURL *fileURL = [enumerator nextObject]) {

We could just write:

    for (NSURL *fileURL : enumerator) {

And use the fast enumeration syntax.

> Source/WebCore/platform/graphics/mac/MediaPlayerPrivateQTKit.mm:1344
> +    UNUSED_PARAM(path);

I suggest just leaving out the argument name rather than using UNUSED_PARAM in a case like this that unconditionally ignores an argument like this.

> Source/WebCore/platform/graphics/mac/MediaPlayerPrivateQTKit.mm:1348
> +        origins.add(SecurityOrigin::create(URL(ParsedURLString, String::fromUTF8([site UTF8String]))));

I’m not sure ParsedURLString is safe here. This should use the other URL constructor like the code above that checks for isValid.

> Source/WebCore/platform/graphics/mac/MediaPlayerPrivateQTKit.mm:1355
> +    UNUSED_PARAM(path);
> +    UNUSED_PARAM(modifiedSince);

Ditto.

> Source/WebCore/platform/graphics/mac/MediaPlayerPrivateQTKit.mm:1362
> +    UNUSED_PARAM(path);

Ditto.

[Jeremy Jones]

(In reply to comment #26)
> Comment on attachment 275950 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=275950&action=review
> 
> > Source/WebCore/platform/graphics/MediaPlayer.cpp:1053
> > +        auto engineOrigins = engine.originsInMediaCache(path);
> > +        addToHash(origins, WTFMove(engineOrigins));
> 
> Reads better as a one-liner:
> 
>     addToHash(origins, engine.originsInMediaCache(path));
> 
> Obviates the need for WTFMove, even.

Done.

> 
> > Source/WebCore/platform/graphics/avfoundation/objc/MediaPlayerPrivateAVFoundationObjC.mm:486
> > +    NSMutableArray<NSURL *> *urlsToDelete = [NSMutableArray array];
> 
> In the past we mostly did manual retain/release rather than autorelease. So
> we would have done [[NSMutableArray alloc] init] here and added a
> [urlsToDelete release] below. Or used RetainPtr<NSMutableArray<NSURL *>> and
> adoptNS.

Done. RetainPtr + adoptNS.

> 
> > Source/WebCore/platform/graphics/avfoundation/objc/MediaPlayerPrivateAVFoundationObjC.mm:487
> > +    while (NSURL *fileURL = [enumerator nextObject]) {
> 
> We could just write:
> 
>     for (NSURL *fileURL : enumerator) {
> 
> And use the fast enumeration syntax.
> 

Done

> > Source/WebCore/platform/graphics/mac/MediaPlayerPrivateQTKit.mm:1344
> > +    UNUSED_PARAM(path);
> 
> I suggest just leaving out the argument name rather than using UNUSED_PARAM
> in a case like this that unconditionally ignores an argument like this.
> 

Done.

> > Source/WebCore/platform/graphics/mac/MediaPlayerPrivateQTKit.mm:1348
> > +        origins.add(SecurityOrigin::create(URL(ParsedURLString, String::fromUTF8([site UTF8String]))));
> 
> I’m not sure ParsedURLString is safe here. This should use the other URL
> constructor like the code above that checks for isValid.
> 

Done.

> > Source/WebCore/platform/graphics/mac/MediaPlayerPrivateQTKit.mm:1355
> > +    UNUSED_PARAM(path);
> > +    UNUSED_PARAM(modifiedSince);
> 
> Ditto.

Ditto.

> 
> > Source/WebCore/platform/graphics/mac/MediaPlayerPrivateQTKit.mm:1362
> > +    UNUSED_PARAM(path);
> 
> Ditto.

Ditto.

[Jeremy Jones]

Created attachment 276170 [details]
Patch for landing.

[WebKit Commit Bot]

Comment on attachment 276170 [details]
Patch for landing.

Clearing flags on attachment: 276170

Committed r199315: <http://trac.webkit.org/changeset/199315>

[Jiewen Tan]

Rolled out in r199321: <http://trac.webkit.org/changeset/199321>.

See: https://build.webkit.org/builders/Apple%20Yosemite%20Release%20%28Build%29/builds/14196

[Jeremy Jones]

Created attachment 276191 [details]
Patch for landing.

[Darin Adler]

Comment on attachment 276191 [details]
Patch for landing.

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

> Source/WebCore/platform/graphics/avfoundation/objc/MediaPlayerPrivateAVFoundationObjC.mm:487
> +    for (NSURL *fileURL : enumerator) {

Oops, I totally meant this to be "in" rather than ":". Does it work with ":"?

[WebKit Commit Bot]

Comment on attachment 276191 [details]
Patch for landing.

Clearing flags on attachment: 276191

Committed r199326: <http://trac.webkit.org/changeset/199326>