Bug 50586 – Layering Violation in ContextMenu - member variable of type HitTestResult

• Component: WebCore Misc.
• Status: RESOLVED FIXED

[Brian Weinstein]

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.

[Brian Weinstein]

<rdar://problem/8734497>

[Brian Weinstein]

Created attachment 75776 [details]
[PATCH] Layering violation fix

[WebKit Review Bot]

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.

[Early Warning System Bot]

Attachment 75776 [details] did not build on qt:
Build output: http://queues.webkit.org/results/6717083

[Brian Weinstein]

Created attachment 75778 [details]
[PATCH] Fix + Qt

[WebKit Review Bot]

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.

[Early Warning System Bot]

Attachment 75778 [details] did not build on qt:
Build output: http://queues.webkit.org/results/6880002

[Brian Weinstein]

Created attachment 75781 [details]
[PATCH] Fix + Qt (Take 2)

[WebKit Review Bot]

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.

[WebKit Review Bot]

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.

[WebKit Review Bot]

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.

[Adam Roben (:aroben)]

(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?

[WebKit Review Bot]

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.

[WebKit Review Bot]

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.

[WebKit Review Bot]

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.

[Darin Adler]

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.

[WebKit Review Bot]

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.

[WebKit Review Bot]

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.

[WebKit Review Bot]

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.

[WebKit Review Bot]

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.

[WebKit Review Bot]

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.

[WebKit Review Bot]

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.

[WebKit Review Bot]

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.

[Brian Weinstein]

Created attachment 75839 [details]
[PATCH] Mac layering violation

[John Sullivan]

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.

[Brian Weinstein]

Comment on attachment 75839 [details]
[PATCH] Mac layering violation

Landed in r73469.

[Brian Weinstein]

Created attachment 75850 [details]
[PATCH] Fix - Win, Qt, Mac

[Brian Weinstein]

Created attachment 75864 [details]
[PATCH] Fix - Win, Mac, Qt, Chromium

[WebKit Review Bot]

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.

[WebKit Review Bot]

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.

[WebKit Review Bot]

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.

[Brian Weinstein]

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

[WebKit Review Bot]

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.

[WebKit Review Bot]

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.

[WebKit Review Bot]

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.

[WebKit Review Bot]

Attachment 75868 [details] did not build on chromium:
Build output: http://queues.webkit.org/results/6758099

[Brian Weinstein]

Created attachment 75877 [details]
[PATCH] Fix

[Brian Weinstein]

Created attachment 75879 [details]
[PATCH] Fix

[Brian Weinstein]

Created attachment 75920 [details]
[PATCH] Fix - all platforms

[Brian Weinstein]

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.

[John Sullivan]

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+.

[Brian Weinstein]

Landed in r73535.