Summary: | [WebKit2][iOS] Main-frame custom representations | ||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Tim Horton <thorton> | ||||||||
Component: | WebKit2 | Assignee: | Tim Horton <thorton> | ||||||||
Status: | RESOLVED FIXED | ||||||||||
Severity: | Normal | CC: | andersca, commit-queue, mitz, sam, simon.fraser | ||||||||
Priority: | P2 | ||||||||||
Version: | 528+ (Nightly build) | ||||||||||
Hardware: | Unspecified | ||||||||||
OS: | Unspecified | ||||||||||
Bug Depends on: | |||||||||||
Bug Blocks: | 129600 | ||||||||||
Attachments: |
|
Description
Tim Horton
2014-03-06 11:07:52 PST
Created attachment 226014 [details]
patch
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? 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 Created attachment 226075 [details]
patch
new patch, moves things around (addressing review comments); enough has changed that this definitely needs a fresh review
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.
Created attachment 226104 [details]
fix the mac build
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. |