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

[bugzilla-daemon at webkit.org]

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.