[webkit-reviews] review denied: [Bug 117631] [GTK] Provide search functionality to MiniBrowser : [Attachment 207320] 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 207320: Patch
https://bugs.webkit.org/attachment.cgi?id=207320&action=review

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


Search bar look a lot nicer now, but I think it should be packed after the main
toolbar. There are some other minor issues to fix, we are pretty close, thanks!


> Tools/MiniBrowser/gtk/BrowserSearchBar.c:48
> +    if (!g_strcmp0(text, "")) {

You can use gtk_entry_get_text_length here to know if there's something and
only get the text below when needed.

> Tools/MiniBrowser/gtk/BrowserSearchBar.c:69
> +    WebKitFindController *controller =
webkit_web_view_get_find_controller(searchBar->webView);
> +    webkit_find_controller_search_next(controller);

This could be a single line

> Tools/MiniBrowser/gtk/BrowserSearchBar.c:75
> +    WebKitFindController *controller =
webkit_web_view_get_find_controller(searchBar->webView);
> +    webkit_find_controller_search_previous(controller);

Ditto.

> Tools/MiniBrowser/gtk/BrowserSearchBar.c:83
> +static void searchEntryIconReleasedCallback(BrowserSearchBar *searchBar,
GtkEntryIconPosition iconPos, GdkEvent *event)

iconPos -> iconPosition.

> Tools/MiniBrowser/gtk/BrowserSearchBar.c:87
> +	   if (event->type == GDK_BUTTON_RELEASE) {

This is always TRUE, this signal is emitted only on button-release-event

> Tools/MiniBrowser/gtk/BrowserSearchBar.c:130
> +   
gtk_style_context_add_class(gtk_widget_get_style_context(GTK_WIDGET(searchBar))
, GTK_STYLE_CLASS_TOOLBAR);

Why not inheriting from GtkToolbar instead of GtkBox + Toolbar style class?

> Tools/MiniBrowser/gtk/BrowserSearchBar.c:151
> +    gtk_button_set_relief(GTK_BUTTON(searchBar->prevButton),
GTK_RELIEF_NONE);

You should also make this button not focusable, so that current focused widget
doesn't lose the focus when clicking this buttons. Use
gtk_button_set_focus_on_click

> Tools/MiniBrowser/gtk/BrowserSearchBar.c:157
> +    gtk_button_set_relief(GTK_BUTTON(searchBar->nextButton),
GTK_RELIEF_NONE);

Ditto.

> Tools/MiniBrowser/gtk/BrowserSearchBar.c:194
> +    g_object_unref(BROWSER_SEARCH_BAR(object)->webView);
> +    g_object_unref(BROWSER_SEARCH_BAR(object)->optionsMenu);

Don't do the cast twice, use a local variable.

> Tools/MiniBrowser/gtk/BrowserSearchBar.c:218
> +    g_return_if_fail(searchBar);

Use BROWSER_IS_SEARCH_BAR

> Tools/MiniBrowser/gtk/BrowserSearchBar.c:230
> +    g_return_if_fail(searchBar);

Ditto.

> Tools/MiniBrowser/gtk/BrowserSearchBar.c:239
> +    g_return_if_fail(searchBar);

Ditto.

> Tools/MiniBrowser/gtk/BrowserWindow.c:483
> +    if (window->searchBarVisible)
> +	   browser_search_bar_open(BROWSER_SEARCH_BAR(window->searchBar));

I think you can call open unconditionally, so that if the search bar is already
open, the entry will be focused.

> Tools/MiniBrowser/gtk/BrowserWindow.c:544
> +    g_object_unref(window->accelGroup);

Now that the accel group is used out of this function, I don't think it's a
good idea to release your reference so early, you should do it in the browser
window dispose/finalize.

> Tools/MiniBrowser/gtk/BrowserWindow.c:583
> +    window->searchItem = GTK_WIDGET(item);

Why do we need to keep this pointer? where is it used?

> Tools/MiniBrowser/gtk/BrowserWindow.c:633
> +    browser_search_bar_add_accelerators(window->searchBar,
window->accelGroup);

You need a cast here, since window->searchBar is a GtkWidget and
browser_search_bar_add_accelerators expects a BrowserSearchBar

> Tools/MiniBrowser/gtk/BrowserWindow.c:635
> +    gtk_box_reorder_child(GTK_BOX(window->mainBox), window->searchBar, 0);

Why showing the search bar over the main toolbar instead of below? It looks
really weird.