With QtMobility SystemInfo module present on the mobile device, proposal is to include Model and Firmware Version information in Webkit Generated USer agent String.
Note that this would be detrimental to privacy, see e.g. <http://panopticlick.eff.org/browser-uniqueness.pdf>.
Added [Qt] in the title, as it is related to Qt implementation.
Created attachment 72535 [details] Implementation proposal v1, wanted to hear comments, if you believe tests are needed, please comment appropriately.
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.
Created attachment 72681 [details] Patch
(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 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.
(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.
(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.
(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.
(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.
(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? ^
Created attachment 72691 [details] Patch
(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..
Attachment 72691 [details] did not build on qt: Build output: http://queues.webkit.org/results/4989016
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.
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?
Created attachment 72824 [details] Patch
(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 on attachment 72824 [details] Patch setting for commit-queue- until decisions are clarified and explained in the bug report.
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.
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"
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
There is an ongoing effort to simplify user agent strings.
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 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).
(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).
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.
(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.
Created attachment 84733 [details] patch2 Update the patch based on Laszlo's latest comments.
Comment on attachment 84733 [details] patch2 LGTM. r+.
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 on attachment 84733 [details] patch2 Weren't we trying to simplify the UA?
Created attachment 84770 [details] patch3 Update patch.
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 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.
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 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 on attachment 84770 [details] patch3 Clearing flags on attachment: 84770 Committed r80424: <http://trac.webkit.org/changeset/80424>
All reviewed patches have been landed. Closing bug.