Bug 138447 – Clients should be able to customize the view used for page previews

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKit2

[Conrad Shultz]

Clients should be able to customize the view used for page previews

[Radar WebKit Bug Importer]

<rdar://problem/18889231>

[Conrad Shultz]

Created attachment 241074 [details]
Patch

[Anders Carlsson]

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?

[Conrad Shultz]

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

[Anders Carlsson]

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.

[Conrad Shultz]

(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.

[Conrad Shultz]

Created attachment 241113 [details]
Patch

[Anders Carlsson]

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).

[Conrad Shultz]

(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.

[Conrad Shultz]

Created attachment 241147 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 241147 [details]
Patch

Clearing flags on attachment: 241147

Committed r175732: <http://trac.webkit.org/changeset/175732>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.