GtkLauncher uses command line options, but I thnk it would be better to be able to change settings while mini browser is running.
Created attachment 113675 [details] Patch
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 on attachment 113675 [details] Patch Attachment 113675 [details] did not pass gtk-ews (gtk): Output: http://queues.webkit.org/results/10333016
hmm, G_VALUE_INIT was added in glib 2.30.
Created attachment 114501 [details] Updated patch to not use G_VALUE_INIT
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.
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.
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 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 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!
(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() :)
(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.
Created attachment 120874 [details] Updated patch according to review comments
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 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
(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.
Committed r103906: <http://trac.webkit.org/changeset/103906>