There is a layering violation in ContextMenu. It currently has a member variable: HitTestResult m_hitTestResult; Which is a layering violation because classes in WebCore/platform can't know about classes in WebCore. The planned fix for this is to move ContextMenu::populate from ContextMenu into ContextMenuController.
<rdar://problem/8734497>
Created attachment 75776 [details] [PATCH] Layering violation fix
Attachment 75776 [details] did not pass style-queue: Failed to run "[u'git', u'reset', u'--hard', u'HEAD']" exit_code: 128 error: Could not write new index file. fatal: Could not reset index file to revision 'HEAD'. If any of these errors are false positives, please file a bug against check-webkit-style.
Attachment 75776 [details] did not build on qt: Build output: http://queues.webkit.org/results/6717083
Created attachment 75778 [details] [PATCH] Fix + Qt
Attachment 75778 [details] did not pass style-queue: Failed to run "[u'git', u'reset', u'--hard', u'HEAD']" exit_code: 128 error: git checkout-index: unable to write file LayoutTests/ChangeLog fatal: Could not reset index file to revision 'HEAD'. If any of these errors are false positives, please file a bug against check-webkit-style.
Attachment 75778 [details] did not build on qt: Build output: http://queues.webkit.org/results/6880002
Created attachment 75781 [details] [PATCH] Fix + Qt (Take 2)
Attachment 75776 [details] did not pass style-queue: Failed to run "[u'git', u'reset', u'--hard', u'refs/remotes/trunk']" exit_code: 128 error: Could not write new index file. fatal: Could not reset index file to revision 'refs/remotes/trunk'. If any of these errors are false positives, please file a bug against check-webkit-style.
Attachment 75778 [details] did not pass style-queue: Failed to run "[u'git', u'reset', u'--hard', u'refs/remotes/trunk']" exit_code: 128 error: Could not write new index file. fatal: Could not reset index file to revision 'refs/remotes/trunk'. If any of these errors are false positives, please file a bug against check-webkit-style.
Attachment 75781 [details] did not pass style-queue: Failed to run "[u'git', u'reset', u'--hard', u'refs/remotes/trunk']" exit_code: 128 error: Could not write new index file. fatal: Could not reset index file to revision 'refs/remotes/trunk'. If any of these errors are false positives, please file a bug against check-webkit-style.
(In reply to comment #11) > Attachment 75781 [details] did not pass style-queue: > > Failed to run "[u'git', u'reset', u'--hard', u'refs/remotes/trunk']" exit_code: 128 > error: Could not write new index file. > fatal: Could not reset index file to revision 'refs/remotes/trunk'. > > > If any of these errors are false positives, please file a bug against check-webkit-style. Eric, any idea what's going on here?
Comment on attachment 75781 [details] [PATCH] Fix + Qt (Take 2) View in context: https://bugs.webkit.org/attachment.cgi?id=75781&action=review > WebCore/page/ContextMenuController.cpp:79 > + , m_hitTestResult(HitTestResult(IntPoint())) This seems a little strange. Do we really want to initialize a hit test result with the point 0,0? Should we add a default constructor for HitTestResult? Or maybe we should use OwnPtr<HitTestResult> and have it be 0 until we can actually do the hit test. > WebCore/page/ContextMenuController.cpp:139 > + return new ContextMenu(); No parentheses needed here when calling new with a constructor with no arguments. Can we change this function’s return type to PassOwnPtr? > WebCore/page/ContextMenuController.h:79 > + void createAndAppendFontSubMenu(ContextMenuItem& fontMenuItem); > + void createAndAppendSpellingAndGrammarSubMenu(ContextMenuItem& spellingAndGrammarMenuItem); > + void createAndAppendSpellingSubMenu(ContextMenuItem& spellingMenuItem); > + void createAndAppendSpeechSubMenu(ContextMenuItem& speechMenuItem); > + void createAndAppendWritingDirectionSubMenu(ContextMenuItem& writingDirectionMenuItem); > + void createAndAppendTextDirectionSubMenu(ContextMenuItem& textDirectionMenuItem); > + void createAndAppendSubstitutionsSubMenu(ContextMenuItem& substitutionsMenuItem); > + void createAndAppendTransformationsSubMenu(ContextMenuItem& transformationsMenuItem); I’m not sure these arguments benefit from the names here.
Created attachment 75839 [details] [PATCH] Mac layering violation
Comment on attachment 75839 [details] [PATCH] Mac layering violation View in context: https://bugs.webkit.org/attachment.cgi?id=75839&action=review > WebKit/mac/WebView/WebHTMLView.mm:3281 > + for (NSInteger i = 0; i < [menu numberOfItems]; ++i) { You should use a local variable for the result of [menu numberOfItems] to avoid unnecessary ObjC dispatch.
Comment on attachment 75839 [details] [PATCH] Mac layering violation Landed in r73469.
Created attachment 75850 [details] [PATCH] Fix - Win, Qt, Mac
Created attachment 75864 [details] [PATCH] Fix - Win, Mac, Qt, Chromium
Attachment 75776 [details] did not pass style-queue: Failed to run "['WebKitTools/Scripts/update-webkit']" exit_code: 2 Updating OpenSource Incomplete data: Delta source ended unexpectedly at /usr/lib/git-core/git-svn line 5061 Died at WebKitTools/Scripts/update-webkit line 132. If any of these errors are false positives, please file a bug against check-webkit-style.
Attachment 75778 [details] did not pass style-queue: Failed to run "['WebKitTools/Scripts/update-webkit']" exit_code: 2 Updating OpenSource Incomplete data: Delta source ended unexpectedly at /usr/lib/git-core/git-svn line 5061 Died at WebKitTools/Scripts/update-webkit line 132. If any of these errors are false positives, please file a bug against check-webkit-style.
Attachment 75781 [details] did not pass style-queue: Failed to run "['WebKitTools/Scripts/update-webkit']" exit_code: 2 Updating OpenSource Incomplete data: Delta source ended unexpectedly at /usr/lib/git-core/git-svn line 5061 Died at WebKitTools/Scripts/update-webkit line 132. If any of these errors are false positives, please file a bug against check-webkit-style.
Created attachment 75868 [details] [PATCH] Fix - Win, Mac, Qt, Chromium - w/ Darin's feedback Forgot to incorporate Darin's feedback into previous versions of the patch - this patch adds: - createContextMenu returns a PassOwnPtr - Default constructor for HitTestResult - Removed variable names from ContextMenuController.h
Attachment 75839 [details] did not pass style-queue: Failed to run "['WebKitTools/Scripts/update-webkit']" exit_code: 2 Updating OpenSource Incomplete data: Delta source ended unexpectedly at /usr/lib/git-core/git-svn line 5061 Died at WebKitTools/Scripts/update-webkit line 132. If any of these errors are false positives, please file a bug against check-webkit-style.
Attachment 75864 [details] did not pass style-queue: Failed to run "['WebKitTools/Scripts/update-webkit']" exit_code: 2 Updating OpenSource Incomplete data: Delta source ended unexpectedly at /usr/lib/git-core/git-svn line 5061 Died at WebKitTools/Scripts/update-webkit line 132. If any of these errors are false positives, please file a bug against check-webkit-style.
Attachment 75868 [details] did not pass style-queue: Failed to run "['WebKitTools/Scripts/update-webkit']" exit_code: 2 Updating OpenSource Incomplete data: Delta source ended unexpectedly at /usr/lib/git-core/git-svn line 5061 Died at WebKitTools/Scripts/update-webkit line 132. If any of these errors are false positives, please file a bug against check-webkit-style.
Attachment 75868 [details] did not build on chromium: Build output: http://queues.webkit.org/results/6758099
Created attachment 75877 [details] [PATCH] Fix
Created attachment 75879 [details] [PATCH] Fix
Created attachment 75920 [details] [PATCH] Fix - all platforms
Created attachment 75925 [details] Diff between reviewed version and latest version I implemented Darin's feedback (adding a default constructor for HitTestResult, removing the function arguments in ContextMenuController.h), and I fixed more platforms.
Comment on attachment 75920 [details] [PATCH] Fix - all platforms I only read through the parts that changed since Darin reviewed this earlier. Those parts look good, so r+.
Landed in r73535.