[webkit-reviews] review denied: [Bug 27443] [Gtk] Combobox should support "find-as-you-type" search : [Attachment 57170] Find-as-you-type proposed patch

[bugzilla-daemon at webkit.org]

Gustavo Noronha (kov) <gns at gnome.org> has denied Apelete Seketeli
<apelete at seketeli.org>'s request for review:
Bug 27443: [Gtk] Combobox should support "find-as-you-type" search
https://bugs.webkit.org/show_bug.cgi?id=27443

Attachment 57170: Find-as-you-type proposed patch
https://bugs.webkit.org/attachment.cgi?id=57170&action=review

------- Additional Comments from Gustavo Noronha (kov) <gns at gnome.org>
WebCore/platform/gtk/PopupMenuGtk.cpp:31
 +  #include <algorithm>
Includes must be ordered, please take a look at
http://webkit.org/coding/coding-style.html

WebCore/platform/PopupMenu.h:77
 +  void trim(std::string&);
We should protect all of the code with the platform checks. Having said that, I
don't like having a trim implementation solely for this case. Aren't there
other trim implementations in standard libraries we can use? I know pango has
pango_trim_string, for example, could we not use it? In case there is no, I'd
prefer having that code go into one of WebKit's string classes, and that class
should be used here instead of std::string.

WebCore/platform/gtk/PopupMenuGtk.cpp:236
 +	     ++i) {
This construct is very unusual in WebKit, please use a single line for code
like this.

WebCore/platform/gtk/PopupMenuGtk.cpp:266
 +		searchDomain = GTK_MENU_SHELL(that->m_popup.get())->children;
We are avoiding using struct fields directly. Please use the same method used
here: http://trac.webkit.org/changeset/61206

Have you considered contributing something similar to GTK+ directly?