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

[bugzilla-daemon at webkit.org]

Xan Lopez <xan.lopez at gmail.com> 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 62186: Find-as-you-type proposed patch
https://bugs.webkit.org/attachment.cgi?id=62186&action=review

------- Additional Comments from Xan Lopez <xan.lopez at gmail.com>
>+    std::string m_searchedWord;

I don't think std::string is commonly used in WebKit, check CString in WTF.

>+    std::list<GList*> m_searchDomainList;

Same for this, mixing two list implementations seems a bit weird. Maybe just
stick to GList?

>+// Test keypresses to see if they are to be passed to the event handler
>+bool PopupMenu::isKeyPressToZap(GdkEventKey* event)

The method name should be what the method does, not what it's used for IMHO.
Since you seem to check if it's a modifier, the name should go along those
lines.

>+{
>+    guint keyval = gdk_unicode_to_keyval(event->keyval);

So... isn't this already a key symbol? I'm confused.

>+
>+    // If the key is a modifier it should be zapped
>+    if (keyval == (event->keyval | 0x01000000))
>+	  return true;

In any case you should mention that this is what the function returns if
there's no corresponding symbol, otherwise it's difficult to know what's going
on.

>+
>+    return false;
>+}
>+
>+// Search for a menuItem in a domain and returns the menuItem if found
>+// searchDomain: the domain searched
>+// word: the menuItem label
>+// resultItem: the menuItem if found
>+bool PopupMenu::search(GList* searchDomain, const std::string& word, GList*&
resultItem)
>+{
>+    ASSERT(searchDomain);
>+
>+    std::string label;
>+    unsigned int size = g_list_length(searchDomain);
>+
>+    for (unsigned int i = 0; i < size; ++i) {
>+	  label = gtk_menu_item_get_label(GTK_MENU_ITEM(searchDomain->data));
>+	  if (!label.empty())
>+	      label = pango_trim_string(label.c_str());
>+	  if (label.size() > word.size())
>+	      label.erase(word.size());
>+
>+	  if ((label.c_str() && word.c_str()) && !strcasecmp(label.c_str(),
word.c_str())) {
>+	      resultItem = searchDomain;
>+	      return true;
>+	  }

I think you should use the GLib UTF8 collate functions here, and just compare
by prefix using the word length, seems more straightforward.

>+	  searchDomain = g_list_next(searchDomain);
>+    }

Micro-optimization time! You don't really need to go through the list once to
get the size and then iterate again, just iterate until NEXT is NULL.

>+
>+    return false;
>+}
>+
>+// Build a search domain containing menuItems starting with 'word'
>+// startingFrom: starting position in the current search domain
>+// word: prefix of menuItem label looked for
>+// currentDomain: current search domain
>+// result: builded search domain if currentDomain has been reduced
>+bool PopupMenu::buildDomain(GList* startingFrom, const std::string& word,
GList* currentDomain, GList*& result)
>+{
>+    ASSERT(startingFrom);
>+
>+    std::string label;
>+    GList* newDomain = 0;
>+    unsigned int size = g_list_length(startingFrom);
>+
>+    for (unsigned int i = g_list_index(startingFrom, startingFrom->data); i <
size; ++i) {
>+	  label = gtk_menu_item_get_label(GTK_MENU_ITEM(startingFrom->data));
>+	  if (!label.empty())
>+	      label = pango_trim_string(label.c_str());
>+	  if (label.size() > word.size())
>+	      label.erase(word.size());
>+
>+	  if ((label.c_str() && word.c_str()) && !strcasecmp(label.c_str(),
word.c_str()))
>+	      newDomain = g_list_prepend(newDomain, startingFrom->data);
>+	  startingFrom = g_list_next(startingFrom);
>+    }

All the comments for the previous method apply.

>+
>+    if (g_list_length(newDomain) < g_list_length(currentDomain)) {
>+	  result = g_list_reverse(newDomain);

Mmm, why do we need to reverse it?

>+	  return true;
>+    }
>+
>+    return false;
>+}
>+
>+// Search the word typed on the keyboard in the PopupMenu and select the
>+// correspnding menuItem if found

typo in 'correspnding'.