|
Description
Sven Herzberg
2008-02-15 08:00:43 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. Created attachment 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.
Created attachment 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.
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. (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 on attachment 27004 [details] Implement "user-agent" web settings property > 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 on attachment 27004 [details] Implement "user-agent" web settings property 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. I did something similar in the bug #21388... Look at the patch there. *** Bug 21388 has been marked as a duplicate of this bug. *** Created attachment 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? (In reply to comment #10) > Created an attachment (id=30468) [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 :) 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 on attachment 30468 [details]
Add WebView APIs to manipulate the UA string
Clearing the review flag, let's split this patch in pieces.
Created attachment 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 on attachment 30885 [details]
WebView API methods to get/set global "user-agent" property using Soup Session
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 =)
Created attachment 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.
(In reply to comment #16) > Created an attachment (id=31040) [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 on attachment 31040 [details] Add webview property to get UA string + refactor UA version string in configure.ac > + * 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 on attachment 31040 [details]
Add webview property to get UA string + refactor UA version string in configure.ac
ok, I thought we had consensus here, but since we're still considering moving this to settings, let me take back my +
(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. (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. (In reply to comment #17) > (In reply to comment #16) > > Created an attachment (id=31040) [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. > (In reply to comment #22) > (In reply to comment #17) > > (In reply to comment #16) > > > Created an attachment (id=31040) [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? (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. Created attachment 31591 [details]
move the user-agent property from the view to the settings
Comment on attachment 31591 [details] move the user-agent property from the view to the settings > +# 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]) > (In reply to comment #25) > Created an attachment (id=31591) [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. (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. > Created attachment 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 on attachment 31641 [details] updated per Christian's feedback > + * 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 on attachment 31641 [details] updated per Christian's feedback > + /** > + * 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. (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'. > > > + 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?
Created attachment 31914 [details]
updated per xan's feedback, converted CString to gchar
Comment on attachment 31914 [details]
updated per xan's feedback, converted CString to gchar
@@ 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?
Created attachment 32055 [details]
updated patch
Created attachment 32056 [details]
updated patch with empty string test
Comment on attachment 32055 [details] updated patch > + // 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. Created attachment 32059 [details]
updated patch with some test and doc fixes
Created attachment 32063 [details]
proper fix to the test + version bump to 531.2
Comment on attachment 32063 [details]
proper fix to the test + version bump to 531.2
Looks good to me. Since Gustavo already said he was OK with it I'll r+.
(In reply to comment #41) > (From update of attachment 32063 [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 |