[webkit-reviews] review denied: [Bug 117631] [GTK] Provide search functionality to MiniBrowser : [Attachment 206667] Patch

[bugzilla-daemon at webkit.org]

Carlos Garcia Campos <cgarcia at igalia.com> has denied Andres Gomez Garcia
<agomez at igalia.com>'s request for review:
Bug 117631: [GTK] Provide search functionality to MiniBrowser
https://bugs.webkit.org/show_bug.cgi?id=117631

Attachment 206667: Patch
https://bugs.webkit.org/attachment.cgi?id=206667&action=review

------- Additional Comments from Carlos Garcia Campos <cgarcia at igalia.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=206667&action=review


Looks good, but I've noticed some weird things after trying it out.

> Tools/MiniBrowser/gtk/BrowserSearchBar.c:45
> +    if (!g_strcmp0(text, ""))
> +	   return;

I guess we should finish the search operation in this case.

> Tools/MiniBrowser/gtk/BrowserSearchBar.c:77
> +    case GTK_ENTRY_ICON_PRIMARY:
> +	   searchNext(searchBar);
> +	   break;

This is very weird, what I expect of a primary button like this one is a menu
with search options, not just another button to jump to the next search result.
So, I would simply remove it, it duplicates the next button functionality and
makes the entry more cluttered.

> Tools/MiniBrowser/gtk/BrowserSearchBar.c:82
> +	   gtk_entry_set_text(GTK_ENTRY(searchBar->entry), "");
> +
> +	   WebKitFindController *controller =
webkit_web_view_get_find_controller(searchBar->webView);
> +	   webkit_find_controller_search_finish(controller);

Do we really need this? I think setting the entry text should be enough, since
the changed signal will be emitted

> Tools/MiniBrowser/gtk/BrowserSearchBar.c:123
> +    GtkWidget *closeButton = gtk_button_new_from_stock(GTK_STOCK_CLOSE);
> +    gtk_box_pack_start(GTK_BOX(hBox), closeButton, FALSE, FALSE, 0);

The find bar is open, but also closed with the button in the toolbar, I would
not duplicate the functionality. I agree it's confusing because the button in
the toolbar works as a toggle button but it's not a toggle button. So I would
either convert the tool button into a toggle button and remove this one or
leave this one (using only the icon and moving it to the right of the find bar)
and make the tool button do nothing when the find bar is already shown.

> Tools/MiniBrowser/gtk/BrowserSearchBar.c:138
> +    GtkWidget *prevButton = gtk_button_new_from_stock(GTK_STOCK_GO_BACK);
> +    gtk_box_pack_start(GTK_BOX(hBox), prevButton, FALSE, FALSE, 0);
> +
> +    GtkWidget *nextButton = gtk_button_new_from_stock(GTK_STOCK_GO_FORWARD);

> +    gtk_box_pack_start(GTK_BOX(hBox), nextButton, FALSE, FALSE, 0);

Using stock icons for these is a bit confusing, you see a huge buttons saying
Back/Forward, I would use buttons without labels and up/down icons instead of
back/forward.