Bug 122681 – [GTK] Search functionality in MiniBrowser provides feedback on search fail

• Component: Tools / Tests
• Status: RESOLVED FIXED

[Andres Gomez Garcia]

When searching in MiniBrowser, if the search is failing, show some kind of feedback. Useful for testing bugs.

[Andres Gomez Garcia]

Created attachment 214034 [details]
Patch

[Mario Sanchez Prada]

Comment on attachment 214034 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=214034&action=review

I think it's a good idea, I would just do a small change in the code. See it below...

> Tools/MiniBrowser/gtk/BrowserSearchBar.c:46
> +static void setEntryBackgroundColor(BrowserSearchBar *searchBar, gboolean failedSearch)

This function is named setEntryBackgroundColor(), but it does not set any color at all, just specifies whether the search failed (so the color is set accordingly).

So, I think you either rename it to something that makes more sense according to the bool parameter (e.g. setFailedStyleForEntry()), so the bool TRUE means "failed" and FALSE means "ok/default", or you go further to even avoid the bool parameter by declaring a new enum with ENTRY_STYLE_DEFAULT/OK/FAIL values (for instance) to be passed to the function, which maybe I would rename then to setStyleForEntry().

For MiniBrowser, I do not have any special preference over those. Any option would be ok to me :)

[Andres Gomez Garcia]

Created attachment 217799 [details]
Patch

[Andres Gomez Garcia]

Thanks for the review!

(In reply to comment #2)
...
> So, I think you either rename it to something that makes more sense according to the bool parameter (e.g. setFailedStyleForEntry()), so the bool TRUE means "failed" and FALSE means 

Done.

[Mario Sanchez Prada]

Comment on attachment 217799 [details]
Patch

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?

[Mario Sanchez Prada]

(In reply to comment #5)
> [...]
> > 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?

Actually, I just realized that you don't need to keep the const char for failStyle in the private struct at all if you follow this advice :)

[Andres Gomez Garcia]

Created attachment 217817 [details]
Patch

[Andres Gomez Garcia]

(In reply to comment #5)
> > 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);

Done.

> 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?

Done.

(In reply to comment #6)
...
> Actually, I just realized that you don't need to keep the const char for failStyle in the private struct at all if you follow this advice :)

Done.

[Mario Sanchez Prada]

Comment on attachment 217817 [details]
Patch

lgtm

[WebKit Commit Bot]

Comment on attachment 217817 [details]
Patch

Clearing flags on attachment: 217817

Committed r159774: <http://trac.webkit.org/changeset/159774>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.