Bug 155783 - When clearing cache, also clear AVFoundation cache.
Summary: When clearing cache, also clear AVFoundation cache.
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit2 (show other bugs)
Version: WebKit Nightly Build
Hardware: Mac Unspecified
: P2 Normal
Assignee: Jeremy Jones
URL:
Keywords: InRadar
Depends on: 156482
Blocks:
  Show dependency treegraph
 
Reported: 2016-03-22 23:06 PDT by Jeremy Jones
Modified: 2017-11-02 11:33 PDT (History)
4 users (show)

See Also:


Attachments
Patch (27.11 KB, patch)
2016-03-22 23:45 PDT, Jeremy Jones
no flags Details | Formatted Diff | Diff
Patch (27.47 KB, patch)
2016-03-23 13:09 PDT, Jeremy Jones
no flags Details | Formatted Diff | Diff
Patch (27.46 KB, patch)
2016-03-23 13:17 PDT, Jeremy Jones
no flags Details | Formatted Diff | Diff
Patch (34.13 KB, patch)
2016-03-26 00:41 PDT, Jeremy Jones
no flags Details | Formatted Diff | Diff
Patch (48.96 KB, patch)
2016-04-05 15:54 PDT, Jeremy Jones
no flags Details | Formatted Diff | Diff
Patch (48.94 KB, patch)
2016-04-06 11:55 PDT, Jeremy Jones
no flags Details | Formatted Diff | Diff
Patch (52.93 KB, patch)
2016-04-07 11:19 PDT, Jeremy Jones
no flags Details | Formatted Diff | Diff
Patch (52.99 KB, patch)
2016-04-07 15:14 PDT, Jeremy Jones
no flags Details | Formatted Diff | Diff
Patch (54.81 KB, patch)
2016-04-07 15:56 PDT, Jeremy Jones
darin: review+
Details | Formatted Diff | Diff
Patch for landing. (54.59 KB, patch)
2016-04-11 13:27 PDT, Jeremy Jones
no flags Details | Formatted Diff | Diff
Patch for landing. (54.57 KB, patch)
2016-04-11 17:11 PDT, Jeremy Jones
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Jeremy Jones 2016-03-22 23:06:13 PDT
When clearing cache, also clear AVFoundation cache.
Comment 1 Jeremy Jones 2016-03-22 23:45:29 PDT
Created attachment 274729 [details]
Patch
Comment 2 Jeremy Jones 2016-03-22 23:47:04 PDT
rdar://problem/25252541
Comment 3 Darin Adler 2016-03-23 08:44:51 PDT
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.
Comment 4 Eric Carlson 2016-03-23 08:59:01 PDT
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.
Comment 5 Jon Lee 2016-03-23 12:28:37 PDT
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
Comment 6 Jeremy Jones 2016-03-23 12:50:12 PDT
(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.
Comment 7 Jeremy Jones 2016-03-23 12:50:51 PDT
(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.
Comment 8 Jeremy Jones 2016-03-23 12:56:08 PDT
(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.
Comment 9 Jeremy Jones 2016-03-23 13:09:17 PDT
Created attachment 274770 [details]
Patch
Comment 10 Jeremy Jones 2016-03-23 13:17:23 PDT
Created attachment 274772 [details]
Patch
Comment 11 Jeremy Jones 2016-03-23 13:35:14 PDT
(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.
Comment 12 Anders Carlsson 2016-03-24 07:32:41 PDT
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.
Comment 13 Darin Adler 2016-03-25 06:45:59 PDT
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.
Comment 14 Jeremy Jones 2016-03-25 20:34:19 PDT
(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.
Comment 15 Jeremy Jones 2016-03-25 20:42:43 PDT
(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.
Comment 16 Jeremy Jones 2016-03-26 00:41:47 PDT
Created attachment 274982 [details]
Patch
Comment 17 Jeremy Jones 2016-03-26 15:59:27 PDT
(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
Comment 18 Jeremy Jones 2016-04-05 15:54:16 PDT
Created attachment 275706 [details]
Patch
Comment 19 Eric Carlson 2016-04-05 19:05:34 PDT
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 :-/ ).
Comment 20 Jon Lee 2016-04-05 19:18:18 PDT
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.
Comment 21 Jeremy Jones 2016-04-06 11:52:55 PDT
(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.
Comment 22 Jeremy Jones 2016-04-06 11:55:34 PDT
Created attachment 275799 [details]
Patch
Comment 23 Jeremy Jones 2016-04-07 11:19:51 PDT
Created attachment 275900 [details]
Patch
Comment 24 Jeremy Jones 2016-04-07 15:14:52 PDT
Created attachment 275945 [details]
Patch
Comment 25 Jeremy Jones 2016-04-07 15:56:41 PDT
Created attachment 275950 [details]
Patch
Comment 26 Darin Adler 2016-04-10 17:32:47 PDT
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.
Comment 27 Jeremy Jones 2016-04-11 11:34:39 PDT
(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.
Comment 28 Jeremy Jones 2016-04-11 13:27:14 PDT
Created attachment 276170 [details]
Patch for landing.
Comment 29 WebKit Commit Bot 2016-04-11 15:43:39 PDT
Comment on attachment 276170 [details]
Patch for landing.

Clearing flags on attachment: 276170

Committed r199315: <http://trac.webkit.org/changeset/199315>
Comment 31 Jeremy Jones 2016-04-11 17:11:42 PDT
Created attachment 276191 [details]
Patch for landing.
Comment 32 Darin Adler 2016-04-11 17:22:11 PDT
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 ":"?
Comment 33 WebKit Commit Bot 2016-04-11 18:00:03 PDT
Comment on attachment 276191 [details]
Patch for landing.

Clearing flags on attachment: 276191

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