The menu should be created in the UI process.
Created attachment 339771 [details] Patch
Created attachment 339774 [details] Patch
<rdar://problem/40043120>
Comment on attachment 339774 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=339774&action=review > Source/WebKit/Shared/mac/PDFContextMenu.h:28 > +#if ENABLE(WEBPROCESS_WINDOWSERVER_BLOCKING) Why would we change the behavior of PDFKit based on whether this feature is enabled or not? Seems like this should just be the behavior of PDFPlugin, period. > Source/WebKit/UIProcess/WebPageProxy.h:1029 > +#if PLATFORM(MAC) && ENABLE(WEBPROCESS_WINDOWSERVER_BLOCKING) None of these should be PLATFORM(MAC), they should all be ENABLE(PDFKIT_PLUGIN). And not ENABLE(WEBPROCESS_WINDOWSERVER_BLOCKING) > Source/WebKit/UIProcess/mac/WebPageProxyMac.mm:82 > +- (id) menuItem; No space after ) > Source/WebKit/UIProcess/mac/WebPageProxyMac.mm:572 > + auto menuTarget = [[WKPDFMenuTarget alloc] init]; > + auto nsMenu = [[NSMenu alloc] init]; Aren’t these leaking?? > Source/WebKit/UIProcess/mac/WebPageProxyMac.mm:589 > + _NSPopUpCarbonMenu3(nsMenu, nil, nil, contextMenu.m_point, -1, nil, 0, nil, NSPopUpMenuTypeContext, nil); I’m a bit concerned how much different this is from WebContextMenuClient::showContextMenu. Can we share some of that? Or at least do the same thing, so we’re not so confused later? > Source/WebKit/WebProcess/Plugins/PDF/PDFPlugin.mm:1582 > + auto item = [nsMenu itemAtIndex:i]; Can’t NSMenus have NSMenu children? This might fall apart in that case. > Source/WebKit/WebProcess/Plugins/PDF/PDFPlugin.mm:1592 > + webFrame()->page()->sendSync(Messages::WebPageProxy::ShowPDFContextMenu(contextMenu), Messages::WebPageProxy::ShowPDFContextMenu::Reply(selectedIndex)); Do we usually block the Web Content process main thread on popup menus? Is that necessary?
(In reply to Tim Horton from comment #4) > Comment on attachment 339774 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=339774&action=review > > > Source/WebKit/Shared/mac/PDFContextMenu.h:28 > > +#if ENABLE(WEBPROCESS_WINDOWSERVER_BLOCKING) > > Why would we change the behavior of PDFKit based on whether this feature is > enabled or not? Seems like this should just be the behavior of PDFPlugin, > period. > > > Source/WebKit/UIProcess/WebPageProxy.h:1029 > > +#if PLATFORM(MAC) && ENABLE(WEBPROCESS_WINDOWSERVER_BLOCKING) > > None of these should be PLATFORM(MAC), they should all be > ENABLE(PDFKIT_PLUGIN). And not ENABLE(WEBPROCESS_WINDOWSERVER_BLOCKING) > > > Source/WebKit/UIProcess/mac/WebPageProxyMac.mm:82 > > +- (id) menuItem; > > No space after ) > > > Source/WebKit/UIProcess/mac/WebPageProxyMac.mm:572 > > + auto menuTarget = [[WKPDFMenuTarget alloc] init]; > > + auto nsMenu = [[NSMenu alloc] init]; > > Aren’t these leaking?? > > > Source/WebKit/UIProcess/mac/WebPageProxyMac.mm:589 > > + _NSPopUpCarbonMenu3(nsMenu, nil, nil, contextMenu.m_point, -1, nil, 0, nil, NSPopUpMenuTypeContext, nil); > > I’m a bit concerned how much different this is from > WebContextMenuClient::showContextMenu. Can we share some of that? Or at > least do the same thing, so we’re not so confused later? > > > Source/WebKit/WebProcess/Plugins/PDF/PDFPlugin.mm:1582 > > + auto item = [nsMenu itemAtIndex:i]; > > Can’t NSMenus have NSMenu children? This might fall apart in that case. I guess not, it’s that you have an NSMenuItem child with a submenu. It doesn’t happen now but you should handle it gracefully. > > Source/WebKit/WebProcess/Plugins/PDF/PDFPlugin.mm:1592 > > + webFrame()->page()->sendSync(Messages::WebPageProxy::ShowPDFContextMenu(contextMenu), Messages::WebPageProxy::ShowPDFContextMenu::Reply(selectedIndex)); > > Do we usually block the Web Content process main thread on popup menus? Is > that necessary?
Comment on attachment 339774 [details] Patch Attachment 339774 [details] did not pass win-ews (win): Output: http://webkit-queues.webkit.org/results/7601845 New failing tests: http/tests/security/contentSecurityPolicy/userAgentShadowDOM/allow-audio.html
Created attachment 339791 [details] Archive of layout-test-results from ews206 for win-future The attached test failures were seen while running run-webkit-tests on the win-ews. Bot: ews206 Port: win-future Platform: CYGWIN_NT-6.1-2.9.0-0.318-5-3-x86_64-64bit
(In reply to Tim Horton from comment #4) > Comment on attachment 339774 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=339774&action=review > > > Source/WebKit/Shared/mac/PDFContextMenu.h:28 > > +#if ENABLE(WEBPROCESS_WINDOWSERVER_BLOCKING) > > Why would we change the behavior of PDFKit based on whether this feature is > enabled or not? Seems like this should just be the behavior of PDFPlugin, > period. > > > Source/WebKit/UIProcess/WebPageProxy.h:1029 > > +#if PLATFORM(MAC) && ENABLE(WEBPROCESS_WINDOWSERVER_BLOCKING) > > None of these should be PLATFORM(MAC), they should all be > ENABLE(PDFKIT_PLUGIN). And not ENABLE(WEBPROCESS_WINDOWSERVER_BLOCKING) > > > Source/WebKit/UIProcess/mac/WebPageProxyMac.mm:82 > > +- (id) menuItem; > > No space after ) > > > Source/WebKit/UIProcess/mac/WebPageProxyMac.mm:572 > > + auto menuTarget = [[WKPDFMenuTarget alloc] init]; > > + auto nsMenu = [[NSMenu alloc] init]; > > Aren’t these leaking?? > > > Source/WebKit/UIProcess/mac/WebPageProxyMac.mm:589 > > + _NSPopUpCarbonMenu3(nsMenu, nil, nil, contextMenu.m_point, -1, nil, 0, nil, NSPopUpMenuTypeContext, nil); > > I’m a bit concerned how much different this is from > WebContextMenuClient::showContextMenu. Can we share some of that? Or at > least do the same thing, so we’re not so confused later? > > > Source/WebKit/WebProcess/Plugins/PDF/PDFPlugin.mm:1582 > > + auto item = [nsMenu itemAtIndex:i]; > > Can’t NSMenus have NSMenu children? This might fall apart in that case. > > > Source/WebKit/WebProcess/Plugins/PDF/PDFPlugin.mm:1592 > > + webFrame()->page()->sendSync(Messages::WebPageProxy::ShowPDFContextMenu(contextMenu), Messages::WebPageProxy::ShowPDFContextMenu::Reply(selectedIndex)); > > Do we usually block the Web Content process main thread on popup menus? Is > that necessary? Yes, _NSPopUpCarbonMenu3 is blocking the main thread. Thanks for reviewing! I will update the patch.
Created attachment 339841 [details] Patch
Created attachment 339846 [details] Patch
(In reply to Tim Horton from comment #5) > (In reply to Tim Horton from comment #4) > > Comment on attachment 339774 [details] > > Patch > > > > View in context: > > https://bugs.webkit.org/attachment.cgi?id=339774&action=review > > > > > Source/WebKit/Shared/mac/PDFContextMenu.h:28 > > > +#if ENABLE(WEBPROCESS_WINDOWSERVER_BLOCKING) > > > > Why would we change the behavior of PDFKit based on whether this feature is > > enabled or not? Seems like this should just be the behavior of PDFPlugin, > > period. > > > > > Source/WebKit/UIProcess/WebPageProxy.h:1029 > > > +#if PLATFORM(MAC) && ENABLE(WEBPROCESS_WINDOWSERVER_BLOCKING) > > > > None of these should be PLATFORM(MAC), they should all be > > ENABLE(PDFKIT_PLUGIN). And not ENABLE(WEBPROCESS_WINDOWSERVER_BLOCKING) > > > > > Source/WebKit/UIProcess/mac/WebPageProxyMac.mm:82 > > > +- (id) menuItem; > > > > No space after ) > > > > > Source/WebKit/UIProcess/mac/WebPageProxyMac.mm:572 > > > + auto menuTarget = [[WKPDFMenuTarget alloc] init]; > > > + auto nsMenu = [[NSMenu alloc] init]; > > > > Aren’t these leaking?? > > > > > Source/WebKit/UIProcess/mac/WebPageProxyMac.mm:589 > > > + _NSPopUpCarbonMenu3(nsMenu, nil, nil, contextMenu.m_point, -1, nil, 0, nil, NSPopUpMenuTypeContext, nil); > > > > I’m a bit concerned how much different this is from > > WebContextMenuClient::showContextMenu. Can we share some of that? Or at > > least do the same thing, so we’re not so confused later? > > > > > Source/WebKit/WebProcess/Plugins/PDF/PDFPlugin.mm:1582 > > > + auto item = [nsMenu itemAtIndex:i]; > > > > Can’t NSMenus have NSMenu children? This might fall apart in that case. > > I guess not, it’s that you have an NSMenuItem child with a submenu. It > doesn’t happen now but you should handle it gracefully. > I would like to handle submenus recursively, but since this is not required at the moment, do you think we could defer that to a follow-up patch?
Sure, what I meant was you should bail/skip the item if it’s a submenu.
Created attachment 339855 [details] Patch
Comment on attachment 339855 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=339855&action=review > Source/WebKit/Shared/mac/PDFContextMenu.h:32 > + String m_title; Public fields in structs don’t usually get m_ prefixes, right? > Source/WebKit/UIProcess/mac/WebPageProxyMac.mm:79 > + id menuItem; should this be “selectedMenuItem”? Also, ObjC members get a leading underscore. > Source/WebKit/UIProcess/mac/WebPageProxyMac.mm:81 > +- (instancetype)init; This is implicit since you’re an NSObject. > Source/WebKit/UIProcess/mac/WebPageProxyMac.mm:89 > + self = [super init]; if (!self) return nil; > Source/WebKit/UIProcess/mac/WebPageProxyMac.mm:570 > + auto menuTarget = [[[WKPDFMenuTarget alloc] init] autorelease]; > + auto nsMenu = [[[NSMenu alloc] init] autorelease]; RetainPtr & adoptNS are the preferred way to solve this problem > Source/WebKit/UIProcess/mac/WebPageProxyMac.mm:580 > + auto nsItem = [[NSMenuItem alloc] init]; This one is a leak too? > Source/WebKit/UIProcess/mac/WebPageProxyMac.mm:591 > + NSWindow* window = m_pageClient.platformWindow(); ObjC stars go on the other side
Created attachment 339859 [details] Patch
Comment on attachment 339859 [details] Patch Thanks for reviewing!
Comment on attachment 339859 [details] Patch Clearing flags on attachment: 339859 Committed r231510: <https://trac.webkit.org/changeset/231510>