[webkit-reviews] review denied: [Bug 27443] [Gtk] Combobox should support "find-as-you-type" search : [Attachment 60537] 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 60537: Find-as-you-type proposed patch
https://bugs.webkit.org/attachment.cgi?id=60537&action=review

------- Additional Comments from Gustavo Noronha (kov) <gns at gnome.org>
 181	 std::string searchedWord;
 182	 std::list<GList*> searchDomainList;

As you can see from the other member variables, these should have m_ preffixing
their names.

 144	 resetWord(that);

This could be turned into that->resetWord();, which would make this look much
more natural:

[...]
 160 // Clear the search and the associated domain list
 161 void PopupMenu::resetWord(PopupMenu* that)
 162 {
 163	 that->searchedWord.clear();
 164	 that->searchDomainList.clear();
 165 }

So you'd make this be a non-static private method, remove the argument, and use
the member variables directly instead of through "that". The same could be the
case for all the other functions that are coded like this only because they are
callbacks. I would prefer having the callbacks be small, and just cause the
object methods to be called. So you would have a keyPressEventCallback static
method that would do if (!that->isKeyPressToZap(event)) {
that->searchItem(event), which should make the bulk of the code more natural.

 167 // Test keypresses to see if they are to be passed to the event handler
 168 bool PopupMenu::isKeyPressToZap(GdkEventKey* event, PopupMenu* that)
 169 {
 170	 guint keyval = gdk_unicode_to_keyval(event->keyval);
 171 
 172	 if (keyval == (event->keyval | 0x01000000)) {
 173	     if (event->keyval == GDK_Delete)
 174		 resetWord(that);
 175	     return true;
 176	 }
 177 
 178	 return false;
 179 }

It doesn't make too much sense to have a "conditional" call actually change
stuff. The call to resetWord should go in the else block in the code that calls
isKeyPressToZap. Also, this function seems to be overloaded for its name, as
the comment above it implies. It's more a function to decide whether the press
should be passed to the handler or not, though I am not positive what you're
trying to do by returning true on this condition: keyval == (event->keyval |
0x01000000), what is that testing?

Is this code based on some other code, btw? I'll say r- so we can get those
issues resolved, thanks!