Bug 122681 - [GTK] Search functionality in MiniBrowser provides feedback on search fail
Summary: [GTK] Search functionality in MiniBrowser provides feedback on search fail
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Tools / Tests (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2013-10-11 15:52 PDT by Andres Gomez Garcia
Modified: 2013-11-25 16:18 PST (History)
4 users (show)

See Also:


Attachments
Patch (5.28 KB, patch)
2013-10-11 16:14 PDT, Andres Gomez Garcia
no flags Details | Formatted Diff | Diff
Patch (5.21 KB, patch)
2013-11-25 05:54 PST, Andres Gomez Garcia
no flags Details | Formatted Diff | Diff
Patch (5.08 KB, patch)
2013-11-25 10:36 PST, Andres Gomez Garcia
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Andres Gomez Garcia 2013-10-11 15:52:16 PDT
When searching in MiniBrowser, if the search is failing, show some kind of feedback. Useful for testing bugs.
Comment 1 Andres Gomez Garcia 2013-10-11 16:14:36 PDT
Created attachment 214034 [details]
Patch
Comment 2 Mario Sanchez Prada 2013-11-25 04:12:45 PST
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 :)
Comment 3 Andres Gomez Garcia 2013-11-25 05:54:54 PST
Created attachment 217799 [details]
Patch
Comment 4 Andres Gomez Garcia 2013-11-25 05:56:42 PST
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.
Comment 5 Mario Sanchez Prada 2013-11-25 06:13:59 PST
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?
Comment 6 Mario Sanchez Prada 2013-11-25 06:19:27 PST
(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 :)
Comment 7 Andres Gomez Garcia 2013-11-25 10:36:48 PST
Created attachment 217817 [details]
Patch
Comment 8 Andres Gomez Garcia 2013-11-25 10:38:14 PST
(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.
Comment 9 Mario Sanchez Prada 2013-11-25 15:50:19 PST
Comment on attachment 217817 [details]
Patch

lgtm
Comment 10 WebKit Commit Bot 2013-11-25 16:18:33 PST
Comment on attachment 217817 [details]
Patch

Clearing flags on attachment: 217817

Committed r159774: <http://trac.webkit.org/changeset/159774>
Comment 11 WebKit Commit Bot 2013-11-25 16:18:35 PST
All reviewed patches have been landed.  Closing bug.