[Webkit-unassigned] [Bug 117631] [GTK] Provide search functionality to MiniBrowser
bugzilla-daemon at webkit.org
bugzilla-daemon at webkit.org
Wed Aug 7 01:27:43 PDT 2013
https://bugs.webkit.org/show_bug.cgi?id=117631
--- Comment #21 from Andres Gomez Garcia <agomez at igalia.com> 2013-08-07 01:27:23 PST ---
(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.
--
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