Bug 122681

Summary: [GTK] Search functionality in MiniBrowser provides feedback on search fail
Product: WebKit Reporter: Andres Gomez Garcia <agomez>
Component: Tools / TestsAssignee: Nobody <webkit-unassigned>
Status: RESOLVED FIXED    
Severity: Normal CC: agomez, cgarcia, commit-queue, mario
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Patch
none
Patch
none
Patch none

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.