[Webkit-unassigned] [Bug 122681] [GTK] Search functionality in MiniBrowser provides feedback on search fail

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=122681


Mario Sanchez Prada <mario at webkit.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
 Attachment #217799|review?                     |review-
               Flag|                            |




--- Comment #5 from Mario Sanchez Prada <mario at webkit.org>  2013-11-25 06:13:59 PST ---
(From update of attachment 217799)
View in context: https://bugs.webkit.org/attachment.cgi?id=217799&action=review

Sorry Andres not to have catched the following issues in the previous review. Looks good anyway, just "one more thing"...

> Tools/MiniBrowser/gtk/BrowserSearchBar.c:35
> +    GtkCssProvider *cssProvider;
> +    const gchar* failStyle;

Wrong * location for failStyle.

Also, I think that we normally use char instead of gchar for internal variables

> Tools/MiniBrowser/gtk/BrowserSearchBar.c:51
> +    if (failedSearch)
> +        gtk_css_provider_load_from_data(searchBar->cssProvider, searchBar->failStyle, -1, NULL);
> +    else
> +        gtk_css_provider_load_from_data(searchBar->cssProvider, "", -1, NULL);

That's more of a personal preference I think, but I'd use the ternary operator here

> Tools/MiniBrowser/gtk/BrowserSearchBar.c:169
> +    searchBar->failStyle = " GtkEntry#searchEntry {\n"
> +        "   background-color: #ff6666;\n"
> +        "}\n";

What about putting this string in one line only (no \n) and moving its definition to a global const (static const char*) up in this file, right after the priv struct?

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.