Bug 48636 - [Qt] Mobile Devices should include Model and Firmware Version in Webkit Generated User Agent String
Summary: [Qt] Mobile Devices should include Model and Firmware Version in Webkit Gener...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit Qt (show other bugs)
Version: 528+ (Nightly build)
Hardware: S60 Hardware S60 3rd edition
: P2 Normal
Assignee: qi
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2010-10-29 07:31 PDT by Stanislav Paltis
Modified: 2011-04-11 11:44 PDT (History)
14 users (show)

See Also:


Attachments
Implementation proposal v1, wanted to hear comments, if you believe tests are needed, please comment appropriately. (5.43 KB, patch)
2010-11-01 11:50 PDT, Stanislav Paltis
no flags Details | Formatted Diff | Diff
Patch (5.64 KB, patch)
2010-11-02 09:40 PDT, Stanislav Paltis
no flags Details | Formatted Diff | Diff
Patch (5.65 KB, patch)
2010-11-02 10:01 PDT, Stanislav Paltis
no flags Details | Formatted Diff | Diff
Patch (3.14 KB, patch)
2010-11-03 08:15 PDT, Stanislav Paltis
laszlo.gombos: review-
Details | Formatted Diff | Diff
patch (4.61 KB, patch)
2011-02-17 12:03 PST, qi
laszlo.gombos: review-
Details | Formatted Diff | Diff
patch2 (4.91 KB, patch)
2011-03-04 05:59 PST, qi
laszlo.gombos: review-
laszlo.gombos: commit-queue-
Details | Formatted Diff | Diff
patch3 (4.91 KB, patch)
2011-03-04 10:06 PST, qi
no flags Details | Formatted Diff | Diff
patch for qtwebkit-2.1, sent by Laszlo / Qi (3.74 KB, patch)
2011-03-04 10:53 PST, Ademar Reis
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Stanislav Paltis 2010-10-29 07:31:40 PDT
With QtMobility SystemInfo module present on the mobile device, proposal is to include Model and Firmware Version information in Webkit Generated USer agent String.
Comment 1 Alexey Proskuryakov 2010-10-29 10:22:51 PDT
Note that this would be detrimental to privacy, see e.g. <http://panopticlick.eff.org/browser-uniqueness.pdf>.
Comment 2 Stanislav Paltis 2010-11-01 11:26:12 PDT
Added [Qt] in the title, as it is related to Qt implementation.
Comment 3 Stanislav Paltis 2010-11-01 11:50:40 PDT
Created attachment 72535 [details]
Implementation proposal v1, wanted to hear comments, if you believe tests are needed, please comment appropriately.
Comment 4 Laszlo Gombos 2010-11-02 02:20:05 PDT
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.
Comment 5 Stanislav Paltis 2010-11-02 09:40:56 PDT
Created attachment 72681 [details]
Patch
Comment 6 Kenneth Rohde Christiansen 2010-11-02 09:43:06 PDT
(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?
Comment 7 Kenneth Rohde Christiansen 2010-11-02 09:46:17 PDT
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.
Comment 8 Stanislav Paltis 2010-11-02 09:49:16 PDT
(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.
Comment 9 Stanislav Paltis 2010-11-02 09:49:59 PDT
(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.
Comment 10 Stanislav Paltis 2010-11-02 09:55:38 PDT
(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.
Comment 11 Kenneth Rohde Christiansen 2010-11-02 09:56:42 PDT
(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.
Comment 12 Kenneth Rohde Christiansen 2010-11-02 09:58:47 PDT
(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? ^
Comment 13 Stanislav Paltis 2010-11-02 10:01:46 PDT
Created attachment 72691 [details]
Patch
Comment 14 Stanislav Paltis 2010-11-02 10:14:14 PDT
(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..
Comment 15 Early Warning System Bot 2010-11-02 11:03:26 PDT
Attachment 72691 [details] did not build on qt:
Build output: http://queues.webkit.org/results/4989016
Comment 16 Simon Hausmann 2010-11-02 12:11:13 PDT
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.
Comment 17 zalan 2010-11-02 12:48:55 PDT
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?
Comment 18 Stanislav Paltis 2010-11-03 08:15:50 PDT
Created attachment 72824 [details]
Patch
Comment 19 Stanislav Paltis 2010-11-03 08:19:34 PDT
(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.
Comment 20 Stanislav Paltis 2010-11-04 08:59:38 PDT
Comment on attachment 72824 [details]
Patch

setting for commit-queue- until decisions are clarified and explained in the bug report.
Comment 21 Laszlo Gombos 2010-11-16 15:30:13 PST
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.
Comment 22 qi 2011-02-17 12:03:42 PST
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"
Comment 23 Alexis Menard (darktears) 2011-02-18 09:37:34 PST
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
Comment 24 Eric Seidel 2011-02-24 03:15:46 PST
There is an ongoing effort to simplify user agent strings.
Comment 25 Peter Kasting 2011-02-24 10:31:06 PST
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.
Comment 26 Laszlo Gombos 2011-02-28 13:23:02 PST
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).
Comment 27 Peter Kasting 2011-02-28 13:26:26 PST
(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).
Comment 28 Ademar Reis 2011-03-03 09:00:14 PST
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.
Comment 29 Ademar Reis 2011-03-03 09:41:35 PST
(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.
Comment 30 qi 2011-03-04 05:59:59 PST
Created attachment 84733 [details]
patch2

Update the patch based on Laszlo's latest comments.
Comment 31 Laszlo Gombos 2011-03-04 06:37:44 PST
Comment on attachment 84733 [details]
patch2

LGTM. r+.
Comment 32 Laszlo Gombos 2011-03-04 07:53:47 PST
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.
Comment 33 Kenneth Rohde Christiansen 2011-03-04 08:17:06 PST
Comment on attachment 84733 [details]
patch2

Weren't we trying to simplify the UA?
Comment 34 qi 2011-03-04 10:06:42 PST
Created attachment 84770 [details]
patch3

Update patch.
Comment 35 Ademar Reis 2011-03-04 10:53:58 PST
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.
Comment 36 Kenneth Rohde Christiansen 2011-03-04 10:58:57 PST
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.
Comment 37 Ademar Reis 2011-03-04 11:55:18 PST
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.
Comment 38 Laszlo Gombos 2011-03-04 14:50:44 PST
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.
Comment 39 WebKit Commit Bot 2011-03-05 09:32:25 PST
Comment on attachment 84770 [details]
patch3

Clearing flags on attachment: 84770

Committed r80424: <http://trac.webkit.org/changeset/80424>
Comment 40 WebKit Commit Bot 2011-03-05 09:32:33 PST
All reviewed patches have been landed.  Closing bug.