[webkit-reviews] review denied: [Bug 42592] PopupMenu refactoring in preparation to WebKit2 : [Attachment 62776] patch 9

[bugzilla-daemon at webkit.org]

Darin Fisher (:fishd, Google) <fishd at chromium.org> has denied Luiz Agostini
<luiz at webkit.org>'s request for review:
Bug 42592: PopupMenu refactoring in preparation to WebKit2
https://bugs.webkit.org/show_bug.cgi?id=42592

Attachment 62776: patch 9
https://bugs.webkit.org/attachment.cgi?id=62776&action=review

------- Additional Comments from Darin Fisher (:fishd, Google)
<fishd at chromium.org>
Looks great overall.

Comments:

Are you sure you wouldn't want to make it valid for create{Search}PopupMenu
to return NULL?  Then you wouldn't need Empty{Search}PopupMenu.  It seems
reasonable for a Client to not create something.


>+++ b/WebCore/page/Chrome.cpp
...
>+PassRefPtr<SearchPopupMenu> Chrome::createSearchPopupMenu(PopupMenuClient*
client) const
>+{
>+    return m_client->createSearchPopupMenu(client);
>+}
>+
> 
> } // namespace WebCore

nit: ^^^ only one new line before the close of the namespace


>diff --git a/WebCore/platform/brew/PopupMenuBrew.cpp
b/WebCore/platform/brew/PopupMenuBrew.cpp
...
>+// bool PopupMenuBrew::itemWritingDirectionIsNatural()
>+// {
>+//	 return true;
>+// }

^^^ ordinarily bad form to leave code commented out without an explanation.  or

perhaps you intended to delete this?


>diff --git a/WebCore/platform/brew/PopupMenuBrew.h
b/WebCore/platform/brew/PopupMenuBrew.h
...
>+class PopupMenuBrew : public PopupMenu {
...
>+private:
>+    PopupMenuClient* m_popupClient;
>+    FrameView* m_view;
>+
>+    PopupMenuClient* client() const { return m_popupClient; }
>+
>+};

nit: ^^^ no new line before the "};"


>+++ b/WebCore/platform/chromium/PopupMenuChromium.h
...
>+class PopupMenuChromium : public PopupMenu {
>+public:
>+    PopupMenuChromium(PopupMenuClient*);
>+    ~PopupMenuChromium();
>+
>+    virtual void show(const IntRect&, FrameView*, int index);
>+    virtual void hide();
>+    virtual void updateFromElement();
>+    virtual void disconnectClient();
>+
>+private:
>+    PopupMenuClient* m_popupClient;
>+    PopupMenuPrivate p;
>+
>+    PopupMenuClient* client() const { return m_popupClient; }
>+};

PopupMenuPrivate is now unnecessary (it can be folded into PopupMenuChromium),
but that refactoring can happen in a separate patch, and you don't need to be
on the hook for it.


>+++ b/WebCore/platform/efl/PopupMenuEfl.h
...
>+private:
>+    PopupMenuClient* m_popupClient;
>+    FrameView* m_view;
>+
>+    PopupMenuClient* client() const { return m_popupClient; }
>+
>+};

nit: ^^^ no new line before "};"

Also, I think it is generally better form to list methods before
variables in the private section of a class.  This applies to the
other newly added classes in this patch.


>+++ b/WebCore/platform/haiku/PopupMenuHaiku.h
...
>+private:
>+    PopupMenuClient* m_popupClient;
>+    HaikuPopup* m_menu;
>+
>+    PopupMenuClient* client() const { return m_popupClient; }
>+
>+};

nit: ^^^ no new line before "};"


>+++ b/WebCore/platform/wx/PopupMenuWx.h
...
>+    PopupMenuClient* client() const { return m_popupClient; }
>+
>+};

nit: ^^^ no new line before "};"


>+++ b/WebKit/chromium/src/ChromeClientImpl.cpp

>+bool ChromeClientImpl::selectItemWritingDirectionIsNatural()
>+{
>+    return false;
>+}
>+
>+PassRefPtr<WebCore::PopupMenu>
ChromeClientImpl::createPopupMenu(WebCore::PopupMenuClient* client) const
>+{
>+    return adoptRef(new WebCore::PopupMenuChromium(client));
>+}
>+
>+PassRefPtr<WebCore::SearchPopupMenu>
ChromeClientImpl::createSearchPopupMenu(WebCore::PopupMenuClient* client) const

>+{
>+    return adoptRef(new WebCore::SearchPopupMenuChromium(client));
>+}

nit: ^^^ no need for the WebCore:: prefix in this file thanks to
the 'using namespace WebCore' at the top of the file.


>+++ b/WebKit2/WebKit2.xcodeproj/project.pbxproj
...
>@@ -1493,7 +1507,12 @@
>				E1EE55F811F8F1BC00CCBEE4 /* WKBundleRange.cpp
in Sources */,
>				E1EE565611F8F71700CCBEE4 /* WKBundleNode.cpp in
Sources */,
>				1AE4976911FF658E0048B464 /* NPJSObject.cpp in
Sources */,
>+<<<<<<< HEAD:WebKit2/WebKit2.xcodeproj/project.pbxproj
>				1AE4987911FF7FAA0048B464 /* JSNPObject.cpp in
Sources */,
>+=======
>+				D3B9484611FF4B6500032B39 /* WebPopupMenu.cpp in
Sources */,
>+				D3B9484811FF4B6500032B39 /*
WebSearchPopupMenu.cpp in Sources */,
>+>>>>>>> PopupMenu refactoring in preparation to
WebKit2:WebKit2/WebKit2.xcodeproj/project.pbxproj

^^^ oops!