Summary: | Use a web view for Quick Look in action menus | ||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|
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, mitz, sam, thorton, webkit-bug-importer | ||||||||||
Priority: | P2 | Keywords: | InRadar | ||||||||||
Version: | 528+ (Nightly build) | ||||||||||||
Hardware: | Unspecified | ||||||||||||
OS: | Unspecified | ||||||||||||
Attachments: |
|
Description
Conrad Shultz
2014-11-04 13:50:47 PST
Created attachment 240948 [details]
Patch
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. 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? (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. (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. Created attachment 240959 [details]
Patch
Created attachment 240961 [details]
Patch
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. (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. Created attachment 240971 [details]
Patch
Comment on attachment 240971 [details] Patch Clearing flags on attachment: 240971 Committed r175589: <http://trac.webkit.org/changeset/175589> All reviewed patches have been landed. Closing bug. Build fix in http://trac.webkit.org/changeset/175591 |