[webkit-reviews] review granted: [Bug 50586] Layering Violation in ContextMenu - member variable of type HitTestResult : [Attachment 75781] [PATCH] Fix + Qt (Take 2)

bugzilla-daemon at webkit.org bugzilla-daemon at webkit.org
Tue Dec 7 09:48:20 PST 2010


Darin Adler <darin at apple.com> has granted Brian Weinstein
<bweinstein at apple.com>'s request for review:
Bug 50586: Layering Violation in ContextMenu - member variable of type
HitTestResult
https://bugs.webkit.org/show_bug.cgi?id=50586

Attachment 75781: [PATCH] Fix + Qt (Take 2)
https://bugs.webkit.org/attachment.cgi?id=75781&action=review

------- Additional Comments from Darin Adler <darin at apple.com>
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.


More information about the webkit-reviews mailing list