[Webkit-unassigned] [Bug 117631] [GTK] Provide search functionality to MiniBrowser

[bugzilla-daemon at webkit.org]

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


Carlos Garcia Campos <cgarcia at igalia.com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
 Attachment #204686|review?, commit-queue?      |review-, commit-queue-
               Flag|                            |




--- Comment #4 from Carlos Garcia Campos <cgarcia at igalia.com>  2013-06-19 10:40:45 PST ---
(From update of attachment 204686)
View in context: https://bugs.webkit.org/attachment.cgi?id=204686&action=review

r- mainly because of the coding style

> Tools/MiniBrowser/gtk/BrowserSearchBar.c:29
> +#define BROWSER_SEARCH_BAR_GET_PRIVATE(object) (G_TYPE_INSTANCE_GET_PRIVATE((object), BROWSER_TYPE_SEARCH_BAR, BrowserSearchBarPrivate))

Don't do this, call G_TYPE_INSTANCE_GET_PRIVATE directly in _init since it's the only place where this is used.

> Tools/MiniBrowser/gtk/BrowserSearchBar.c:41
> +    LastSignal

We don't follow the style for the signals enum, use CLOSE, LAST_SIGNAL

> Tools/MiniBrowser/gtk/BrowserSearchBar.c:51
> +struct _BrowserSearchBar {
> +    GtkBox parent;
> +
> +    /*< private >*/
> +    BrowserSearchBarPrivate *priv;
> +};

Since you are defining the struct in the c file and nobody is going to inherit from this object, you can get rid of the private struct.

> Tools/MiniBrowser/gtk/BrowserSearchBar.c:56
> +/* private functions */

Comments should start with capital and finish with period, but I would just remove this comment here.

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

this should be doSearch, use the wk coding style for non-generated private methods.

> Tools/MiniBrowser/gtk/BrowserSearchBar.c:67
> +    WebKitFindOptions options = WEBKIT_FIND_OPTIONS_NONE;

This is unused if you return early in the if below, so move this after it.

> Tools/MiniBrowser/gtk/BrowserSearchBar.c:80
> +static void search_close_button_clicked_cb(GtkButton *button, BrowserSearchBar *searchBar)

Wrong coding style here too, and the same for all other private functions that are not generrated. Also use Callback instead of the abbreviation cb

> Tools/MiniBrowser/gtk/BrowserSearchBar.c:85
> +static void search_entry_icon_released_cb(GtkEntry *entry, GtkEntryIconPosition icon_pos, GdkEvent  *event, BrowserSearchBar *searchBar)

icon_pos ->iconPosition, or simply omit it if it's unused. Extra space in GdkEvent  *event.

> Tools/MiniBrowser/gtk/BrowserSearchBar.c:117
> +static void search_entry_activated_cb(GtkEntry *entry, BrowserSearchBar *searchBar)
> +{
> +    browser_search_bar_search_next(searchBar);
> +}
> +
> +static void search_prev_button_clicked_cb(GtkButton *button, BrowserSearchBar *searchBar)
> +{
> +    browser_search_bar_search_previous(searchBar);
> +}
> +
> +static void search_next_button_clicked_cb(GtkButton *button, BrowserSearchBar *searchBar)
> +{
> +    browser_search_bar_search_next(searchBar);
> +}

You can probably avoid all these using g_signal_connect_swapped and passing browser_search_bar_search_next/previous as callbacks.

> Tools/MiniBrowser/gtk/BrowserSearchBar.c:130
> +    GtkWidget *hBox;
> +    GtkWidget *closeButton;
> +    GtkWidget *label;
> +    GtkWidget *prevButton;
> +    GtkWidget *nextButton;

Do not add a declarations block.

> Tools/MiniBrowser/gtk/BrowserSearchBar.c:199
> +/* public functions */

Same comment about the comment.

> Tools/MiniBrowser/gtk/BrowserSearchBar.c:208
> +    g_weak_ref_init(&priv->weak, controller);

Why using this weak reaf instead of just getting a reference? I think you don't even need to keep a reference, since the find controller will always be available while the web view is alive

> Tools/MiniBrowser/gtk/BrowserWindow.c:477
> +        browser_search_bar_request_close(BROWSER_SEARCH_BAR((window->searchBar)));

Extra parentheses there in BROWSER_SEARCH_BAR((window->searchBar)). Why don't you simply close the search bar here? this calls request_close, that emits close signal, that ends up calling searchBarCloseCallback that calls browser_search_bar_close(). It seems to me that we can just call browser_search_bar_close

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