[Webkit-unassigned] [Bug 128459] [GTK] Minibrowser: On opening search bar, search for selected text if any

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=128459





--- Comment #2 from Andres Gomez Garcia <agomez at igalia.com>  2014-03-24 07:45:44 PST ---
(From update of attachment 223572)
View in context: https://bugs.webkit.org/attachment.cgi?id=223572&action=review

> Tools/MiniBrowser/gtk/BrowserSearchBar.c:275
> +static void open_search_bar(BrowserSearchBar *searchBar)

Non public API follows WK's C++ style. Hence, the name should be something like openSearchBar.

> Tools/MiniBrowser/gtk/BrowserSearchBar.c:286
> +static void get_selected_text_cb(GObject *object, GAsyncResult *result, gpointer userData)

Ditto.

In addition, the outcome of this cb is to open the search bar regardless to the existence of selected text. Hence, the name of the function is actually misleading.

> Tools/MiniBrowser/gtk/BrowserSearchBar.c:293
> +

As this should be C++ style, only declare your variables when using, and not sooner.

> Tools/MiniBrowser/gtk/BrowserSearchBar.c:298
> +        g_error_free(error);

Don't we do anything with the error?

> Tools/MiniBrowser/gtk/BrowserSearchBar.c:310
> +

Same here, don't declare your variables in advance.

> Tools/MiniBrowser/gtk/BrowserSearchBar.c:324
> +    open_search_bar(searchBar);

Just wondering if it wouldn't be better to change the "open_search_bar" API to have a second parameter with the text to set in the entry, instead of doing that here.

> Tools/MiniBrowser/gtk/BrowserSearchBar.c:330
> +    gchar *script;

Do not declare in advance.

> Tools/MiniBrowser/gtk/BrowserSearchBar.c:340
> +    open_search_bar_and_search_selected_text(searchBar);

I would place here the content of this function and remove it completely. It is an unneeded indirection.

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.