Bug 138370 – Use a web view for Quick Look in action menus

• Component: WebKit2
• Status: RESOLVED FIXED

[Conrad Shultz]

WebKit should manage its own view for the Quick Look menu item.

[Radar WebKit Bug Importer]

<rdar://problem/18871842>

[Conrad Shultz]

Created attachment 240948 [details]
Patch

[Anders Carlsson]

Comment on attachment 240948 [details]
Patch

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

> Source/WebKit2/UIProcess/mac/WKActionMenuController.mm:70
> +@interface _WKPagePreviewViewController : NSViewController {

This does not be prefixed by an underscore since it's an internal class.

[Tim Horton]

Comment on attachment 240948 [details]
Patch

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

> Source/WebKit2/UIProcess/mac/WKActionMenuController.mm:71
> +    @private;

don't think that semicolon belongs, does it?

> Source/WebKit2/UIProcess/mac/WKActionMenuController.mm:91
> +    _preferredSize = NSMakeSize(320, 568);

Where do these crazy numbers come from!

> Source/WebKit2/UIProcess/mac/WKActionMenuController.mm:223
> +    NSPopover *popover = [[NSPopover alloc] init];

is the memory management of popover correct?

[Conrad Shultz]

(In reply to comment #3)
> Comment on attachment 240948 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=240948&action=review
> 
> > Source/WebKit2/UIProcess/mac/WKActionMenuController.mm:70
> > +@interface _WKPagePreviewViewController : NSViewController {
> 
> This does not be prefixed by an underscore since it's an internal class.

Removed.

[Conrad Shultz]

(In reply to comment #4)
> Comment on attachment 240948 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=240948&action=review
> 
> > Source/WebKit2/UIProcess/mac/WKActionMenuController.mm:71
> > +    @private;
> 
> don't think that semicolon belongs, does it?

Nope, not sure how that snuck in.

> 
> > Source/WebKit2/UIProcess/mac/WKActionMenuController.mm:91
> > +    _preferredSize = NSMakeSize(320, 568);
> 
> Where do these crazy numbers come from!

"somewhat arbitrary" = "iPhone 5 screen size"; I'll add that to the ChangeLog.

> 
> > Source/WebKit2/UIProcess/mac/WKActionMenuController.mm:223
> > +    NSPopover *popover = [[NSPopover alloc] init];
> 
> is the memory management of popover correct?

No, that should use RetainPtr too.

New patch coming.

[Conrad Shultz]

Created attachment 240959 [details]
Patch

[Conrad Shultz]

Created attachment 240961 [details]
Patch

[Anders Carlsson]

Comment on attachment 240961 [details]
Patch

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

> Source/WebKit2/UIProcess/mac/WKActionMenuController.mm:71
> +    @private

I don't think this needs to be indented.

> Source/WebKit2/UIProcess/mac/WKActionMenuController.mm:77
> +@property (nonatomic, readonly) NSURL *url;
> +@property (nonatomic) NSSize preferredSize;

I think these can just be ivars, they don't need to be properties.

[Conrad Shultz]

(In reply to comment #9)
> Comment on attachment 240961 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=240961&action=review
> 
> > Source/WebKit2/UIProcess/mac/WKActionMenuController.mm:71
> > +    @private
> 
> I don't think this needs to be indented.

Fixed.

> 
> > Source/WebKit2/UIProcess/mac/WKActionMenuController.mm:77
> > +@property (nonatomic, readonly) NSURL *url;
> > +@property (nonatomic) NSSize preferredSize;
> 
> I think these can just be ivars, they don't need to be properties.

It feels really strange to me to prefer direct ivar access, but I'll post a new version doing so.

[Conrad Shultz]

Created attachment 240971 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 240971 [details]
Patch

Clearing flags on attachment: 240971

Committed r175589: <http://trac.webkit.org/changeset/175589>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Tim Horton]

Build fix in http://trac.webkit.org/changeset/175591