Bug 48636 – [Qt] Mobile Devices should include Model and Firmware Version in Webkit Generated User Agent String

• Component: WebKit Qt
• Status: RESOLVED FIXED

[Stanislav Paltis]

With QtMobility SystemInfo module present on the mobile device, proposal is to include Model and Firmware Version information in Webkit Generated USer agent String.

[Alexey Proskuryakov]

Note that this would be detrimental to privacy, see e.g. <http://panopticlick.eff.org/browser-uniqueness.pdf>.

[Stanislav Paltis]

Added [Qt] in the title, as it is related to Qt implementation.

[Stanislav Paltis]

Created attachment 72535 [details]
Implementation proposal v1, wanted to hear comments, if you believe tests are needed, please comment appropriately.

[Laszlo Gombos]

Comment on attachment 72535 [details]
Implementation proposal v1, wanted to hear comments, if you believe tests are needed, please comment appropriately.

View in context: https://bugs.webkit.org/attachment.cgi?id=72535&action=review

If this is up for review, please set the r? flag.

> WebKit/qt/ChangeLog:11
> +        Fixed addition of S60 Version in User Agent, where extra space woudl show up, if S60

typo - woudl. Please fix.

> WebKit/qt/ChangeLog:16
> +        (QWebPagePrivate::_q_onLoadProgressChanged):

Changes in the body of this function is not related, please remove them.

> WebKit/qt/Api/qwebpage.cpp:3601
> +            firstPartTemp += QLatin1Char(' ');

Shouldn't the order of the concatenation changed here ? 
Also perhaps we can combine the two string operations into one " Series60/3.1"

> WebKit/qt/Api/qwebpage.cpp:3605
> +            firstPartTemp += QLatin1Char(' ');

Ditto.

> WebKit/qt/Api/qwebpage.cpp:3609
> +            firstPartTemp += QLatin1Char(' ');

Ditto.

> WebKit/qt/Api/qwebpage.cpp:3622
> +#if ENABLE(QT_SYSTEMINFO)

Perhaps we should check if the model or the Firmware version is empty even if ENABLE_QT_SYSTEMINFO is defined. I do not think we should add "/" if the Firmware version is empty.
Similarly I do not think we should add the firmware version if the model information is empty.

> WebKit/qt/Api/qwebpage.cpp:-3673
> -

Unrelated change, please remove.

> WebKit/qt/Api/qwebpage.cpp:-3680
> -

Ditto.

[Stanislav Paltis]

Created attachment 72681 [details]
Patch

[Kenneth Rohde Christiansen]

