[Webkit-unassigned] [Bug 117631] [GTK] Provide search functionality to MiniBrowser
bugzilla-daemon at webkit.org
bugzilla-daemon at webkit.org
Fri Jul 26 00:40:55 PDT 2013
https://bugs.webkit.org/show_bug.cgi?id=117631
Carlos Garcia Campos <cgarcia at igalia.com> changed:
What |Removed |Added
----------------------------------------------------------------------------
Attachment #207320|review?, commit-queue? |review-, commit-queue-
Flag| |
--- Comment #20 from Carlos Garcia Campos <cgarcia at igalia.com> 2013-07-26 00:40:45 PST ---
(From update of attachment 207320)
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.
--
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.
More information about the webkit-unassigned
mailing list