[webkit-reviews] review granted: [Bug 129809] [WebKit2][iOS] Main-frame custom representations : [Attachment 226014] patch

[bugzilla-daemon at webkit.org]

Simon Fraser (smfr) <simon.fraser at apple.com> has granted Tim Horton
<thorton at apple.com>'s request for review:
Bug 129809: [WebKit2][iOS] Main-frame custom representations
https://bugs.webkit.org/show_bug.cgi?id=129809

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

------- Additional Comments from Simon Fraser (smfr) <simon.fraser at apple.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=226014&action=review


> Source/WebKit2/ChangeLog:9
> +	   Re-introduce custom representations to WebKit2 (removed in r152841),
but
> +	   for iOS this time.

s/representation/presentation everywhere?

> Source/WebKit2/UIProcess/API/Cocoa/WKWebView.mm:103
> +    std::unique_ptr<WebKit::CustomRepresentationRegistry>
_customRepresentationRegistry;
> +    RetainPtr<UIView <WKCustomRepresentation>> _customRepresentationView;

It's a bit odd that this registry lives on the WKWebView. I'd expect it to live
on WebPageProxy (but then we'd need C++ wrappers around representation
providers, though that isn't terrible).

> Source/WebKit2/UIProcess/API/Cocoa/WKWebView.mm:336
> +	   Class representationClass =
_customRepresentationRegistry->customRepresentationForMIMEType(mimeType);

What if there is none?

> Source/WebKit2/UIProcess/API/Cocoa/WKWebView.mm:340
> +	   [_scrollView addSubview:_customRepresentationView.get()];
> +	   [_contentView removeFromSuperview];

The ordering is surprising.

> Source/WebKit2/UIProcess/API/Cocoa/WKWebViewInternal.h:71
> +-
(void)_didFinishLoadingDataForCustomRepresentationWithSuggestedFilename:(const
WTF::String&)suggestedFilename dataReference:(const
IPC::DataReference&)dataReference;

Would be slightly nicer to send in an NSData* I think.

> Source/WebKit2/UIProcess/Cocoa/CustomRepresentationRegistry.h:31
> +#if WK_API_ENABLED

Why?

> Source/WebKit2/UIProcess/Cocoa/CustomRepresentationRegistry.mm:29
> +#if WK_API_ENABLED

?

> Source/WebKit2/UIProcess/Cocoa/CustomRepresentationRegistry.mm:50
> +	   WTFLogAlways("Cannot register two custom representations for the
same MIME type.\n");

Why not just allow the second one to win?

> Source/WebKit2/UIProcess/Cocoa/WKCustomRepresentation.h:41
> + at protocol WKCustomRepresentation <NSObject>

Representation of what? Would be nice if this was slightly more explanatory.
Isn't it more like WKCustomContentPresentation?

> Source/WebKit2/WebProcess/WebCoreSupport/WebFrameLoaderClient.cpp:914
> +	   m_pluginView = 0;

nullptr?