Bug 17375 - [Gtk] Set user-agent from application
: [Gtk] Set user-agent from application
Status: RESOLVED FIXED
: WebKit
WebKit Gtk
: 528+ (Nightly build)
: PC Mac OS X 10.5
: P2 Normal
Assigned To:
:
: Gtk
:
:
  Show dependency treegraph
 
Reported: 2008-02-15 08:00 PST by
Modified: 2009-07-03 02:58 PST (History)


Attachments
Really old patch (4.01 KB, patch)
2008-08-08 10:49 PST, Sven Herzberg
no flags Review Patch | Details | Formatted Diff | Diff
Implement "user-agent" web settings property (8.89 KB, patch)
2009-01-24 19:37 PST, Christian Dywan
no flags Review Patch | Details | Formatted Diff | Diff
Add WebView APIs to manipulate the UA string (21.90 KB, patch)
2009-05-19 06:28 PST, Jan Alonzo
no flags Review Patch | Details | Formatted Diff | Diff
WebView API methods to get/set global "user-agent" property using Soup Session (4.88 KB, patch)
2009-06-02 16:45 PST, Edgar Acosta
no flags Review Patch | Details | Formatted Diff | Diff
Add webview property to get UA string + refactor UA version string in configure.ac (16.18 KB, patch)
2009-06-08 00:54 PST, Jan Alonzo
no flags Review Patch | Details | Formatted Diff | Diff
move the user-agent property from the view to the settings (18.82 KB, patch)
2009-06-20 02:09 PST, Jan Alonzo
no flags Review Patch | Details | Formatted Diff | Diff
updated per Christian's feedback (19.83 KB, patch)
2009-06-22 03:35 PST, Jan Alonzo
no flags Review Patch | Details | Formatted Diff | Diff
updated per xan's feedback, converted CString to gchar (19.73 KB, patch)
2009-06-26 00:56 PST, Jan Alonzo
no flags Review Patch | Details | Formatted Diff | Diff
updated patch (19.75 KB, patch)
2009-06-30 05:42 PST, Jan Alonzo
no flags Review Patch | Details | Formatted Diff | Diff
updated patch with empty string test (20.01 KB, patch)
2009-06-30 05:45 PST, Jan Alonzo
no flags Review Patch | Details | Formatted Diff | Diff
updated patch with some test and doc fixes (20.02 KB, patch)
2009-06-30 06:03 PST, Jan Alonzo
no flags Review Patch | Details | Formatted Diff | Diff
proper fix to the test + version bump to 531.2 (20.06 KB, patch)
2009-06-30 06:36 PST, Jan Alonzo
xan.lopez: review+
Review Patch | Details | Formatted Diff | Diff


Note

You need to log in before you can comment on or make changes to this bug.


