[webkit-reviews] review denied: [Bug 85300] Support for web content filter delegate for filtering https content : [Attachment 139656] Patch

[bugzilla-daemon at webkit.org]

Alexey Proskuryakov <ap at webkit.org> has denied Jeffrey Pfau
<jeffrey at endrift.com>'s request for review:
Bug 85300: Support for web content filter delegate for filtering https content
https://bugs.webkit.org/show_bug.cgi?id=85300

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

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


> Source/WebCore/ChangeLog:21
> +	   * platform/mac/WebCoreSystemInterface.h:
> +	   * platform/mac/WebCoreSystemInterface.mm:

You also need to update headers and binaries in WebKitLibraries.

> Source/WebCore/loader/MainResourceLoader.cpp:501
> +	   if (wkFilterWasBlocked(filter))
> +	       didFinishLoading(0);

I don't think that this is right. DidFinishLoading is called when network layer
finishes, not when you want to cancel a ResourceHandle.

> Source/WebCore/loader/MainResourceLoader.cpp:502
> +	   wkFilterRelease(filter);

I don't understand why the filter needs to be released here.

> Source/WebCore/loader/MainResourceLoader.cpp:517
> +	       didReceiveData(data, length, -1, false);

DidReceiveData can do anything, including getting MainResourceLoader indirectly
deleted. You need to at least protect the loader with a RefPtr.

> Source/WebCore/loader/MainResourceLoader.h:38
> +#include "WebCoreSystemInterface.h"

No need to include, a forward declaration would work better.