Context menu support for WebKit2
Patches coming shortly, starting with some small WebCore/ChromeClient changes.
In radar as <rdar://problem/7660547>
Created attachment 72420 [details] Part 1 - lay Chrome/ChromeClient groundwork in all the WebKits Just get all this crazy ChromeClient stuff in place before the serious WK2 patch comes.
Comment on attachment 72420 [details] Part 1 - lay Chrome/ChromeClient groundwork in all the WebKits Rejecting patch 72420 from commit-queue. Failed to run "['./WebKitTools/Scripts/webkit-patch', '--status-host=queues.webkit.org', 'land-attachment', '--force-clean', '--ignore-builders', '--non-interactive', '--parent-command=commit-queue', 72420]" exit_code: 1 Last 500 characters of output: .cgi?id=72420&action=edit Fetching: https://bugs.webkit.org/show_bug.cgi?id=48699&ctype=xml Processing 1 patch from 1 bug. Cleaning working directory Updating working directory Processing patch 72420 from bug 48699. NOBODY (OOPS!) found in /Projects/CommitQueue/WebCore/ChangeLog does not appear to be a valid reviewer according to committers.py. ERROR: /Projects/CommitQueue/WebCore/ChangeLog neither lists a valid reviewer nor contains the string "Unreviewed" or "Rubber stamp" (case insensitive). Full output: http://queues.webkit.org/results/4790080
Created attachment 72444 [details] Implement cross platform infrastructure and actual menu implementation for Mac Wasn't getting any review traction trying to lay in a small piece fist, may as well put the whole thing up.
Attachment 72444 [details] did not pass style-queue: Failed to run "['WebKitTools/Scripts/check-webkit-style']" exit_code: 1 WebKit2/UIProcess/WebContextMenuProxy.h:34: Code inside a namespace should not be indented. [whitespace/indent] [4] WebKit2/Shared/WebContextMenuItem.h:33: Code inside a namespace should not be indented. [whitespace/indent] [4] Total errors found: 2 in 52 files If any of these errors are false positives, please file a bug against check-webkit-style.
Attachment 72444 [details] did not build on qt: Build output: http://queues.webkit.org/results/4745097
Created attachment 72445 [details] Patch v2 - Fix Qt (and presumably the Windows) builds with the right client method signatures.
Attachment 72445 [details] did not pass style-queue: Failed to run "['WebKitTools/Scripts/check-webkit-style']" exit_code: 1 WebKit2/UIProcess/WebContextMenuProxy.h:34: Code inside a namespace should not be indented. [whitespace/indent] [4] WebKit2/Shared/WebContextMenuItem.h:33: Code inside a namespace should not be indented. [whitespace/indent] [4] Total errors found: 2 in 52 files If any of these errors are false positives, please file a bug against check-webkit-style.
Attachment 72445 [details] did not build on qt: Build output: http://queues.webkit.org/results/4854111
Created attachment 72446 [details] v3 - Now with more colons!
(In reply to comment #11) > Created an attachment (id=72446) [details] > v3 - Now with more colons! WebKit2/win/WebKit2.vcproj is in conflict with ToT. Could you update? EWS bots can test only appliable patches.
Created attachment 72462 [details] v4 - Updated to this morning I updated and there weren't any conflicts with the WebKit2.vcproj, but reattaching the new patch anyways...
Attachment 72462 [details] did not pass style-queue: Failed to run "['WebKitTools/Scripts/check-webkit-style']" exit_code: 1 WebKit2/UIProcess/WebContextMenuProxy.h:34: Code inside a namespace should not be indented. [whitespace/indent] [4] WebKit2/Shared/WebContextMenuItem.h:33: Code inside a namespace should not be indented. [whitespace/indent] [4] Total errors found: 2 in 52 files If any of these errors are false positives, please file a bug against check-webkit-style.
Attachment 72462 [details] did not build on qt: Build output: http://queues.webkit.org/results/4855105
Created attachment 72474 [details] v5 - More Qt build fixing
Attachment 72474 [details] did not pass style-queue: Failed to run "['WebKitTools/Scripts/check-webkit-style']" exit_code: 1 WebKit2/UIProcess/WebContextMenuProxy.h:34: Code inside a namespace should not be indented. [whitespace/indent] [4] WebKit2/Shared/WebContextMenuItem.h:33: Code inside a namespace should not be indented. [whitespace/indent] [4] Total errors found: 2 in 52 files If any of these errors are false positives, please file a bug against check-webkit-style.
Attachment 72474 [details] did not build on qt: Build output: http://queues.webkit.org/results/4797111
(In reply to comment #17) > Attachment 72474 [details] did not pass style-queue: > > Failed to run "['WebKitTools/Scripts/check-webkit-style']" exit_code: 1 > WebKit2/UIProcess/WebContextMenuProxy.h:34: Code inside a namespace should not be indented. [whitespace/indent] [4] > WebKit2/Shared/WebContextMenuItem.h:33: Code inside a namespace should not be indented. [whitespace/indent] [4] > Total errors found: 2 in 52 files > > > If any of these errors are false positives, please file a bug against check-webkit-style. You should fix these style errors.
(In reply to comment #19) > (In reply to comment #17) > > Attachment 72474 [details] [details] did not pass style-queue: > > > > Failed to run "['WebKitTools/Scripts/check-webkit-style']" exit_code: 1 > > WebKit2/UIProcess/WebContextMenuProxy.h:34: Code inside a namespace should not be indented. [whitespace/indent] [4] > > WebKit2/Shared/WebContextMenuItem.h:33: Code inside a namespace should not be indented. [whitespace/indent] [4] > > Total errors found: 2 in 52 files > > > > > > If any of these errors are false positives, please file a bug against check-webkit-style. > > You should fix these style errors. Do you mean I should file a bug on check-webkit-style, as the style errors are bogus?
Will work on getting Qt (and presumably Windows as it falls in a similar situation) building this morning, but the fixes will be stubs - someone still can review this... :)
(In reply to comment #20) > (In reply to comment #19) > > You should fix these style errors. > > Do you mean I should file a bug on check-webkit-style, as the style errors are bogus? I didn't notice they were bogus. You should definitely file a bug!
(In reply to comment #22) > (In reply to comment #20) > > (In reply to comment #19) > > > You should fix these style errors. > > > > Do you mean I should file a bug on check-webkit-style, as the style errors are bogus? > > I didn't notice they were bogus. You should definitely file a bug! Upon reading the official style guidelines, they are not bogus. But they are an exception to the "no indented code in namespaces" rule that we use liberally throughout the project - forward declarations of other-namespaced types in headers. I suppose an email to webkit-dev to formalize that rule is in order.
Created attachment 72514 [details] v6 - Hopefully the final build fix
Attachment 72514 [details] did not pass style-queue: Failed to run "['WebKitTools/Scripts/check-webkit-style']" exit_code: 1 WebKit2/UIProcess/WebContextMenuProxy.h:34: Code inside a namespace should not be indented. [whitespace/indent] [4] WebKit2/Shared/WebContextMenuItem.h:33: Code inside a namespace should not be indented. [whitespace/indent] [4] Total errors found: 2 in 57 files If any of these errors are false positives, please file a bug against check-webkit-style.
Comment on attachment 72514 [details] v6 - Hopefully the final build fix View in context: https://bugs.webkit.org/attachment.cgi?id=72514&action=review > WebCore/platform/qt/ContextMenuQt.cpp:82 > + return result; Can just return a temporary here: return Vector<ContextMenuItem>(); > WebCore/platform/win/ContextMenuWin.cpp:164 > +} Can just return a temporary here: return Vector<ContextMenuItem>(); > WebKit2/Shared/WebContextMenuItem.cpp:61 > + ASSERT(type == WebCore::SubmenuType); If you assert that the type is SubmenuType do you even need to pass it to the constructor? > WebKit2/Shared/WebContextMenuItem.cpp:64 > +WebContextMenuItem::WebContextMenuItem(WebCore::ContextMenuItem& item, WebCore::ContextMenu* menu) Can this be a const reference? > WebKit2/Shared/WebContextMenuItem.h:43 > +struct WebContextMenuItem { Why is this a struct and not a class? > WebKit2/UIProcess/WebContextMenuProxy.h:45 > + } It's better if you define the destructor in the .cpp file instead; that way the vtable won't be weak and emitted everywhere. > WebKit2/UIProcess/WebContextMenuProxy.h:53 > + } Might define the constructor in the .cpp file too. > WebKit2/UIProcess/mac/WebContextMenuProxyMac.mm:52 > + target = [[WebMenuTarget alloc] init]; You can just initialize target directly here. > WebKit2/UIProcess/mac/WebContextMenuProxyMac.mm:112 > + { { goes on the same line as the case. > WebKit2/UIProcess/mac/WebContextMenuProxyMac.mm:125 > + { { goes on the same line as the case. > WebKit2/WebProcess/WebPage/WebContextMenu.h:35 > + static PassRefPtr<WebContextMenu> create(WebPage* page) { return adoptRef(new WebContextMenu(page)); } { should go on a new line, as should the return statement and the closing brace. > WebKit2/WebProcess/WebPage/WebPage.cpp:912 > + m_contextMenu->itemSelected(item); Could you zero out the m_contextMenu variable here instead of keeping it around for the lifetime of the page?
(In reply to comment #26) > (From update of attachment 72514 [details]) > > WebKit2/Shared/WebContextMenuItem.cpp:64 > > +WebContextMenuItem::WebContextMenuItem(WebCore::ContextMenuItem& item, WebCore::ContextMenu* menu) > > Can this be a const reference? No - WebCore actually modifies the ContextMenuItem when doing the "checkOrEnableIfNeeded() on it. I'm not happy about this. > > WebKit2/Shared/WebContextMenuItem.h:43 > > +struct WebContextMenuItem { > > Why is this a struct and not a class? Fixing. > > > WebKit2/UIProcess/WebContextMenuProxy.h:45 > > + } > > It's better if you define the destructor in the .cpp file instead; that way the vtable won't be weak and emitted everywhere. > Might define the constructor in the .cpp file too. Done. > > WebKit2/WebProcess/WebPage/WebPage.cpp:912 > > + m_contextMenu->itemSelected(item); > > Could you zero out the m_contextMenu variable here instead of keeping it around for the lifetime of the page? Yup.
Created attachment 72524 [details] v7 - with Anders' feedback
Attachment 72524 [details] did not pass style-queue: Failed to run "['WebKitTools/Scripts/check-webkit-style']" exit_code: 1 WebKit2/UIProcess/WebContextMenuProxy.h:34: Code inside a namespace should not be indented. [whitespace/indent] [4] WebKit2/Shared/WebContextMenuItem.h:33: Code inside a namespace should not be indented. [whitespace/indent] [4] Total errors found: 2 in 58 files If any of these errors are false positives, please file a bug against check-webkit-style.
Landed in r71041
(In reply to comment #30) > Landed in r71041 With a Windows build fix in r71042