[webkit-reviews] review denied: [Bug 117631] [GTK] Provide search functionality to MiniBrowser : [Attachment 205085] Patch
bugzilla-daemon at webkit.org
bugzilla-daemon at webkit.org
Mon Jul 15 00:41:07 PDT 2013
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 205085: Patch
https://bugs.webkit.org/attachment.cgi?id=205085&action=review
------- Additional Comments from Carlos Garcia Campos <cgarcia at igalia.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=205085&action=review
Looks good to me in general, there are just a few minor details. Thanks!
> Tools/MiniBrowser/gtk/BrowserSearchBar.c:60
> +static void searchEntryIconReleasedCallback(BrowserSearchBar *searchBar)
I would call this searchEntryClearIconReleasedCallback to make more obvious
this is the callback of the clear icon.
> Tools/MiniBrowser/gtk/BrowserSearchBar.c:146
> +
There's an extra line here.
> Tools/MiniBrowser/gtk/BrowserSearchBar.h:52
> +void browser_search_bar_search_next(BrowserSearchBar *);
> +void browser_search_bar_search_previous(BrowserSearchBar *);
These are not used outside BrowserSearchBar, make them private.
> Tools/MiniBrowser/gtk/BrowserWindow.c:310
> + browser_search_bar_close(BROWSER_SEARCH_BAR((window->searchBar)));
Extra parentheses there. BROWSER_SEARCH_BAR((
> Tools/MiniBrowser/gtk/BrowserWindow.c:324
> + browser_search_bar_open(BROWSER_SEARCH_BAR((window->searchBar)));
And here too
> Tools/MiniBrowser/gtk/BrowserWindow.c:583
> + window->searchBarVisible = FALSE;
You don't need this, this is already FALSE at this point.
> Tools/MiniBrowser/gtk/BrowserWindow.c:627
> + window->searchBar = GTK_WIDGET(browser_search_bar_new(window->webView));
browser_search_bar_new returns a GtkWidget *, you don't need the cast.
More information about the webkit-reviews
mailing list