Description From 2008-02-15 08:00:43 PST
Some applications want to set the user-agent manually. Here's a patch that allows this.
------- Comment #1 From 2008-02-18 10:25:58 PST -------
(In reply to comment #0)
> Some applications want to set the user-agent manually. Here's a patch that
> allows this.
> 

Think you forgot the patch.
------- Comment #2 From 2008-08-08 10:49:12 PST -------
Created an attachment (id=22707) [details]
Really old patch

Here's the really old patch (rev 30190) :-)

It was originally written before the settings stuff was in place, so it requires quite some modifications to fit into the current API.
------- Comment #3 From 2009-01-24 19:37:40 PST -------
Created an attachment (id=27004) [details]
Implement "user-agent" web settings property

This is a patch that implements "user-agent" as a new property of the web settings. It is probably not quite done, for instance I didn't check whether the string also needs to be applied on frames or whether the frame loader client is valid when it is set.

I patched GtkLauncher to verify that the new feature works correctly. I would leave that part out in the commit.

If we agree on adding this I can look into the mentioned open issues.
------- Comment #4 From 2009-01-25 11:53:00 PST -------
I quite like the idea. I think it would be a good thing to return the default User-Agent when the property is queried and nothing was set, though. This would allow easy 'appending/messing' with the otherwise default U-A, which might be needed by some apps.
------- Comment #5 From 2009-01-25 13:57:33 PST -------
(In reply to comment #4)
> I quite like the idea. I think it would be a good thing to return the default
> User-Agent when the property is queried and nothing was set, though. This would
> allow easy 'appending/messing' with the otherwise default U-A, which might be
> needed by some apps.

I think it's useful that NULL, ie. the default, signifies the default string, since it allows applications to change the string and reset it afterwards. More importantly if an application is interested in the string, modifying the default is bound to be very hairy and not something I would like to support.

Instead we *might* introduce a function that creates a user agent, however that is not exactly straightforward. Consider strings for robots, browsers, email clients, feed readers, libraries... the browsers disguising as Mozilla are the only really hard ones, though, so probably only a function for that is needed.
------- Comment #6 From 2009-01-26 05:51:57 PST -------
(From update of attachment 27004 [details])
> diff --git a/WebCore/loader/FrameLoaderClient.h b/WebCore/loader/FrameLoaderClient.h
> index b90cecd..f4b9861 100644
> --- a/WebCore/loader/FrameLoaderClient.h
> +++ b/WebCore/loader/FrameLoaderClient.h
> @@ -179,6 +179,9 @@ namespace WebCore {
>          virtual PassRefPtr<DocumentLoader> createDocumentLoader(const ResourceRequest&, const SubstituteData&) = 0;
>          virtual void setTitle(const String& title, const KURL&) = 0;
>  
> +#if PLATFORM(GTK)
> +        virtual void setUserAgent(const String&) = 0;
> +#endif

that is not needed. I would like to keep it out. Also there is no need to make it virtual.




> +        virtual void setUserAgent(const WebCore::String&);

just plain void setUserAgent.



>  
> +static void webViewSetUserAgent(WebKitWebView* webView, const gchar* userAgent)
> +{
> +    /* FIXME: Do this on all frames */
> +    /* FIXME: Should we check if loader and client are valid? */
> +    core(webView)->mainFrame()->loader()->client()->setUserAgent(String::fromUTF8(userAgent));
> +}

check FrameView::layoutIfNeededRecursively to iterate over the frame tree, it is pretty easy. In the case of Frame's every FrameLoaderClient should be the Gtk+ one. There is SVGImage which is using the EmptyLoaderClient (WebCore/loader/EmptyClients.h) but this is not a frame in the frametree (AFAIK). This also means your current patch does not compile with svg on. :)





> +

I will be kicked if I let this in.

>  static void webkit_web_view_update_settings(WebKitWebView* webView)
>  {
> +    webViewSetUserAgent(webView, userAgent);

the user agent should also be inherited of FrameLoaderClient::createFrame or from within WebKitWebFrame construction. I think the current approach does not work for loading pages with frames. the newly created subframes will have another user agent. If I'm wrong I'm happy to r=+ this patch, but I think it needs another iteration.




>
------- Comment #7 From 2009-01-30 04:51:12 PST -------
(From update of attachment 27004 [details])
I'm don't think that it makes sense to store the user agent on a single FrameLoaderClient instance.  The FIXME's indicate that you were also unsure about this.  Setting the user agent on a WebView should affect all frames in that view, rather than just the main frame.  The way that this is handled on the Mac is that FrameLoaderClient::userAgent asks the WebView for the user agent for a given URL, which it computes if needed or returns the overridden value if the application has set one.  This will ensure that subframes get the correct behaviour.

I'm going to say r- on this basis.


> diff --git a/WebCore/loader/FrameLoaderClient.h b/WebCore/loader/FrameLoaderClient.h
> index b90cecd..f4b9861 100644
> --- a/WebCore/loader/FrameLoaderClient.h
> +++ b/WebCore/loader/FrameLoaderClient.h
> @@ -179,6 +179,9 @@ namespace WebCore {
>          virtual PassRefPtr<DocumentLoader> createDocumentLoader(const ResourceRequest&, const SubstituteData&) = 0;
>          virtual void setTitle(const String& title, const KURL&) = 0;
>  
> +#if PLATFORM(GTK)
> +        virtual void setUserAgent(const String&) = 0;
> +#endif
>          virtual String userAgent(const KURL&) = 0;
>          
>          virtual void savePlatformDataToCachedPage(CachedPage*) = 0;

It doesn't make sense for this to be #if'd in cross-platform code in WebCore.  It would live in FrameLoaderClientGtk.h in WebKit.  There's also no need for it to be declared virtual.
------- Comment #8 From 2009-03-09 10:10:47 PST -------
I did something similar in the bug #21388... Look at the patch there.
------- Comment #9 From 2009-03-09 11:14:35 PST -------
*** Bug 21388 has been marked as a duplicate of this bug. ***
------- Comment #10 From 2009-05-19 06:28:32 PST -------
Created an attachment (id=30468) [details]
Add WebView APIs to manipulate the UA string 

This patch adds WebView APIs to manipulate the UA-String to be sent by WebKit. It allows:

1. Clients to add an 'application name' as part of the default User-Agent string.
2. Set a custom UA to be used by WebKit instead of the default one.
3. Query the "currently used" User-Agent string through a "user-agent" property in the WebView.

I also added a unit test to test the functionality. I also moved the WebKit UA version in configure so we can easily change it without touching the code.

I also changed the UA slightly from (platform, osversion, etc.. are just samples):

Mozilla/5.0 (X11; U; Linux x86_64; en-us) AppleWebKit/530.11+ (KHTML, like Gecko,Safari/530.11+)

to:

Mozilla/5.0 (X11; U; Linux x86_64; en-us) AppleWebKit/530.11+ (KHTML, like Gecko) Safari/530.11+

since I think the latter is a closer interpretation of https://developer.mozilla.org/En/User_Agent_Strings_Reference than the former.

Thoughts?
------- Comment #11 From 2009-05-20 11:35:30 PST -------
(In reply to comment #10)
> Created an attachment (id=30468) [details] [review]
> Add WebView APIs to manipulate the UA string 
> 
> This patch adds WebView APIs to manipulate the UA-String to be sent by WebKit.
> It allows:
> 
> 1. Clients to add an 'application name' as part of the default User-Agent
> string.
> 2. Set a custom UA to be used by WebKit instead of the default one.
> 3. Query the "currently used" User-Agent string through a "user-agent" property
> in the WebView.

Why do you add all this to the WebView instead of WebSettings? I think this is plain wrong.

If you want to allow spoofing based on the address, we should rather have "request-user-agent" or something like that, which would be more useful than trying to modify the string early enough.

> I also added a unit test to test the functionality. I also moved the WebKit UA
> version in configure so we can easily change it without touching the code.

Very nice.

> I also changed the UA slightly from (platform, osversion, etc.. are just
> samples):
> 
> Mozilla/5.0 (X11; U; Linux x86_64; en-us) AppleWebKit/530.11+ (KHTML, like
> Gecko) Safari/530.11+

For what I want, I would prefer a real user agent and limit the spoofing to the hacks done in the Mac port. But I don't expect anyone to agree with me in that respect :)
------- Comment #12 From 2009-05-23 06:27:05 PST -------
Reading the comments, can we start this by moving the user agent default to configure.ac and adding the property and simple getter function? That looks fairly uncontroversial, and then we can move forward on deciding how to do the API to customize it.
------- Comment #13 From 2009-05-23 06:27:45 PST -------
(From update of attachment 30468 [details])
Clearing the review flag, let's split this patch in pieces.
------- Comment #14 From 2009-06-02 16:45:44 PST -------
Created an attachment (id=30885) [details]
WebView API methods to get/set global "user-agent" property using Soup Session

I worked on a solution to this bug, before I became aware of it. I am posting a different approach, this is not as flexible as Alonzo's patch, but works fine in my projects.

This patch adds WebView API methods to set/get the "user-agent" property directly in Soup Session.
------- Comment #15 From 2009-06-02 18:33:10 PST -------
(From update of attachment 30885 [details])
Hey, thanks for the patch. There are a bunch of indentation problems in your patch (tabs vs spaces, most probably), and it is really the wrong solution to the problem. The most proeminent issue being that you are messing with global stuff using WebView API, which is supposed to act on a per-WebView basis, so that's no good. We probably want to do what Xan proposed, to move forward. By the way, you need to mark your patches with '?' in the review combo box, if you want them to catch our attention. You were lucky I'm watching this bug report =)
------- Comment #16 From 2009-06-08 00:54:02 PST -------
Created an attachment (id=31040) [details]
Add webview property to get UA string + refactor UA version string in configure.ac

Same patch with just the property + version macros and without the getter/setter API.
------- Comment #17 From 2009-06-08 06:04:14 PST -------
(In reply to comment #16)
> Created an attachment (id=31040) [details] [review]
> Add webview property to get UA string + refactor UA version string in
> configure.ac
> 
> Same patch with just the property + version macros and without the
> getter/setter API.

I still don't think this makes sense to have in the view. What is your reasoning as opposed to a setting? From my point of view, this means complicating the creation of views and having to manually keep track of the value to keep all views in sync.
------- Comment #18 From 2009-06-08 06:17:57 PST -------
(From update of attachment 31040 [details])
> +     * Since: 1.1.8

This should be 1.1.9, when landing.

> +// Internal use only. See #WebKitWebView 'user-agent' property to get the
> +// User-Agent string used by the WebView

We will probably want to have getter and setter functions, since we have for most things already, so I recommend making this public. For now, I think this suffices, and since we already had a consensus on adding the property, including the getter (which is the API addition), I'll just go ahead and r+.
------- Comment #19 From 2009-06-08 06:30:11 PST -------
(From update of attachment 31040 [details])
ok, I thought we had consensus here, but since we're still considering moving this to settings, let me take back my +
------- Comment #20 From 2009-06-08 06:47:05 PST -------
(In reply to comment #17)
It makes sense to have it in the view, provided it is a per view setting, since a user may want to force different UA depending on the url being visited. 
------- Comment #21 From 2009-06-08 12:50:56 PST -------
(In reply to comment #20)
> (In reply to comment #17)
> It makes sense to have it in the view, provided it is a per view setting, since
> a user may want to force different UA depending on the url being visited. 

To quote myself from this bug report:

If you want to allow spoofing based on the address, we should rather have
"request-user-agent" or something like that, which would be more useful than
trying to modify the string early enough.

Web settings are designed so that you can share them between views. I don't think it makes sense to break the design now. Instead, we should separately consider a different way.
For example, a "request-setting" signal on the web view which would allow you to override settings whenever they are read out. That would also make something like toggling scripts or Netscape plugins easy.
------- Comment #22 From 2009-06-08 13:57:14 PST -------
(In reply to comment #17)
> (In reply to comment #16)
> > Created an attachment (id=31040) [details] [review] [review]
> > Add webview property to get UA string + refactor UA version string in
> > configure.ac
> > 
> > Same patch with just the property + version macros and without the
> > getter/setter API.
> 
> I still don't think this makes sense to have in the view. What is your
> reasoning as opposed to a setting? From my point of view, this means

1. Having it in the view (properties, getters and setters) makes it more flexible for applications to control what's in their UA string.

2. Having it in the view allows for per site and per view spoofing.

3. I tend to associate UA strings to a WebView, not toa Setting.

> complicating the creation of views and having to manually keep track of the
> value to keep all views in sync.

What made you say it complicates things? Have you looked at the patch?

In comment #21:

"For example, a "request-setting" signal on the web view which would allow you
to override settings whenever they are read out. That would also make something
like toggling scripts or Netscape plugins easy."

Why would you want to send a signal for possibly every request WebKitGtk makes? UA strings should be set before a request is made. I'm not sure why we would want to ask for a UA everytime we need one.



> 
------- Comment #23 From 2009-06-08 14:10:00 PST -------
(In reply to comment #22)
> (In reply to comment #17)
> > (In reply to comment #16)
> > > Created an attachment (id=31040) [details] [review] [review] [review]
> > > Add webview property to get UA string + refactor UA version string in
> > > configure.ac
> > > 
> > > Same patch with just the property + version macros and without the
> > > getter/setter API.
> > 
> > I still don't think this makes sense to have in the view. What is your
> > reasoning as opposed to a setting? From my point of view, this means
> 
> 1. Having it in the view (properties, getters and setters) makes it more
> flexible for applications to control what's in their UA string.
> 
> 2. Having it in the view allows for per site and per view spoofing.
> 
> 3. I tend to associate UA strings to a WebView, not toa Setting.
> 
> > complicating the creation of views and having to manually keep track of the
> > value to keep all views in sync.
> 
> What made you say it complicates things? Have you looked at the patch?

It complicates because I have to keep track of the string instead of setting it once like any other setting.

> "For example, a "request-setting" signal on the web view which would allow you
> to override settings whenever they are read out. That would also make something
> like toggling scripts or Netscape plugins easy."
> 
> Why would you want to send a signal for possibly every request WebKitGtk makes?
> UA strings should be set before a request is made. I'm not sure why we would
> want to ask for a UA everytime we need one.

Because you want to spoof your identitiy to work around bugs in websites. Safari and Opera do it according to a blacklist. My primary interest is a consistent state across all tabs, however, so I can only guess the use case.

Essentially I'm still interested in knowing how you are using the API. Do you think this is different from enabling scripts or images based on the location?
------- Comment #24 From 2009-06-20 00:38:43 PST -------
(In reply to comment #23)
> > 
> > What made you say it complicates things? Have you looked at the patch?
> 
> It complicates because I have to keep track of the string instead of setting it
> once like any other setting.

Ah, ok. I think I get where you're coming from. So a WebSetting property will do what you want right? I'll revise the patch with this in mind.

> Do you think this is different from enabling scripts or images based on the location?

I don't think this use case is something I've thought about or even in the scope of my proposed API. But I'll think it in mind once I get to the API bits. Thanks for bringing it up.
------- Comment #25 From 2009-06-20 02:09:22 PST -------
Created an attachment (id=31591) [details]
move the user-agent property from the view to the settings
------- Comment #26 From 2009-06-20 05:27:47 PST -------
(From update of attachment 31591 [details])
> +# Sourced from WebCore/Configurations/Version.xcconfig
> +m4_define([webkit_user_agent_major_version], [531])
> +m4_define([webkit_user_agent_minor_version], [0])

I believe this needs to be raised already? The patch looks fine to me, and it's good that we have a test =). r=me if Xan and Christian are OK with the proposed API.

> +
>  AC_INIT([WebKit],[webkit_major_version.webkit_minor_version.webkit_micro_version],[http://bugs.webkit.org/])
>  
>  AC_CONFIG_MACRO_DIR([autotools])
> @@ -16,9 +23,13 @@ AC_CANONICAL_HOST
>  WEBKIT_MAJOR_VERSION=webkit_major_version
>  WEBKIT_MINOR_VERSION=webkit_minor_version
>  WEBKIT_MICRO_VERSION=webkit_micro_version
> +WEBKIT_USER_AGENT_MAJOR_VERSION=webkit_user_agent_major_version
> +WEBKIT_USER_AGENT_MINOR_VERSION=webkit_user_agent_minor_version
>  AC_SUBST(WEBKIT_MAJOR_VERSION)
>  AC_SUBST(WEBKIT_MINOR_VERSION)
>  AC_SUBST(WEBKIT_MICRO_VERSION)
> +AC_SUBST(WEBKIT_USER_AGENT_MAJOR_VERSION)
> +AC_SUBST(WEBKIT_USER_AGENT_MINOR_VERSION)
>  
>  AC_CONFIG_SRCDIR([WebCore/config.h])
>  
------- Comment #27 From 2009-06-20 13:13:33 PST -------
(In reply to comment #25)
> Created an attachment (id=31591) [details] [review]
> move the user-agent property from the view to the settings

> Ah, ok. I think I get where you're coming from. So a WebSetting property will
> do what you want right? I'll revise the patch with this in mind.

Yes. Thanks a lot for the new patch.

+#if GLIB_CHECK_VERSION(2, 16, 0) && GTK_CHECK_VERSION(2, 14, 0)

You can omit the Glib check in new tests, we require Glib 2.16 these days.

+    g_assert_cmpstr(userAgent, ==, "Mozilla/5.0 (X11; U; Linux x86_64; c) AppleWebKit/531.0+ (KHTML, like Gecko) Safari/531.0+");

I have the feeling this test fails on non-linux and non-AMD64 machines. I think you can compare to webkit_get_user_agent() instead?

I wonder what DEFINE_STATIC_LOCAL is and why you rewrote the user agent functions. Looks fine, though, so it doesn't really matter to me.

@@ -617,6 +640,8 @@ static void webkit_web_settings_set_property(GObject* object, guint prop_id, con
         g_slist_free(priv->spell_checking_languages_list);
         priv->spell_checking_languages_list = spellLanguages;
         break;
+    case PROP_USER_AGENT:
+        break; // do nothing for now

I think you forgot to add a line to update the string? :)

> +# Sourced from WebCore/Configurations/Version.xcconfig
> +m4_define([webkit_user_agent_major_version], [531])
> +m4_define([webkit_user_agent_minor_version], [0])

I suggest in a different patch we simply start reading Version.xcconfig or alternatively propose a port-agnostic place to define this version.
------- Comment #28 From 2009-06-20 16:19:31 PST -------
(In reply to comment #27)
> +#if GLIB_CHECK_VERSION(2, 16, 0) && GTK_CHECK_VERSION(2, 14, 0)
> 
> You can omit the Glib check in new tests, we require Glib 2.16 these days.

That will be in a separate bug/patch since we need to update all our unit tests.

> 
> +    g_assert_cmpstr(userAgent, ==, "Mozilla/5.0 (X11; U; Linux x86_64; c)
> AppleWebKit/531.0+ (KHTML, like Gecko) Safari/531.0+");
> 
> I have the feeling this test fails on non-linux and non-AMD64 machines. I think
> you can compare to webkit_get_user_agent() instead?

Yeah. I'll update. Thanks.

> +    case PROP_USER_AGENT:
> +        break; // do nothing for now
> 
> I think you forgot to add a line to update the string? :)

I'm meant to add those in a separate patch. But since the design changed I might as well include it in the next patch.

> 
> > +# Sourced from WebCore/Configurations/Version.xcconfig
> > +m4_define([webkit_user_agent_major_version], [531])
> > +m4_define([webkit_user_agent_minor_version], [0])
> 
> I suggest in a different patch we simply start reading Version.xcconfig or
> alternatively propose a port-agnostic place to define this version.

Yup.

I'll update the patch. Thanks for the feedback.

> 
------- Comment #29 From 2009-06-22 03:35:11 PST -------
Created an attachment (id=31641) [details]
updated per Christian's feedback

I've updated the patch based on Christian's feedback. Also with this patch you can set a custom user-agent string via the user-agent property. If a custom UA string has been set. get_user_agent will return the custom UA string. If there's no custom UA string, it will return the default UA string.
------- Comment #30 From 2009-06-22 10:44:39 PST -------
(From update of attachment 31641 [details])
> +     * WebKitWebSettings:user-agent:
> +     *
> +     * The User-Agent string used by WebKitGtk.
> +     *
> +     * This will return a default User-Agent string if a custom string wasn't
> +     * provided by the application.

Only a nitpick: I wouldn't say 'return' here, since this is a property, not a function. How about 'This holds a default User-Agent string, if a custom string...'? I know we have other properties with similarly incorrect docs, but I'd rather we fix them, than adding more =D. Looks good to me otherwise. Christian, Xan?
------- Comment #31 From 2009-06-23 06:33:14 PST -------
(From update of attachment 31641 [details])
> +    /**
> +     * WebKitWebSettings:user-agent:
> +     *
> +     * The User-Agent string used by WebKitGtk.
> +     *
> +     * This will return a default User-Agent string if a custom string wasn't
> +     * provided by the application.
> +     *
> +     * Since: 1.1.11
> +     */
> +    g_object_class_install_property(gobject_class, PROP_USER_AGENT,
> +                                    g_param_spec_string("user-agent",
> +                                                        _("User Agent"),
> +                                                        _("The User-Agent string used by WebKitGtk"),
> +                                                        NULL,

Shouldn't you specify the actual default value instead of NULL?

>  
> +    priv->custom_user_agent = CString();
> +    priv->user_agent = CString();

Shouldn't you use 'delete' here?

> +    case PROP_USER_AGENT:
> +        // Set a custom User-Agent string

I think this comment is a bit superfluous.

> +/**
> + * webkit_web_settings_get_user_agent:
> + * @web_view: a #WebKitWebSettings

@web_settings, not @web_view

> + *
> + * Returns the User-Agent string currently used by the WebView.

By the web view(s) associated with the @web_settings.
------- Comment #32 From 2009-06-24 11:50:46 PST -------
(In reply to comment #31)
> Shouldn't you specify the actual default value instead of NULL?

hrm, I guess that makes sense.

> > +    priv->custom_user_agent = CString();
> > +    priv->user_agent = CString();
> 
> Shouldn't you use 'delete' here?

No, because there's no 'new'.
------- Comment #33 From 2009-06-24 11:58:42 PST -------
> > > +    priv->custom_user_agent = CString();
> > > +    priv->user_agent = CString();
> > 
> > Shouldn't you use 'delete' here?
> 
> No, because there's no 'new'.
> 

Ah, right. Since CString seems to be refcounted then release the reference I guess. My point is that assigning a new empty CString, while possibly has the side-effect of freeing the old one, does not seem to be a very clean way of freeing memory?
------- Comment #34 From 2009-06-26 00:56:11 PST -------
Created an attachment (id=31914) [details]
updated per xan's feedback, converted CString to gchar
------- Comment #35 From 2009-06-29 14:52:35 PST -------
(From update of attachment 31914 [details])
@@ WebKitWebSettings* webkit_web_settings_copy(WebKitWebSettings* web_settings)
794893                  "enable-html5-database", priv->enable_html5_database,
795894                  "enable-html5-local-storage", priv->enable_html5_local_storage,
796895                  "enable-xss-auditor", priv->enable_xss_auditor,
 896                  "user-agent", webkit_web_settings_get_user_agent(web_settings),
797897                  NULL));
798898 
799899     return copy;

Here, maybe it makes sense to check if the user agent has been customized, and only set user-agent if so? The rest keeps looking good to me. Ah, you seem to still be including CString.h, I think you don't need it anymore?
------- Comment #36 From 2009-06-30 05:42:32 PST -------
Created an attachment (id=32055) [details]
updated patch
------- Comment #37 From 2009-06-30 05:45:52 PST -------
Created an attachment (id=32056) [details]
updated patch with empty string test
------- Comment #38 From 2009-06-30 05:48:55 PST -------
(From update of attachment 32055 [details])
> +    // test the default UA string
> +    g_object_get(G_OBJECT(settings),"user-agent", &userAgent, NULL);
> +    g_assert_cmpstr(userAgent, ==, webkit_web_settings_get_user_agent(settings));
> +    g_free(userAgent);
> +

This is not really checking anything, right? Just that you get the same through the property and the direct API. You should cache the value, though...

> +    // test a custom UA string
> +    userAgent = NULL;
> +    g_object_set(G_OBJECT(settings), "user-agent", "testwebsettings/0.1", NULL);
> +    g_object_get(G_OBJECT(settings),"user-agent", &userAgent, NULL);
> +    g_assert_cmpstr(userAgent, ==, "testwebsettings/0.1");
> +    g_free(userAgent);
> +
> +    // setting it to NULL or an empty value should give us the default UA string
> +    userAgent = NULL;
> +    g_object_set(G_OBJECT(settings), "user-agent", NULL, NULL);
> +    g_object_get(G_OBJECT(settings),"user-agent", &userAgent, NULL);
> +    g_assert_cmpstr(userAgent, ==, webkit_web_settings_get_user_agent(settings));

... so you can compare it here. Otherwise you are not really checking anything, right?



> +    /**
> +     * WebKitWebSettings:user-agent:
> +     *
> +     * The User-Agent string used by WebKitGtk.
> +     *
> +     * This will return a default User-Agent string if a custom string wasn't
> +     * provided by the application.
> +     *
> +     * Since: 1.1.11

The doc sould mention the bit about NULL being an effective reset.

With those two things it looks good to me, so r=me from my side.
------- Comment #39 From 2009-06-30 06:03:46 PST -------
Created an attachment (id=32059) [details]
updated patch with some test and doc fixes
------- Comment #40 From 2009-06-30 06:36:08 PST -------
Created an attachment (id=32063) [details]
proper fix to the test + version bump to 531.2
------- Comment #41 From 2009-06-30 22:52:45 PST -------
(From update of attachment 32063 [details])
Looks good to me. Since Gustavo already said he was OK with it I'll r+.
------- Comment #42 From 2009-07-03 02:58:28 PST -------
(In reply to comment #41)
> (From update of attachment 32063 [details] [details])
> Looks good to me. Since Gustavo already said he was OK with it I'll r+.

Landed in r45531 - http://trac.webkit.org/changeset/45531