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

• Status: NEW
• Component: WebKitGTK

[Diego Pino]

In all major most browsers, selecting a text and opening the search bar immediately searches for that string in the current page. Implement that functionality in Minibrowser.

[Diego Pino]

Created attachment 223572 [details]
Patch

[Andres Gomez Garcia]

Comment on attachment 223572 [details]
Patch

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.

[Michael Catanzaro]

Comment on attachment 223572 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=223572&action=review

Thanks for the review, Berto. Updating the flags to make it official and remove this from request queue....

> Tools/MiniBrowser/gtk/BrowserSearchBar.c:322
> +        g_free(selectedText);

Surely you need to free selectedText even if it's zero-length, since you allocated the memory.

[Diego Pino]

I'm afraid that I won't be able to work on this patch in the short/medium term. If anyone would like to take it over I'm happy with that :)