Bug 71568 - [GTK] Add a way to change web view settings in MiniBrowser
Summary: [GTK] Add a way to change web view settings in MiniBrowser
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Tools / Tests (show other bugs)
Version: 528+ (Nightly build)
Hardware: PC Linux
: P2 Normal
Assignee: Nobody
URL:
Keywords: Gtk
Depends on:
Blocks:
 
Reported: 2011-11-04 10:19 PDT by Carlos Garcia Campos
Modified: 2012-01-02 06:56 PST (History)
4 users (show)

See Also:


Attachments
Patch (27.95 KB, patch)
2011-11-04 10:27 PDT, Carlos Garcia Campos
gns: commit-queue-
Details | Formatted Diff | Diff
Updated patch to not use G_VALUE_INIT (27.88 KB, patch)
2011-11-10 08:00 PST, Carlos Garcia Campos
no flags Details | Formatted Diff | Diff
Updated patch (35.28 KB, patch)
2011-12-28 06:38 PST, Carlos Garcia Campos
no flags Details | Formatted Diff | Diff
Updated patch according to review comments (35.56 KB, patch)
2012-01-02 05:47 PST, Carlos Garcia Campos
pnormand: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
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>