Bug 138447 - Clients should be able to customize the view used for page previews
Summary: Clients should be able to customize the view used for page previews
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit2 (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Conrad Shultz
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2014-11-05 17:08 PST by Conrad Shultz
Modified: 2014-11-06 18:02 PST (History)
6 users (show)

See Also:


Attachments
Patch (5.94 KB, patch)
2014-11-05 17:18 PST, Conrad Shultz
no flags Details | Formatted Diff | Diff
Patch (5.90 KB, patch)
2014-11-06 09:11 PST, Conrad Shultz
no flags Details | Formatted Diff | Diff
Patch (5.97 KB, patch)
2014-11-06 17:00 PST, Conrad Shultz
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Conrad Shultz 2014-11-05 17:08:27 PST
Clients should be able to customize the view used for page previews
Comment 1 Radar WebKit Bug Importer 2014-11-05 17:09:15 PST
<rdar://problem/18889231>
Comment 2 Conrad Shultz 2014-11-05 17:18:21 PST
Created attachment 241074 [details]
Patch
Comment 3 Anders Carlsson 2014-11-05 17:33:04 PST
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?
Comment 4 Conrad Shultz 2014-11-05 17:34:40 PST
(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 5 Anders Carlsson 2014-11-05 17:39:33 PST
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.
Comment 6 Conrad Shultz 2014-11-05 17:45:03 PST
(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.
Comment 7 Conrad Shultz 2014-11-06 09:11:19 PST
Created attachment 241113 [details]
Patch
Comment 8 Anders Carlsson 2014-11-06 13:17:02 PST
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).
Comment 9 Conrad Shultz 2014-11-06 15:50:34 PST
(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.
Comment 10 Conrad Shultz 2014-11-06 17:00:24 PST
Created attachment 241147 [details]
Patch
Comment 11 WebKit Commit Bot 2014-11-06 18:02:03 PST
Comment on attachment 241147 [details]
Patch

Clearing flags on attachment: 241147

Committed r175732: <http://trac.webkit.org/changeset/175732>
Comment 12 WebKit Commit Bot 2014-11-06 18:02:07 PST
All reviewed patches have been landed.  Closing bug.