Bug 50586

Summary:

Layering Violation in ContextMenu - member variable of type HitTestResult

Product:

WebKit

Reporter:

Brian Weinstein <bweinstein>

Component:

WebCore Misc.

Assignee:

Brian Weinstein <bweinstein>

Status:

RESOLVED FIXED

Severity:

Normal

CC:

aroben, bdakin, dglazkov, eric, leandro, mrobinson, tonikitoo, webkit-ews, webkit.review.bot

Priority:

Keywords:

InRadar

Version:

528+ (Nightly build)

Hardware:

All

OS:

All

Bug Depends on:

50676, 51331

Bug Blocks:

50514

Attachments:

Description	Flags
[PATCH] Layering violation fix	none
[PATCH] Fix + Qt	none
[PATCH] Fix + Qt (Take 2)	none
[PATCH] Mac layering violation	none
[PATCH] Fix - Win, Qt, Mac	none
[PATCH] Fix - Win, Mac, Qt, Chromium	bweinstein: commit-queue-
[PATCH] Fix - Win, Mac, Qt, Chromium - w/ Darin's feedback	none
[PATCH] Fix	none
[PATCH] Fix	none
[PATCH] Fix - all platforms	sullivan: review+
Diff between reviewed version and latest version	none

Description Brian Weinstein 2010-12-06 13:33:33 PST

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.

Comment 1 Brian Weinstein 2010-12-06 13:33:58 PST

<rdar://problem/8734497>

Comment 2 Brian Weinstein 2010-12-06 19:46:56 PST

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

Comment 3 WebKit Review Bot 2010-12-06 19:49:35 PST

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.

Comment 4 Early Warning System Bot 2010-12-06 19:57:55 PST

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

Comment 5 Brian Weinstein 2010-12-06 20:16:22 PST

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

Comment 6 WebKit Review Bot 2010-12-06 20:18:10 PST

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.

Comment 7 Early Warning System Bot 2010-12-06 20:27:56 PST

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

Comment 8 Brian Weinstein 2010-12-06 20:34:51 PST

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

Comment 9 WebKit Review Bot 2010-12-07 08:25:36 PST

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.

Comment 10 WebKit Review Bot 2010-12-07 08:29:50 PST

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.

Comment 11 WebKit Review Bot 2010-12-07 08:47:03 PST

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.

Comment 12 Adam Roben (:aroben) 2010-12-07 08:49:28 PST

(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 13 WebKit Review Bot 2010-12-07 09:27:10 PST

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.

Comment 14 WebKit Review Bot 2010-12-07 09:31:21 PST

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.

Comment 15 WebKit Review Bot 2010-12-07 09:48:10 PST

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.

Comment 16 Darin Adler 2010-12-07 09:48:21 PST

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.

Comment 17 WebKit Review Bot 2010-12-07 10:28:16 PST

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.

Comment 18 WebKit Review Bot 2010-12-07 10:32:26 PST

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.

Comment 19 WebKit Review Bot 2010-12-07 10:49:24 PST

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.

Comment 20 WebKit Review Bot 2010-12-07 11:29:28 PST

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.

Comment 21 WebKit Review Bot 2010-12-07 11:33:37 PST

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.

Comment 22 WebKit Review Bot 2010-12-07 11:50:37 PST

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.

Comment 23 WebKit Review Bot 2010-12-07 12:30:36 PST

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.

Comment 24 Brian Weinstein 2010-12-07 13:46:26 PST

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

Comment 25 John Sullivan 2010-12-07 13:58:37 PST

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 26 Brian Weinstein 2010-12-07 14:48:05 PST

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

Landed in r73469.

Comment 27 Brian Weinstein 2010-12-07 16:55:06 PST

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

Comment 28 Brian Weinstein 2010-12-07 20:15:03 PST

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

Comment 29 WebKit Review Bot 2010-12-07 21:29:14 PST

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.

Comment 30 WebKit Review Bot 2010-12-07 21:30:14 PST

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.

Comment 31 WebKit Review Bot 2010-12-07 21:34:22 PST

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.

Comment 32 Brian Weinstein 2010-12-07 21:34:33 PST

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

Comment 33 WebKit Review Bot 2010-12-07 21:52:23 PST

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.

Comment 34 WebKit Review Bot 2010-12-07 21:59:14 PST

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.

Comment 35 WebKit Review Bot 2010-12-07 22:00:37 PST

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.

Comment 36 WebKit Review Bot 2010-12-07 23:14:57 PST

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

Comment 37 Brian Weinstein 2010-12-07 23:23:16 PST

Created attachment 75877 [details]
[PATCH] Fix

Comment 38 Brian Weinstein 2010-12-08 00:07:20 PST

Created attachment 75879 [details]
[PATCH] Fix

Comment 39 Brian Weinstein 2010-12-08 10:14:23 PST

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

Comment 40 Brian Weinstein 2010-12-08 10:30:01 PST

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 41 John Sullivan 2010-12-08 10:36:32 PST

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

Comment 42 Brian Weinstein 2010-12-08 12:20:04 PST

Landed in r73535.