Bug 117631

Created attachment 204686 [details] Patch

Attachment 204686 [details] did not pass style-queue: Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Tools/ChangeLog', u'Tools/MiniBrowser/gtk/BrowserSearchBar.c', u'Tools/MiniBrowser/gtk/BrowserSearchBar.h', u'Tools/MiniBrowser/gtk/BrowserWindow.c', u'Tools/MiniBrowser/gtk/GNUmakefile.am']" exit_code: 1 Tools/MiniBrowser/gtk/BrowserSearchBar.h:48: Extra space before ( in function call [whitespace/parens] [4] Tools/MiniBrowser/gtk/BrowserSearchBar.c:26: Found header this file implements before WebCore config.h. Should be: config.h, primary header, blank line, and then alphabetically sorted. [build/include_order] [4] Tools/MiniBrowser/gtk/BrowserSearchBar.c:188: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] Tools/MiniBrowser/gtk/BrowserSearchBar.c:189: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] Tools/MiniBrowser/gtk/BrowserSearchBar.c:190: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] Tools/MiniBrowser/gtk/BrowserSearchBar.c:191: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] Tools/MiniBrowser/gtk/BrowserSearchBar.c:192: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] Tools/MiniBrowser/gtk/BrowserSearchBar.c:193: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] Total errors found: 8 in 5 files If any of these errors are false positives, please file a bug against check-webkit-style.

