Bug 71568 – [GTK] Add a way to change web view settings in MiniBrowser

• Status: RESOLVED
• Resolution: FIXED
• Component: Tools / Tests

[Carlos Garcia Campos]

GtkLauncher uses command line options, but I thnk it would be better to be able to change settings while mini browser is running.

[Carlos Garcia Campos]

Created attachment 113675 [details]
Patch

[WebKit Review Bot]

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.

[Gustavo Noronha (kov)]

Comment on attachment 113675 [details]
Patch

Attachment 113675 [details] did not pass gtk-ews (gtk):
Output: http://queues.webkit.org/results/10333016

[Carlos Garcia Campos]

hmm, G_VALUE_INIT was added in glib 2.30.

[Carlos Garcia Campos]

Created attachment 114501 [details]
Updated patch to not use G_VALUE_INIT

[WebKit Review Bot]

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.

[Carlos Garcia Campos]

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.

[WebKit Review Bot]

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.

[Philippe Normand]

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?

[Carlos Garcia Campos]

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!

[Philippe Normand]

(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() :)

[Carlos Garcia Campos]

(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.

[Carlos Garcia Campos]

Created attachment 120874 [details]
Updated patch according to review comments

[WebKit Review Bot]

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.

[Philippe Normand]

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

[Carlos Garcia Campos]

(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.

[Carlos Garcia Campos]

Committed r103906: <http://trac.webkit.org/changeset/103906>