[webkit-reviews] review denied: [Bug 113422] Mem mapped resource data improvements : [Attachment 195353] Patch v1

[bugzilla-daemon at webkit.org]

Alexey Proskuryakov <ap at webkit.org> has denied Brady Eidson
<beidson at apple.com>'s request for review:
Bug 113422: Mem mapped resource data improvements
https://bugs.webkit.org/show_bug.cgi?id=113422

Attachment 195353: Patch v1
https://bugs.webkit.org/attachment.cgi?id=195353&action=review

------- Additional Comments from Alexey Proskuryakov <ap at webkit.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=195353&action=review


I think that I'd like to see a version of this patch with threading cleaned up
a bit more. I'm not sure if I can fully understand it now.

> Source/WebKit2/NetworkProcess/NetworkResourceLoader.cpp:-211
> -    ASSERT(!isMainThread());

Should the condition be just reverted instead of removing the check?

> Source/WebKit2/NetworkProcess/mac/DiskCachingMonitor.h:30
> +#include "NetworkResourceLoader.h"

Seems unneeded (and you already have a forward declaration in place).

> Source/WebKit2/NetworkProcess/mac/DiskCachingMonitor.mm:36
> +
> +
> +SOFT_LINK_FRAMEWORK(CFNetwork)

Seems like an abuse of the macro if you are only doing this to get a
CFNetworkLibrary() function.

But then, you'll probably be deleting the dynamic loading code soon.

> Source/WebKit2/NetworkProcess/mac/DiskCachingMonitor.mm:52
> +static const double DiskCachingMonitorTimeout = 10;

I don;t think that starting constants with upper case is WebKit style.

> Source/WebKit2/NetworkProcess/mac/DiskCachingMonitor.mm:56
> +    static uint64_t currentID = 1;

Maybe ASSERT(isMainThread())?

> Source/WebKit2/NetworkProcess/mac/DiskCachingMonitor.mm:64
> +    DEFINE_STATIC_LOCAL(DiskCachingMonitorMap, map, ());

Maybe ASSERT(!diskCachingMonitorMapMutex().tryLock())?

> Source/WebKit2/NetworkProcess/mac/DiskCachingMonitor.mm:81
> +    OwnPtr<DiskCachingMonitor> request = adoptPtr(new
DiskCachingMonitor(cachedResponse, loader));

We usually hide the constructor and expose a create function for this.

> Source/WebKit2/NetworkProcess/mac/DiskCachingMonitor.mm:93
> +    // Step 1: Set up this request's data members on the main thread.
> +    dispatch_async(dispatch_get_main_queue(), ^{

If it's dispatch_async, then it's not step 1, it's step "some time".

Also, I don't understand whether this constructor is only called on main thread
anyway. Calling generateUniqueDiskCachingMonitorID() seems to require that, and
all client code is main thread now.

> Source/WebKit2/NetworkProcess/mac/DiskCachingMonitor.mm:98
> +    // Step2: Set up the disk caching callback so this request can be
destroyed upon successfull disk caching.

No space in "Step2".

> Source/WebKit2/NetworkProcess/mac/NetworkResourceLoaderMac.mm:83
> +    DEFINE_STATIC_LOCAL(RetainPtr<CFURLCacheRef>, cache, (AdoptCF,
CFURLCacheCopySharedURLCache()));
> +    if (!cache)
> +	   return;

Don't we change the cache when enabling private browsing? I'm also unsure how
this will work with cache partitioning.

> Source/WebKit2/NetworkProcess/mac/NetworkResourceLoaderMac.mm:99
> +void NetworkResourceLoader::willCacheResponseAsync(WebCore::ResourceHandle*,
NSCachedURLResponse* nsResponse)

Please no WebCore:: prefix. This also has a misplaced star.