(In reply to comment #2) > Attachment 204686 [details] did not pass style-queue: ... > Tools/MiniBrowser/gtk/BrowserSearchBar.h:48: Extra space before ( in function call [whitespace/parens] [4] GObject signal definition. Without that space will be awful. > Tools/MiniBrowser/gtk/BrowserSearchBar.c:26: Found header this file implements before WebCore config.h. Should be: config.h, primary header, blank line, and then alphabetically sorted. [build/include_order] [4] MiniBrowser doesn't use the config.h. Harmless false positive, not to be reported as a check-webkit-style bug. > Tools/MiniBrowser/gtk/BrowserSearchBar.c:188: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] > Tools/MiniBrowser/gtk/BrowserSearchBar.c:189: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] > Tools/MiniBrowser/gtk/BrowserSearchBar.c:190: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] > Tools/MiniBrowser/gtk/BrowserSearchBar.c:191: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] > Tools/MiniBrowser/gtk/BrowserSearchBar.c:192: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] > Tools/MiniBrowser/gtk/BrowserSearchBar.c:193: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] GObject signal addition following GTK+ style. IMHO, a harmless false positive, not to be reported as a check-webkit-style bug.

Comment on attachment 204686 [details] Patch 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

(In reply to comment #4) > (From update of attachment 204686 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=204686&action=review > > r- mainly because of the coding style > > 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. Totally removed. > > 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 Yeah, I think I was doing too much defensive programming ...

Created attachment 205085 [details] Patch

Attachment 205085 [details] did not pass style-queue: Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Tools/ChangeLog', u'Tools/MiniBrowser/gtk/BrowserSearchBar.c', u'Tools/MiniBrowser/gtk/BrowserSearchBar.h', u'Tools/MiniBrowser/gtk/BrowserWindow.c', u'Tools/MiniBrowser/gtk/GNUmakefile.am']" exit_code: 1 Tools/MiniBrowser/gtk/BrowserSearchBar.c:26: Found header this file implements before WebCore config.h. Should be: config.h, primary header, blank line, and then alphabetically sorted. [build/include_order] [4] Total errors found: 1 in 5 files If any of these errors are false positives, please file a bug against check-webkit-style.

(In reply to comment #7) > Attachment 205085 [details] did not pass style-queue: > > Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Tools/ChangeLog', u'Tools/MiniBrowser/gtk/BrowserSearchBar.c', u'Tools/MiniBrowser/gtk/BrowserSearchBar.h', u'Tools/MiniBrowser/gtk/BrowserWindow.c', u'Tools/MiniBrowser/gtk/GNUmakefile.am']" exit_code: 1 > Tools/MiniBrowser/gtk/BrowserSearchBar.c:26: Found header this file implements before WebCore config.h. Should be: config.h, primary header, blank line, and then alphabetically sorted. [build/include_order] [4] > Total errors found: 1 in 5 files > > > If any of these errors are false positives, please file a bug against check-webkit-style. As I said in comment #3, MiniBrowser doesn't use the config.h. Harmless false positive, not to be reported as a check-webkit-style bug.

Comment on attachment 205085 [details] Patch 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.

(In reply to comment #9) > > 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. I've added now the primary icon so I've kept the callback name. > > Tools/MiniBrowser/gtk/BrowserSearchBar.c:146 > > + > > There's an extra line here. Corrected. > > 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. Done. > > 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 Done. > > Tools/MiniBrowser/gtk/BrowserWindow.c:583 > > + window->searchBarVisible = FALSE; > > You don't need this, this is already FALSE at this point. Done. > > 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. Done.

Created attachment 206667 [details] Patch

Attachment 206667 [details] did not pass style-queue: Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Tools/ChangeLog', u'Tools/MiniBrowser/gtk/BrowserSearchBar.c', u'Tools/MiniBrowser/gtk/BrowserSearchBar.h', u'Tools/MiniBrowser/gtk/BrowserWindow.c', u'Tools/MiniBrowser/gtk/GNUmakefile.am']" exit_code: 1 Tools/MiniBrowser/gtk/BrowserSearchBar.c:26: Found header this file implements before WebCore config.h. Should be: config.h, primary header, blank line, and then alphabetically sorted. [build/include_order] [4] Total errors found: 1 in 5 files If any of these errors are false positives, please file a bug against check-webkit-style.

(In reply to comment #12) > Attachment 206667 [details] did not pass style-queue: > > Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Tools/ChangeLog', u'Tools/MiniBrowser/gtk/BrowserSearchBar.c', u'Tools/MiniBrowser/gtk/BrowserSearchBar.h', u'Tools/MiniBrowser/gtk/BrowserWindow.c', u'Tools/MiniBrowser/gtk/GNUmakefile.am']" exit_code: 1 > Tools/MiniBrowser/gtk/BrowserSearchBar.c:26: Found header this file implements before WebCore config.h. Should be: config.h, primary header, blank line, and then alphabetically sorted. [build/include_order] [4] > Total errors found: 1 in 5 files > > > If any of these errors are false positives, please file a bug against check-webkit-style. See comment #8.

Comment on attachment 206667 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=206667&action=review Looks good, but I've noticed some weird things after trying it out. > Tools/MiniBrowser/gtk/BrowserSearchBar.c:45 > + if (!g_strcmp0(text, "")) > + return; I guess we should finish the search operation in this case. > Tools/MiniBrowser/gtk/BrowserSearchBar.c:77 > + case GTK_ENTRY_ICON_PRIMARY: > + searchNext(searchBar); > + break; This is very weird, what I expect of a primary button like this one is a menu with search options, not just another button to jump to the next search result. So, I would simply remove it, it duplicates the next button functionality and makes the entry more cluttered. > Tools/MiniBrowser/gtk/BrowserSearchBar.c:82 > + gtk_entry_set_text(GTK_ENTRY(searchBar->entry), ""); > + > + WebKitFindController *controller = webkit_web_view_get_find_controller(searchBar->webView); > + webkit_find_controller_search_finish(controller); Do we really need this? I think setting the entry text should be enough, since the changed signal will be emitted > Tools/MiniBrowser/gtk/BrowserSearchBar.c:123 > + GtkWidget *closeButton = gtk_button_new_from_stock(GTK_STOCK_CLOSE); > + gtk_box_pack_start(GTK_BOX(hBox), closeButton, FALSE, FALSE, 0); The find bar is open, but also closed with the button in the toolbar, I would not duplicate the functionality. I agree it's confusing because the button in the toolbar works as a toggle button but it's not a toggle button. So I would either convert the tool button into a toggle button and remove this one or leave this one (using only the icon and moving it to the right of the find bar) and make the tool button do nothing when the find bar is already shown. > Tools/MiniBrowser/gtk/BrowserSearchBar.c:138 > + GtkWidget *prevButton = gtk_button_new_from_stock(GTK_STOCK_GO_BACK); > + gtk_box_pack_start(GTK_BOX(hBox), prevButton, FALSE, FALSE, 0); > + > + GtkWidget *nextButton = gtk_button_new_from_stock(GTK_STOCK_GO_FORWARD); > + gtk_box_pack_start(GTK_BOX(hBox), nextButton, FALSE, FALSE, 0); Using stock icons for these is a bit confusing, you see a huge buttons saying Back/Forward, I would use buttons without labels and up/down icons instead of back/forward.

Comment on attachment 206667 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=206667&action=review > Tools/MiniBrowser/gtk/BrowserSearchBar.c:182 > + gtk_widget_show(GTK_WIDGET(searchBar)); Forgot to mention that it would be nice if the search entry is focused when the search bar is shown

Created attachment 207320 [details] Patch

Attachment 207320 [details] did not pass style-queue: Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Tools/ChangeLog', u'Tools/MiniBrowser/gtk/BrowserSearchBar.c', u'Tools/MiniBrowser/gtk/BrowserSearchBar.h', u'Tools/MiniBrowser/gtk/BrowserWindow.c', u'Tools/MiniBrowser/gtk/GNUmakefile.am']" exit_code: 1 Tools/MiniBrowser/gtk/BrowserSearchBar.c:26: Found header this file implements before WebCore config.h. Should be: config.h, primary header, blank line, and then alphabetically sorted. [build/include_order] [4] Total errors found: 1 in 5 files If any of these errors are false positives, please file a bug against check-webkit-style.

(In reply to comment #14) > > Tools/MiniBrowser/gtk/BrowserSearchBar.c:45 > > + if (!g_strcmp0(text, "")) > > + return; > > I guess we should finish the search operation in this case. Done. > > Tools/MiniBrowser/gtk/BrowserSearchBar.c:77 > > + case GTK_ENTRY_ICON_PRIMARY: > > + searchNext(searchBar); > > + break; > > This is very weird, what I expect of a primary button like this one is a menu with search options, not just another button to jump to the next search result. So, I would simply remove it, it duplicates the next button functionality and makes the entry more cluttered. As I've redesigned the whole thing, now it pops up a context menu. > > Tools/MiniBrowser/gtk/BrowserSearchBar.c:82 > > + gtk_entry_set_text(GTK_ENTRY(searchBar->entry), ""); > > + > > + WebKitFindController *controller = webkit_web_view_get_find_controller(searchBar->webView); > > + webkit_find_controller_search_finish(controller); > > Do we really need this? I think setting the entry text should be enough, since the changed signal will be emitted Done. > > Tools/MiniBrowser/gtk/BrowserSearchBar.c:123 > > + GtkWidget *closeButton = gtk_button_new_from_stock(GTK_STOCK_CLOSE); > > + gtk_box_pack_start(GTK_BOX(hBox), closeButton, FALSE, FALSE, 0); > > The find bar is open, but also closed with the button in the toolbar, I would not duplicate the functionality. I agree it's confusing because the button in the toolbar works as a toggle button but it's not a toggle button. So I would either convert the tool button into a toggle button and remove this one or leave this one (using only the icon and moving it to the right of the find bar) and make the tool button do nothing when the find bar is already shown. Done. > > Tools/MiniBrowser/gtk/BrowserSearchBar.c:138 > > + GtkWidget *prevButton = gtk_button_new_from_stock(GTK_STOCK_GO_BACK); > > + gtk_box_pack_start(GTK_BOX(hBox), prevButton, FALSE, FALSE, 0); > > + > > + GtkWidget *nextButton = gtk_button_new_from_stock(GTK_STOCK_GO_FORWARD); > > + gtk_box_pack_start(GTK_BOX(hBox), nextButton, FALSE, FALSE, 0); > > Using stock icons for these is a bit confusing, you see a huge buttons saying Back/Forward, I would use buttons without labels and up/down icons instead of back/forward. Done. (In reply to comment #15) > > Tools/MiniBrowser/gtk/BrowserSearchBar.c:182 > > + gtk_widget_show(GTK_WIDGET(searchBar)); > > Forgot to mention that it would be nice if the search entry is focused when the search bar is shown Done. --- I've redesigned the UI. Now, the search bar is at the top with the entry in the middle and small up/down buttons and small close button at the right side. The search entry has two icons to clear and to show a pop up menu with extended search options. In addition, I've added several keyboard accelerators.

(In reply to comment #17) > Attachment 207320 [details] did not pass style-queue: > > Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Tools/ChangeLog', u'Tools/MiniBrowser/gtk/BrowserSearchBar.c', u'Tools/MiniBrowser/gtk/BrowserSearchBar.h', u'Tools/MiniBrowser/gtk/BrowserWindow.c', u'Tools/MiniBrowser/gtk/GNUmakefile.am']" exit_code: 1 > Tools/MiniBrowser/gtk/BrowserSearchBar.c:26: Found header this file implements before WebCore config.h. Should be: config.h, primary header, blank line, and then alphabetically sorted. [build/include_order] [4] > Total errors found: 1 in 5 files > > > If any of these errors are false positives, please file a bug against check-webkit-style. See comment #8.

Comment on attachment 207320 [details] Patch 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.

(In reply to comment #20) > > 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. Done. > > 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. Done. > > Tools/MiniBrowser/gtk/BrowserSearchBar.c:83 > > +static void searchEntryIconReleasedCallback(BrowserSearchBar *searchBar, GtkEntryIconPosition iconPos, GdkEvent *event) > > iconPos -> iconPosition. Done. > > 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 Removed, but also split in 2 different callbacks. Now, the Primary button menu is called in "button-press" instead of in "button-release". It is nicer since the menu will appear just in its proper position. > > 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? GtkToolbar was not really providing anything but the bargain of having to add a single toolitem to add the rest of the stuff. a GtkBox was more flexible for other uses of the SearchBar. In any case, as we are only using it in Minibrowser, seems fair just to use a toolbar. Done. > > 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. Done. > > 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. Done. > > 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. Done. > > 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. Done. > > 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. This is not really necessary since the window already holds a reference that is unref-ed on the finalize of the parent class. In any case, let's keep the additional reference until the finalize for extra safety reasons ... > > Tools/MiniBrowser/gtk/BrowserWindow.c:583 > > + window->searchItem = GTK_WIDGET(item); > > Why do we need to keep this pointer? where is it used? Garbage from previous patches ... removed. > > 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 Done. > > 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. My fault when checking Ephy as per what we discussed regarding the style.

Created attachment 208268 [details] Patch

Attachment 208268 [details] did not pass style-queue: Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Tools/ChangeLog', u'Tools/MiniBrowser/gtk/BrowserSearchBar.c', u'Tools/MiniBrowser/gtk/BrowserSearchBar.h', u'Tools/MiniBrowser/gtk/BrowserWindow.c', u'Tools/MiniBrowser/gtk/GNUmakefile.am']" exit_code: 1 Tools/MiniBrowser/gtk/BrowserSearchBar.c:26: Found header this file implements before WebCore config.h. Should be: config.h, primary header, blank line, and then alphabetically sorted. [build/include_order] [4] Total errors found: 1 in 5 files If any of these errors are false positives, please file a bug against check-webkit-style.

Comment on attachment 208268 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=208268&action=review > Tools/MiniBrowser/gtk/BrowserSearchBar.c:50 > + if (failedSearch) > + gtk_style_context_add_provider(gtk_widget_get_style_context(searchBar->entry), GTK_STYLE_PROVIDER(searchBar->cssProvider), GTK_STYLE_PROVIDER_PRIORITY_APPLICATION); > + else > + gtk_style_context_remove_provider(gtk_widget_get_style_context(searchBar->entry), GTK_STYLE_PROVIDER(searchBar->cssProvider)); For some reason this is not working for me. I'm not a theming expert, but I think a better approach would be to use always a custom css provider and load it with different data, instead of having two providers and add/remove them. I think you can use gtk_style_context_reset_widgets after calling gtk_css_provider_load_from_data.

Created attachment 213598 [details] Patch

(In reply to comment #24) > (From update of attachment 208268 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=208268&action=review > > > Tools/MiniBrowser/gtk/BrowserSearchBar.c:50 > > + if (failedSearch) > > + gtk_style_context_add_provider(gtk_widget_get_style_context(searchBar->entry), GTK_STYLE_PROVIDER(searchBar->cssProvider), GTK_STYLE_PROVIDER_PRIORITY_APPLICATION); > > + else > > + gtk_style_context_remove_provider(gtk_widget_get_style_context(searchBar->entry), GTK_STYLE_PROVIDER(searchBar->cssProvider)); > > For some reason this is not working for me. I'm not a theming expert, but I think a better approach would be to use always a custom css provider and load it with different data, instead of having two providers and add/remove them. I think you can use gtk_style_context_reset_widgets after calling gtk_css_provider_load_from_data. ... AFAIK, it should work either way. In any case, I've rebased the patch and removed the styling bits. Let's leave it for a future patch ...

Attachment 213598 [details] did not pass style-queue: Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Tools/ChangeLog', u'Tools/MiniBrowser/gtk/BrowserSearchBar.c', u'Tools/MiniBrowser/gtk/BrowserSearchBar.h', u'Tools/MiniBrowser/gtk/BrowserWindow.c', u'Tools/MiniBrowser/gtk/GNUmakefile.am']" exit_code: 1 Tools/MiniBrowser/gtk/BrowserSearchBar.c:26: Found header this file implements before WebCore config.h. Should be: config.h, primary header, blank line, and then alphabetically sorted. [build/include_order] [4] Total errors found: 1 in 5 files If any of these errors are false positives, please file a bug against check-webkit-style.

Comment on attachment 213598 [details] Patch Clearing flags on attachment: 213598 Committed r157218: <http://trac.webkit.org/changeset/157218>

All reviewed patches have been landed. Closing bug.