[webkit-reviews] review denied: [Bug 220525] [Cocoa] Network extension sandbox extensions are sometimes issued too late : [Attachment 417412] Patch

[bugzilla-daemon at webkit.org]

Brent Fulgham <bfulgham at webkit.org> has denied Per Arne Vollan
<pvollan at apple.com>'s request for review:
Bug 220525: [Cocoa] Network extension sandbox extensions are sometimes issued
too late
https://bugs.webkit.org/show_bug.cgi?id=220525

Attachment 417412: Patch

https://bugs.webkit.org/attachment.cgi?id=417412&action=review




--- Comment #10 from Brent Fulgham <bfulgham at webkit.org> ---
Comment on attachment 417412
  --> https://bugs.webkit.org/attachment.cgi?id=417412
Patch

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

I think this is good, but would be improved by refactoring slightly.

> Source/WebKit/UIProcess/WebPageProxy.cpp:302
> +#endif

You won't need this Cocoa-specific include if you leave the implementation in
WebPageProxyCocoa.mm and use a stub in WebPageProxy.cpp for non-Cocoa clients.

> Source/WebKit/UIProcess/WebPageProxy.cpp:5018
> +static SandboxExtension::HandleArray
createNetworkExtensionsSandboxExtensions(WebProcessProxy& process)

I think this would be cleaner if this method stayed in WebPageProxyCocoa.mm,
with a stub in WebPageProxy.cpp for non-Cocoa cases.

> Source/WebKit/UIProcess/Cocoa/WebPageProxyCocoa.mm:156
>  

Then you could just move the below code into a method body instead of deleting
it here.

> Source/WebKit/WebProcess/WebPage/WebPage.cpp:346
> +#endif

You could avoid adding this cocoa-specific header here, if you moved the
Sandbox consumption to WebPageCocoa.mm, which already knows about content
filtering and sandbox stuff.

> Source/WebKit/WebProcess/WebPage/WebPage.cpp:3375
> +#endif

I think this would be cleaner with a new method stub in this file called
"consumeSandboxExtensions" that is a no-op for non-Cocoa builds. Then the real
implementation could live in WebPageCocoa.mm, which already knows about all the
important things and no new headers need to be added anywhere.

> Source/WebKit/WebProcess/WebPage/Cocoa/WebPageCocoa.mm:-90
> -#endif

... and this could just be moved to a new method (well, I guess the
conditionals wouldn't be needed anymore in the new code path).