Bug 129809 – [WebKit2][iOS] Main-frame custom representations

• Component: WebKit2
• Status: RESOLVED FIXED

[Tim Horton]

Bringing back code that was removed in r152841 and adapting it to iOS.

[Tim Horton]

Created attachment 226014 [details]
patch

[Simon Fraser (smfr)]

Comment on attachment 226014 [details]
patch

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
> +@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?

[Tim Horton]

Comment on attachment 226014 [details]
patch

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

>> Source/WebKit2/UIProcess/API/Cocoa/WKWebView.mm:103
>> +    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).

mitz thinks this doesn’t belong on WebPageProxy either. maybe the configuration? (andersca agrees)

> Source/WebKit2/UIProcess/Cocoa/CustomRepresentationRegistry.h:27
> +#ifndef CustomRepresentationRegistry_h
> +#define CustomRepresentationRegistry_h

don’t need these in objc headers

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

because it’s part of WKWebView. or the configuration. or whatever. it’s only used with the API

>> 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?

if this is internal, we should just assert or something (like allow the second one to win), if it becomes API, we should raise an exception

>> Source/WebKit2/UIProcess/Cocoa/WKCustomRepresentation.h:41
>> +@protocol WKCustomRepresentation <NSObject>
> 
> Representation of what? Would be nice if this was slightly more explanatory. Isn't it more like WKCustomContentPresentation?

need an underscore maybe

dan and anders suggest WKWebViewContentProvider

[Tim Horton]

Created attachment 226075 [details]
patch

new patch, moves things around (addressing review comments); enough has changed that this definitely needs a fresh review

[WebKit Commit Bot]

Attachment 226075 [details] did not pass style-queue:


ERROR: Source/WebKit2/UIProcess/API/Cocoa/WKWebViewConfigurationInternal.h:36:  Missing spaces around =  [whitespace/operators] [4]
Total errors found: 1 in 25 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Tim Horton]

Created attachment 226104 [details]
fix the mac build

[WebKit Commit Bot]

Attachment 226104 [details] did not pass style-queue:


ERROR: Source/WebKit2/UIProcess/API/Cocoa/WKWebViewConfigurationInternal.h:36:  Missing spaces around =  [whitespace/operators] [4]
Total errors found: 1 in 25 files


If any of these errors are false positives, please file a bug against check-webkit-style.

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

r=me. Ignore all my comments about naming. Pay attention to the other comments.

> Source/WebKit2/UIProcess/API/Cocoa/WKWebView.mm:50
>  #import "WKVisitedLinkProviderInternal.h"

V < W

> Source/WebKit2/UIProcess/API/Cocoa/WKWebView.mm:102
> +    RetainPtr<UIView <WKWebViewContentProvider>> _customContentProviderView;

Probably don’t need “provider” in this variable’s name.

> Source/WebKit2/UIProcess/API/Cocoa/WKWebView.mm:336
> +- (void)_setPageHasCustomContentProvider:(BOOL)pageHasCustomContentProvider loadedMIMEType:(const WTF::String&)mimeType

-_setHasCustomContentView:loadedMIMEType: (not quite sure why the word “loaded” is needed)

> Source/WebKit2/UIProcess/API/Cocoa/WKWebView.mm:343
> +        _customContentProviderView = adoptNS([[representationClass alloc] initWithFrame:CGRectZero]);

Can just call -init if the frame is CGRectZero.

> Source/WebKit2/UIProcess/API/Cocoa/WKWebView.mm:512
> +- (BOOL)hasContentView

-hasNormalContentView? -hasStandardContentView? Better yet, -usesStandardContentView, since we seem to always “have” _contentView even when it’s not parented.

> Source/WebKit2/UIProcess/API/Cocoa/WKWebViewConfigurationInternal.h:36
> +@property (nonatomic, setter=_setContentProviderRegistry:) WKWebViewContentProviderRegistry* _contentProviderRegistry;

