Bug 71568

Summary: [GTK] Add a way to change web view settings in MiniBrowser
Product: WebKit Reporter: Carlos Garcia Campos <cgarcia>
Component: Tools / TestsAssignee: Nobody <webkit-unassigned>
Status: RESOLVED FIXED    
Severity: Normal CC: gustavo, pnormand, webkit.review.bot, xan.lopez
Priority: P2 Keywords: Gtk
Version: 528+ (Nightly build)   
Hardware: PC   
OS: Linux   
Attachments:
Description Flags
Patch
gustavo: commit-queue-
Updated patch to not use G_VALUE_INIT
none
Updated patch
none
Updated patch according to review comments pnormand: review+

Description Carlos Garcia Campos 2011-11-04 10:19:45 PDT 
GtkLauncher uses command line options, but I thnk it would be better to be able to change settings while mini browser is running.
Comment 1 Carlos Garcia Campos 2011-11-04 10:27:34 PDT 
Created attachment 113675 [details]
Patch
Comment 2 WebKit Review Bot 2011-11-04 10:31:14 PDT 
Attachment 113675 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'ChangeLog', u'GNUmakefile.am', u'Tools/Cha..." exit_code: 1

Tools/MiniBrowser/gtk/BrowserSettingsDialog.h:46:  The parameter name "settings" adds no information, so it should be removed.  [readability/parameter_name] [5]
Tools/MiniBrowser/gtk/BrowserSettingsDialog.c:26:  Found header this file implements before WebCore config.h. Should be: config.h, primary header, blank line, and then alphabetically sorted.  [build/include_order] [4]
Tools/MiniBrowser/gtk/BrowserCellRendererVariant.c:26:  Found header this file implements before WebCore config.h. Should be: config.h, primary header, blank line, and then alphabetically sorted.  [build/include_order] [4]
Total errors found: 3 in 10 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 3 Gustavo Noronha (kov) 2011-11-04 10:38:02 PDT 
Comment on attachment 113675 [details]
Patch

Attachment 113675 [details] did not pass gtk-ews (gtk):
Output: http://queues.webkit.org/results/10333016
Comment 4 Carlos Garcia Campos 2011-11-04 11:13:06 PDT 
hmm, G_VALUE_INIT was added in glib 2.30.
Comment 5 Carlos Garcia Campos 2011-11-10 08:00:39 PST 
Created attachment 114501 [details]
Updated patch to not use G_VALUE_INIT
Comment 6 WebKit Review Bot 2011-11-10 08:13:31 PST 
Attachment 114501 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'ChangeLog', u'GNUmakefile.am', u'Tools/Cha..." exit_code: 1

Tools/MiniBrowser/gtk/BrowserSettingsDialog.h:46:  The parameter name "settings" adds no information, so it should be removed.  [readability/parameter_name] [5]
Tools/MiniBrowser/gtk/BrowserSettingsDialog.c:26:  Found header this file implements before WebCore config.h. Should be: config.h, primary header, blank line, and then alphabetically sorted.  [build/include_order] [4]
Tools/MiniBrowser/gtk/BrowserCellRendererVariant.c:26:  Found header this file implements before WebCore config.h. Should be: config.h, primary header, blank line, and then alphabetically sorted.  [build/include_order] [4]
Total errors found: 3 in 10 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 7 Carlos Garcia Campos 2011-12-28 06:38:05 PST 
Created attachment 120663 [details]
Updated patch

Several fixes in this new patch:

 - Support all setting types (boolean, string and uint)
 - Fix compile warning with the GValue stack initialization.
 - Use GtkToolItem instead of GtkMenuToolItem for the preferences toolbar button.
 - Use the property nick instead of the property name in the dialog.
 - Show the property blurb as a tooltip.
Comment 8 WebKit Review Bot 2011-12-28 06:48:04 PST 
Attachment 120663 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'ChangeLog', u'GNUmakefile.am', u'Tools/Cha..." exit_code: 1

Tools/MiniBrowser/gtk/BrowserSettingsDialog.h:46:  The parameter name "settings" adds no information, so it should be removed.  [readability/parameter_name] [5]
Tools/MiniBrowser/gtk/BrowserSettingsDialog.c:26:  Found header this file implements before WebCore config.h. Should be: config.h, primary header, blank line, and then alphabetically sorted.  [build/include_order] [4]
Tools/MiniBrowser/gtk/BrowserCellRendererVariant.c:26:  Found header this file implements before WebCore config.h. Should be: config.h, primary header, blank line, and then alphabetically sorted.  [build/include_order] [4]
Total errors found: 3 in 10 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 9 Philippe Normand 2012-01-02 04:23:36 PST 
Comment on attachment 120663 [details]
Updated patch

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

> Tools/MiniBrowser/gtk/BrowserCellRendererVariant.c:65
> +    g_object_unref(renderer->toggleRenderer);

Shouldn't textRenderer and spinRenderer be unreffed here as well?

> Tools/MiniBrowser/gtk/BrowserCellRendererVariant.c:186
> +    if (errno || value > gtk_adjustment_get_upper(adjustment) || value < gtk_adjustment_get_lower(adjustment) || endPtr == newText)

