Summary: | Clients should be able to customize the view used for page previews | ||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Conrad Shultz <conrad_shultz> | ||||||||
Component: | WebKit2 | Assignee: | Conrad Shultz <conrad_shultz> | ||||||||
Status: | RESOLVED FIXED | ||||||||||
Severity: | Normal | CC: | andersca, bdakin, commit-queue, conrad_shultz, thorton, webkit-bug-importer | ||||||||
Priority: | P2 | Keywords: | InRadar | ||||||||
Version: | 528+ (Nightly build) | ||||||||||
Hardware: | Unspecified | ||||||||||
OS: | Unspecified | ||||||||||
Attachments: |
|
Description
Conrad Shultz
2014-11-05 17:08:27 PST
Created attachment 241074 [details]
Patch
Comment on attachment 241074 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=241074&action=review > Source/WebKit2/UIProcess/API/Cocoa/WKViewPrivate.h:124 > +- (NSView *)_previewViewForURL:(NSURL *)url frame:(NSRect)frame; This sounds kind of ambiguous, as both a verb and a noun. I can't think of a better name though. Can it return a WKView? (In reply to comment #3) > Comment on attachment 241074 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=241074&action=review > > > Source/WebKit2/UIProcess/API/Cocoa/WKViewPrivate.h:124 > > +- (NSView *)_previewViewForURL:(NSURL *)url frame:(NSRect)frame; > > This sounds kind of ambiguous, as both a verb and a noun. I can't think of a > better name though. > Can it return a WKView? I'd rather not constrain it as such, especially since the internal implementation actually uses WKWebView. I initially had -_viewForPreviewingURL:frame:, but thought this read better. Perhaps I was wrong? Comment on attachment 241074 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=241074&action=review > Source/WebKit2/UIProcess/mac/WKActionMenuController.mm:112 > + RetainPtr<NSView> previewView; > + NSRect defaultFrame = NSMakeRect(0, 0, _mainViewSize.width, _mainViewSize.height); I'd move these around, put the initial assignment here and then have the check below be if (!previewView) { ... } > Source/WebKit2/UIProcess/mac/WKActionMenuController.mm:819 > +- (RetainPtr<NSView>)pagePreviewViewController:(WKPagePreviewViewController *)pagePreviewViewController previewViewForURL:(NSURL *)url frame:(NSRect)frame I don't think this needs to return a RetainPtr. (In reply to comment #5) > Comment on attachment 241074 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=241074&action=review > > > Source/WebKit2/UIProcess/mac/WKActionMenuController.mm:112 > > + RetainPtr<NSView> previewView; > > + NSRect defaultFrame = NSMakeRect(0, 0, _mainViewSize.width, _mainViewSize.height); > > I'd move these around, put the initial assignment here and then have the > check below be > > if (!previewView) { > ... > } > Agreed, will fix. > > Source/WebKit2/UIProcess/mac/WKActionMenuController.mm:819 > > +- (RetainPtr<NSView>)pagePreviewViewController:(WKPagePreviewViewController *)pagePreviewViewController previewViewForURL:(NSURL *)url frame:(NSRect)frame > > I don't think this needs to return a RetainPtr. I don't think so either. Created attachment 241113 [details]
Patch
Comment on attachment 241113 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=241113&action=review > Source/WebKit2/UIProcess/API/Cocoa/WKViewPrivate.h:124 > +- (NSView *)_viewForPreviewingURL:(NSURL *)url frame:(NSRect)frame; I think this should be called initialFrameSize (and take an NSSize). (In reply to comment #8) > Comment on attachment 241113 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=241113&action=review > > > Source/WebKit2/UIProcess/API/Cocoa/WKViewPrivate.h:124 > > +- (NSView *)_viewForPreviewingURL:(NSURL *)url frame:(NSRect)frame; > > I think this should be called initialFrameSize (and take an NSSize). Agreed; new version coming. Created attachment 241147 [details]
Patch
Comment on attachment 241147 [details] Patch Clearing flags on attachment: 241147 Committed r175732: <http://trac.webkit.org/changeset/175732> All reviewed patches have been landed. Closing bug. |