Misplaced star.

> Source/WebKit2/UIProcess/Cocoa/WKWebViewContentProviderRegistry.h:32
> +#import <objc/objc.h>

What’s this for? Class?

> Source/WebKit2/UIProcess/Cocoa/WKWebViewContentProviderRegistry.h:47
> +- (void)registerContentProvider:(Class <WKWebViewContentProvider>)contentProvider forMIMEType:(const String&)mimeType;
> +- (Class <WKWebViewContentProvider>)contentProviderForMIMEType:(const String&)mimeType;

Can drop “content” from these names.

> Source/WebKit2/UIProcess/Cocoa/WKWebViewContentProviderRegistry.mm:34
> +#import "WebPageProxy.h"
> +#import "WKWebViewInternal.h"

K < e

> Source/WebKit2/UIProcess/Cocoa/WKWebViewContentProviderRegistry.mm:46
> +@interface WKWebViewContentProviderRegistry () {
> +    HashMap<String, Class <WKWebViewContentProvider>, CaseFoldingHash> _contentProviderForMIMEType;
> +    HashCountedSet<WebPageProxy*> _pages;
> +}
> +@end

ivars normally go in the @implementation, not a class extension. If you do that, you don’t even need the extension in this case.

> Source/WebKit2/UIProcess/Cocoa/WKWebViewContentProviderRegistry.mm:52
> +    _pages.add(&page);

Should probably assert that the page isn’t already in the set.

> Source/WebKit2/UIProcess/WebPageProxy.cpp:2280
> +        m_pageClient.didCommitLoadForMainFrame(mimeType, frameHasCustomContentProvider);

Is it OK to send this a second time? Why is this needed?

> Source/WebKit2/UIProcess/ios/PageClientImplIOS.mm:44
>  #import <WebCore/SharedBuffer.h>
>  
> +#import "WKWebViewInternal.h"

" < <

> Source/WebKit2/UIProcess/ios/PageClientImplIOS.mm:411
> +    RetainPtr<NSData> data = adoptNS([[NSData alloc] initWithBytes:dataReference.data() length:dataReference.size()]);

Is this the best way to get NSData from a DataReference?

> Source/WebKit2/WebProcess/WebCoreSupport/WebFrameLoaderClient.cpp:116
> +bool WebFrameLoaderClient::hasHTMLView() const
> +{
> +    return !m_frameHasCustomContentProvider;
> +}

This makes it obvious that the member variable (and many other things) should be about a custom content view, not a custom content provider.

> Source/WebKit2/WebProcess/WebCoreSupport/WebFrameLoaderClient.cpp:902
>      }
>  
> -    m_pluginView->manualLoadDidFinishLoading();
> -    m_pluginView = 0;
> -    m_hasSentResponseToPluginView = false;
> +    if (m_pluginView) {

Can’t this just be } else {, or return } ?

> Source/WebKit2/WebProcess/WebCoreSupport/WebFrameLoaderClient.cpp:1190
> +    WebPage* webPage = m_frame->page();
> +    bool isMainFrame = webPage->mainWebFrame() == m_frame;

Still no better way to check from mainframe-ness?

> Source/WebKit2/WebProcess/WebPage/WebPage.cpp:3842
> +    if (!m_mimeTypesWithCustomContentProviders.contains(response.mimeType()))
> +        return false;
> +
> +    // If a plug-in exists that claims to support this response, it should take precedence over the custom content provider.
> +    return !canPluginHandleResponse(response);

This can be written as one line (+ comment):
return m_mimeTypesWithCustomContentProviders.contains(response.mimeType()) && !canPluginHandleResponse(response).

> Source/WebKit2/WebProcess/WebPage/WebPage.h:629
> +    void addMIMETypeWithCustomContentProvider(const String& mimeType);

Can drop the variable name here.

[Tim Horton]

http://trac.webkit.org/changeset/165303