Maybe it'd be good to emit a warning in this case?

> Tools/MiniBrowser/gtk/BrowserCellRendererVariant.c:193
> +static gint browserCellRendererVariantCellRendererActivate(GtkCellRenderer *cell, GdkEvent *event, GtkWidget *widget, const gchar *path, const GdkRectangle *bgArea, const GdkRectangle *cellArea, GtkCellRendererState flags)

Just a nit, but shouldn't the return type be gboolean?
Comment 10 Carlos Garcia Campos 2012-01-02 04:30:21 PST 
Comment on attachment 120663 [details]
Updated patch

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

>> Tools/MiniBrowser/gtk/BrowserCellRendererVariant.c:65
>> +    g_object_unref(renderer->toggleRenderer);
> 
> Shouldn't textRenderer and spinRenderer be unreffed here as well?

Indeed.

>> Tools/MiniBrowser/gtk/BrowserCellRendererVariant.c:186
>> +    if (errno || value > gtk_adjustment_get_upper(adjustment) || value < gtk_adjustment_get_lower(adjustment) || endPtr == newText)
> 
> Maybe it'd be good to emit a warning in this case?

You mean g_warning()?

>> Tools/MiniBrowser/gtk/BrowserCellRendererVariant.c:193
>> +static gint browserCellRendererVariantCellRendererActivate(GtkCellRenderer *cell, GdkEvent *event, GtkWidget *widget, const gchar *path, const GdkRectangle *bgArea, const GdkRectangle *cellArea, GtkCellRendererState flags)
> 
> Just a nit, but shouldn't the return type be gboolean?

Right, I don't know why I used gint there :-P Thanks!
Comment 11 Philippe Normand 2012-01-02 04:45:13 PST 
(In reply to comment #10)

> >> Tools/MiniBrowser/gtk/BrowserCellRendererVariant.c:186
> >> +    if (errno || value > gtk_adjustment_get_upper(adjustment) || value < gtk_adjustment_get_lower(adjustment) || endPtr == newText)
> > 
> > Maybe it'd be good to emit a warning in this case?
> 
> You mean g_warning()?
> 

Yes I was think about g_warning() :)
Comment 12 Carlos Garcia Campos 2012-01-02 05:09:10 PST 
(In reply to comment #11)
> (In reply to comment #10)
> 
> > >> Tools/MiniBrowser/gtk/BrowserCellRendererVariant.c:186
> > >> +    if (errno || value > gtk_adjustment_get_upper(adjustment) || value < gtk_adjustment_get_lower(adjustment) || endPtr == newText)
> > > 
> > > Maybe it'd be good to emit a warning in this case?
> > 
> > You mean g_warning()?
> > 
> 
> Yes I was think about g_warning() :)

Ah, ok, I'll add the g_warning() then.
Comment 13 Carlos Garcia Campos 2012-01-02 05:47:58 PST 
Created attachment 120874 [details]
Updated patch according to review comments
Comment 14 WebKit Review Bot 2012-01-02 05:51:52 PST 
Attachment 120874 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'ChangeLog', u'GNUmakefile.am', u'Tools/Cha..." exit_code: 1

Tools/MiniBrowser/gtk/BrowserSettingsDialog.h:46:  The parameter name "settings" adds no information, so it should be removed.  [readability/parameter_name] [5]
Tools/MiniBrowser/gtk/BrowserSettingsDialog.c:26:  Found header this file implements before WebCore config.h. Should be: config.h, primary header, blank line, and then alphabetically sorted.  [build/include_order] [4]
Tools/MiniBrowser/gtk/BrowserCellRendererVariant.c:26:  Found header this file implements before WebCore config.h. Should be: config.h, primary header, blank line, and then alphabetically sorted.  [build/include_order] [4]
Total errors found: 3 in 10 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 15 Philippe Normand 2012-01-02 06:07:50 PST 
Comment on attachment 120874 [details]
Updated patch according to review comments

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

Thanks! Just a last thing to fix before landing, I didn't notice it during the first review.

> Tools/MiniBrowser/gtk/BrowserSettingsDialog.c:131
> +    GParamSpec **properties = g_object_class_list_properties(G_OBJECT_GET_CLASS(settings), &propertiesCount);

Looks like properties is leaked
Comment 16 Carlos Garcia Campos 2012-01-02 06:53:46 PST 
(In reply to comment #15)
> (From update of attachment 120874 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=120874&action=review
> 
> Thanks! Just a last thing to fix before landing, I didn't notice it during the first review.

No problem! thanks for reviewing it :-)

> > Tools/MiniBrowser/gtk/BrowserSettingsDialog.c:131
> > +    GParamSpec **properties = g_object_class_list_properties(G_OBJECT_GET_CLASS(settings), &propertiesCount);
> 
> Looks like properties is leaked

Yes, good catch, I'll fix it before landing.
Comment 17 Carlos Garcia Campos 2012-01-02 06:56:54 PST 
Committed r103906: <http://trac.webkit.org/changeset/103906>