(In reply to comment #5)
> Created an attachment (id=72681) [details]
> Patch

Will this work for all upcoming MeeGo powered devices as well? Has it been tested?

[Kenneth Rohde Christiansen]

Comment on attachment 72681 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=72681&action=review

> WebKit/qt/ChangeLog:12
> +        Model is not empty, but Firmware is empty string, than "/" won't be 

Why are some words as Firmware and Model written with capital? It seems strange

> WebKit/qt/ChangeLog:15
> +        Fixed addition of S60 Versionin User Agent, where extra space would show up, 

Versionin ? Needs a space I guess

> WebKit/qt/Api/qwebpage.cpp:3616
>          firstPartTemp += QString::fromLatin1("Unknown");

Why no added space here then. Might we get Unknown8u923u2 in a UA?

> WebKit/qt/Api/qwebpage.cpp:3627
> +        
> +        QString model = systemDeviceInfo.model();
> +        if (!model.isEmpty()) {
> +            firstPartTemp += systemDeviceInfo.model();
> +            QString firmware = systemInfo.version(QtMobility::QSystemInfo::Firmware);

Maybe we should prepend an empty space here instead? ofcourse we need to consider performance.

[Stanislav Paltis]

(In reply to comment #6)
> (In reply to comment #5)
> > Created an attachment (id=72681) [details] [details]
> > Patch
> 
> Will this work for all upcoming MeeGo powered devices as well? Has it been tested?

No, it was not tested on Meego yet, only on Symbian device.

[Stanislav Paltis]

(In reply to comment #4)
> (From update of attachment 72535 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=72535&action=review
> 
> If this is up for review, please set the r? flag.
> 
> > WebKit/qt/ChangeLog:11
> > +        Fixed addition of S60 Version in User Agent, where extra space woudl show up, if S60
> 
> typo - woudl. Please fix.
> 
> > WebKit/qt/ChangeLog:16
> > +        (QWebPagePrivate::_q_onLoadProgressChanged):
> 
> Changes in the body of this function is not related, please remove them.
> 
> > WebKit/qt/Api/qwebpage.cpp:3601
> > +            firstPartTemp += QLatin1Char(' ');
> 
> Shouldn't the order of the concatenation changed here ? 
> Also perhaps we can combine the two string operations into one " Series60/3.1"
> 
> > WebKit/qt/Api/qwebpage.cpp:3605
> > +            firstPartTemp += QLatin1Char(' ');
> 
> Ditto.
> 
> > WebKit/qt/Api/qwebpage.cpp:3609
> > +            firstPartTemp += QLatin1Char(' ');
> 
> Ditto.
> 
> > WebKit/qt/Api/qwebpage.cpp:3622
> > +#if ENABLE(QT_SYSTEMINFO)
> 
> Perhaps we should check if the model or the Firmware version is empty even if ENABLE_QT_SYSTEMINFO is defined. I do not think we should add "/" if the Firmware version is empty.
> Similarly I do not think we should add the firmware version if the model information is empty.
> 
> > WebKit/qt/Api/qwebpage.cpp:-3673
> > -
> 
> Unrelated change, please remove.
> 
> > WebKit/qt/Api/qwebpage.cpp:-3680
> > -
> 
> Ditto.

Fixed the patch according to comments.

[Stanislav Paltis]

(In reply to comment #7)
> (From update of attachment 72681 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=72681&action=review
> 
> > WebKit/qt/ChangeLog:12
> > +        Model is not empty, but Firmware is empty string, than "/" won't be 
> 
> Why are some words as Firmware and Model written with capital? It seems strange

I will fix that. Sorry about that.
> 
> > WebKit/qt/ChangeLog:15
> > +        Fixed addition of S60 Versionin User Agent, where extra space would show up, 
> 
> Versionin ? Needs a space I guess

you are right, my mistake.
> 
> > WebKit/qt/Api/qwebpage.cpp:3616
> >          firstPartTemp += QString::fromLatin1("Unknown");
> 
> Why no added space here then. Might we get Unknown8u923u2 in a UA?

In what case?
> 
> > WebKit/qt/Api/qwebpage.cpp:3627
> > +        
> > +        QString model = systemDeviceInfo.model();
> > +        if (!model.isEmpty()) {
> > +            firstPartTemp += systemDeviceInfo.model();
> > +            QString firmware = systemInfo.version(QtMobility::QSystemInfo::Firmware);
> 
> Maybe we should prepend an empty space here instead? of course we need to consider performance.

space is prepended either after Symbian version, if on Symbian, and/or after adding SSL Support.

[Kenneth Rohde Christiansen]

(In reply to comment #8)
> (In reply to comment #6)
> > (In reply to comment #5)
> > > Created an attachment (id=72681) [details] [details] [details]
> > > Patch
> > 
> > Will this work for all upcoming MeeGo powered devices as well? Has it been tested?
> 
> No, it was not tested on Meego yet, only on Symbian device.

I would like it tested to make sure it doesn't cause any regression. If so we would need to make this change only apply to Symbian devices at least until it can be made to work for MeeGo.

[Kenneth Rohde Christiansen]

(In reply to comment #1)
> Note that this would be detrimental to privacy, see e.g. <http://panopticlick.eff.org/browser-uniqueness.pdf>.

Christian, did you see this? ^

[Stanislav Paltis]

Created attachment 72691 [details]
Patch

[Stanislav Paltis]

(In reply to comment #11)
> (In reply to comment #8)
> > (In reply to comment #6)
> > > (In reply to comment #5)
> > > > Created an attachment (id=72681) [details] [details] [details] [details]
> > > > Patch
> > > 
> > > Will this work for all upcoming MeeGo powered devices as well? Has it been tested?
> > 
> > No, it was not tested on Meego yet, only on Symbian device.
> 
> I would like it tested to make sure it doesn't cause any regression. If so we would need to make this change only apply to Symbian devices at least until it can be made to work for MeeGo.

At this point, I will isolate it only for Symbian, unless there are objections from others..

[Early Warning System Bot]

Attachment 72691 [details] did not build on qt:
Build output: http://queues.webkit.org/results/4989016

[Simon Hausmann]

Comment on attachment 72691 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=72691&action=review

One thing that is also missing from this patch is an update of the documentation, which explains the contents of the string. The documentation should be extended to explain how the user agent looks like with the device information included IMHO.

I think the patch is good otherwise, and I don't think we should limit it to Symbian.

> WebKit/qt/Api/qwebpage.cpp:3622
> +        QtMobility::QSystemDeviceInfo systemDeviceInfo = QtMobility::QSystemDeviceInfo(d->q);
> +        QtMobility::QSystemInfo systemInfo = QtMobility::QSystemInfo(d->q);

I think this should be written simpler:

QtMobility::QSystemDeviceInfo systemDeviceInfo;
QtMobility::QSystemInfo systemInfo;

The objects are on the stack, no need to provide parents.

[zalan]

what is the browser use case to include firmware version? Do we know how big that string is going to be? -just wondering about payload of transferring all this information on each and every request. 
Isn't there any other way for backends to query this information?

[Stanislav Paltis]

Created attachment 72824 [details]
Patch

[Stanislav Paltis]

(In reply to comment #16)
> (From update of attachment 72691 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=72691&action=review
> 
> One thing that is also missing from this patch is an update of the documentation, which explains the contents of the string. The documentation should be extended to explain how the user agent looks like with the device information included IMHO.
> 
> I think the patch is good otherwise, and I don't think we should limit it to Symbian.
> 
> > WebKit/qt/Api/qwebpage.cpp:3622
> > +        QtMobility::QSystemDeviceInfo systemDeviceInfo = QtMobility::QSystemDeviceInfo(d->q);
> > +        QtMobility::QSystemInfo systemInfo = QtMobility::QSystemInfo(d->q);
> 
> I think this should be written simpler:
> 
> QtMobility::QSystemDeviceInfo systemDeviceInfo;
> QtMobility::QSystemInfo systemInfo;
> 
> The objects are on the stack, no need to provide parents.

Done. patch submitted.

[Stanislav Paltis]

Comment on attachment 72824 [details]
Patch

setting for commit-queue- until decisions are clarified and explained in the bug report.

[Laszlo Gombos]

Comment on attachment 72824 [details]
Patch

This patch has been only tested on Symbian and it seems that this really only works well on Symbian. We should either guard the code with SYMBIAN (which I do not recommend) or consider other platforms as well. 

On Linux desktop (QtMobility installed) the patch results in the following User-Agent string: "Mozilla/5.0 (X11; N; Linux i686i686 Other/2.6.32-24-generic; en-US) ...". 
Notice that the CPU architecture (i686) is reported twice and a space seems to be missing.

In general for most OSs I would suggest to test if QT_SYSTEMINFO is available and if it is, than we should report the values returned by QtMobility (runtime). As a fall-back we can have compile time flags in QtWebKit (as we have now) when Qt_MOBILITY is not available. 

In general we should keep the User-Agent small and simple and not report information just because it is available. 

I'm not sure if we really want to report QtMobility::QSystemInfo::Firmware, however I think reporting the model or parts of the model make sense.

r- as non-Symbian platforms needs to be addressed as well.

[qi]

Created attachment 82840 [details]
patch

1. only keep model in user agent string, no hardware version.
2. remove duplicates ";" 
3. for linux:
   with qtmobility: "Mozilla/5.0 (X11; U; Linux; i686 Tower; en-US) AppleWebKit/534.17 (KHTML, like Gecko) QtTestBrowser/0.1 Safari/534.17"
   without qtmobility: "Mozilla/5.0 (X11; U; Linux i686; en-US) AppleWebKit/534.17 (KHTML, like Gecko) QtTestBrowser/0.1 Safari/534.17"
4. for symbian:
   with qtmobility: "Mozilla/5.0 (Symbian/3; U; N8-00; en-US) AppleWebKit/534.17 (KHTML, like Gecko) QtTestBrowser/0.1 Safari/534.3"
   without qtmobility: "Mozilla/5.0 (Symbian/3; U; en-US) AppleWebKit/534.17 (KHTML, like Gecko) QtTestBrowser/0.1 Safari/534.3"

[Alexis Menard (darktears)]

I remember the WebKit team having this discussion with a visitor from Boston and we said it was a bad idea. Instead can the website detects what the browser support?

Interesting link btw : http://diveintohtml5.org/detect.html

[Eric Seidel (no email)]

There is an ongoing effort to simplify user agent strings.

[Peter Kasting]

Please don't add to the UA string if at all possible.  There is a cross-browser effort to simplify UA strings.

See http://hacks.mozilla.org/2010/09/final-user-agent-string-for-firefox-4/ for some details, including notes about how Firefox for Android and Maemo constructs its UA string.  Any string we send ought to be in sync with what they're doing.

I have scanned all the comments on this bug and there's been no justification of this change, or links to outside discussions, despite multiple people's questions or reservations.

[Laszlo Gombos]

Comment on attachment 82840 [details]
patch

Cross-posting from the Gecko discussion (https://bugzilla.mozilla.org/show_bug.cgi?id=595669)

1./ From the Android 2.3 Compatibility Definition it is clear that Android
mandates reporting the DeviceModel -
(http://source.android.com/compatibility/android-2.3-cdd.pdf):

The user agent string reported by the WebView MUST be in this format:

Mozilla/5.0 (Linux; U; Android $(VERSION); $(LOCALE); $(MODEL) Build/$(BUILD))
AppleWebKit/533.1 (KHTML, like
Gecko) Version/4.0 Mobile Safari/533.1

2./ IE for Windows Phone mandates DeviceModel section - 
http://blogs.msdn.com/b/iemobile/archive/2010/03/25/ladies-and-gentlemen-please-welcome-the-ie-mobile-user-agent-string.aspx.

3./ Device manufactures (e.g. Nokia) typically includes DeviceModel.

I think there is a real need for an HTTP request to identify the device, and the DeviceModel in the user agent string seems to be the most commonly used approach today.  

The DeviceModel could be considered the %Subplatform% for some (all?) mobile platforms, which would fit nicely into our WebKit UA template.

When it comes to the patch attached, I do agree that on desktop (non-mobile) platforms QtWebKit should not report the systemDeviceInfo.model just because QtWebKit was built with QtMobility libraries available. I will mark the patch r- for this specific reason.

I think we should only report the model info for specific mobile platforms (e.g. Symbian; MeeGo). For these mobile platforms the "%Platform%" part of the UA string should be specific to the mobile platform as well (e.g report "MeeGo" as the platform, instead of Linux or X11 - as MeeGo implies Linux already, just like Android would).

[Peter Kasting]

(In reply to comment #26)
> (From update of attachment 82840 [details])
> 1./ From the Android 2.3 Compatibility Definition it is clear that Android
> mandates reporting the DeviceModel -
> (http://source.android.com/compatibility/android-2.3-cdd.pdf):
> 
> The user agent string reported by the WebView MUST be in this format:
> 
> Mozilla/5.0 (Linux; U; Android $(VERSION); $(LOCALE); $(MODEL) Build/$(BUILD))
> AppleWebKit/533.1 (KHTML, like
> Gecko) Version/4.0 Mobile Safari/533.1

Interesting... that's probably going to have to change as we're scrapping U; and $(LOCALE); and I doubt Android browsers are going to want to differ.

> I think we should only report the model info for specific mobile platforms (e.g. Symbian; MeeGo). For these mobile platforms the "%Platform%" part of the UA string should be specific to the mobile platform as well (e.g report "MeeGo" as the platform, instead of Linux or X11 - as MeeGo implies Linux already, just like Android would).

Yes, please do this, it matches what Firefox 4 is doing (see http://hacks.mozilla.org/2010/09/final-user-agent-string-for-firefox-4/ point 4).

[Ademar Reis]

Please remember that tomorrow is the deadline to have this fix included in the weekly tag of Qtwebkit-2.1, so if you have a patch, you should submit it ASAP.

[Ademar Reis]

(In reply to comment #28)
> Please remember that tomorrow is the deadline to have this fix included in the weekly tag of Qtwebkit-2.1, so if you have a patch, you should submit it ASAP.

By tomorrow I mean tomorrow by noon.

[qi]

Created attachment 84733 [details]
patch2

Update the patch based on Laszlo's latest comments.

[Laszlo Gombos]

Comment on attachment 84733 [details]
patch2

LGTM. r+.

[Laszlo Gombos]

Comment on attachment 84733 [details]
patch2

+    !CONFIG(QTDIR_build):!contains(DEFINES, ENABLE_QT_USERAGENT_DEVICEMODEL=.) {
+        contains(MOBILITY_CONFIG, systeminfo) {
+            DEFINES += ENABLE_QT_SYSTEMINFO=1
+        }

I missed that ENABLE_QT_SYSTEMINFO needs to be renamed to ENABLE_QT_USERAGENT_DEVICEMODEL r- to take it out from the queue.

[Kenneth Rohde Christiansen]

Comment on attachment 84733 [details]
patch2

Weren't we trying to simplify the UA?

[qi]

Created attachment 84770 [details]
patch3

Update patch.

[Ademar Reis]

Created attachment 84782 [details]
patch for qtwebkit-2.1, sent by Laszlo / Qi

Laszlo sent me this backport rubber-stamped. It's being added to 2.1 right now.

[Kenneth Rohde Christiansen]

Comment on attachment 84782 [details]
patch for qtwebkit-2.1, sent by Laszlo / Qi

View in context: https://bugs.webkit.org/attachment.cgi?id=84782&action=review

> WebCore/WebCore.pri:139
> +# systeminfo support if QtMobility systeminfo module exists, but only for symbian, maemo and meego
> +symbian|maemo5|maemo6 {
> +    !CONFIG(QTDIR_build):!contains(DEFINES, ENABLE_QT_USERAGENT_DEVICEMODEL=.) {

Does the people who decided adding this device info know there is an ongoing effort to remove these things from the UA's? This seems like a step in the wrong direction.

> WebKit/qt/Api/qwebpage.cpp:1997
> -    Its argument, either true or false, indicates whether or not the load
> +    Its argument, either true or false, indicates whether or not the load 

Why adding a trailing space here?

> WebKit/qt/Api/qwebpage.cpp:3873
> +        // adding Model Number

We normally use proper sentences.

[Ademar Reis]

Well... this bug discussion clearly shows why the policy is to only add patches to a branch after they're included in trunk and not before, but given the circunstances (symbian need/pressure + extended holiday), I had no choice but to add it to the branch. Hopefully it does end in trunk AS IS.

Please have in mind that I'm now on vacation and we have an extended holiday ahead of us here in Brazil. If some problem happens in the next days, someone with commit privileges on the branches please go ahead and revert it.

Patch added to 2.1 as 01f01edc8dde851f60a1067190e9a819c9bb024a, merged into 2.1.x as well.

[Laszlo Gombos]

Comment on attachment 84770 [details]
patch3

> Does the people who decided adding this device info know there is an ongoing effort to remove these things from the UA's? This seems like a step in the wrong direction.

Well, I can only speak for myself, but I'm aware of bug 54556 (hint: check the reporter).

> Weren't we trying to simplify the UA?

The simplified UA model is "Mozilla/5.0 (%Platform%; %Subplatform%)...."; this patch fits into that model and makes both the %Platform% and %Subplatform% more specific for that narrow use-case where this information is used.

Note that in general neither %Platform% nor %Subplatform% should not be used for making a decision on the server what sort of web content to offer for the browser.

This patch allows to turn this reporting of by setting ENABLE_QT_USERAGENT_DEVICEMODEL=0 at build time.

I believe most serious concerns were addressed. r=me.

[WebKit Commit Bot]

Comment on attachment 84770 [details]
patch3

Clearing flags on attachment: 84770

Committed r80424: <http://trac.webkit.org/changeset/80424>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.