Bug 31961 - [Qt][Symbian] Report SymbianOS in user agent string for Symbian
Summary: [Qt][Symbian] Report SymbianOS in user agent string for Symbian
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Page Loading (show other bugs)
Version: 528+ (Nightly build)
Hardware: PC S60 3rd edition
: P2 Normal
Assignee: Nobody
URL:
Keywords: Qt
Depends on:
Blocks: 27065
  Show dependency treegraph
 
Reported: 2009-11-29 12:46 PST by Laszlo Gombos
Modified: 2009-11-30 15:20 PST (History)
3 users (show)

See Also:


Attachments
1st try (3.08 KB, patch)
2009-11-29 13:15 PST, Laszlo Gombos
hausmann: review-
Details | Formatted Diff | Diff
2nd try (3.08 KB, patch)
2009-11-29 19:27 PST, Laszlo Gombos
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Laszlo Gombos 2009-11-29 12:46:34 PST
In general the guideline is that we want a user agent string that is consistent with (but distinguishable from) previous S60 WebKit ports. In addition we want a user agent string that allows client side user agent detection (http://www.quirksmode.org/js/detect.html is probably one good test).

I believe Abhinav Mithal has a patch for this - I contacted him to see if he can upload his proposed changes.
Comment 1 Laszlo Gombos 2009-11-29 13:15:29 PST
Created attachment 43991 [details]
1st try

Abhinav is out for the rest of the year, I uploading his patch on his behalf.
Comment 2 Simon Hausmann 2009-11-29 15:53:41 PST
Comment on attachment 43991 [details]
1st try

In principle I think this patch looks good, but there are a few style issues:

> +    // Placeholder for SubPlatfrom Version

from -> form

> +    "%4; ");
> +
> +// Platform Version

This should be indented 4 spaces

> +    QString osVer;
> +#ifdef Q_OS_SYMBIAN
> +    QSysInfo::SymbianVersion symbianVersion = QSysInfo::symbianVersion();
> +switch (symbianVersion) {

This line is also missing indentation.

> +  case QSysInfo::SV_9_2:
> +      osVer = "/9.2";
> +      break;
> +  case QSysInfo::SV_9_3:
> +      osVer = "/9.3";
> +      break;
> +  case QSysInfo::SV_9_4:
> +      osVer = "/9.4";
> +      break;
> +  default: 
> +      osVer = "Unknown";
> +    }
> +#else
> +    osVer = "";
> +#endif

I suggest to omit the #else block that assigns an empty string
to the string that is already a null string.

> +// SubPlatform Version

Indentation

> +    QString subPlatformVer;
> +#ifdef Q_OS_SYMBIAN
> +    QSysInfo::S60Version s60Version = QSysInfo::s60Version();
> +switch (s60Version) {

Indentation

> +  case QSysInfo::SV_S60_3_1:
> +      subPlatformVer = "/3.1";
> +      break;
> +  case QSysInfo::SV_S60_3_2:
> +      subPlatformVer = "/3.2";
> +      break;
> +  case QSysInfo::SV_S60_5_0:
> +      subPlatformVer = "/5.0";
> +      break;
> +  default: 
> +      subPlatformVer = " Unknown";
> +    }
> +#else
> +    subPlatformVer = "";
> +#endif

Same as above, the assignment is unnecessary.
Comment 3 Laszlo Gombos 2009-11-29 19:27:33 PST
Created attachment 44005 [details]
2nd try

Incorporate Simon's review comments.
Comment 4 Adam Barth 2009-11-30 12:48:55 PST
style-queue ran check-webkit-style on attachment 44005 [details] without any errors.
Comment 5 WebKit Commit Bot 2009-11-30 15:20:11 PST
Comment on attachment 44005 [details]
2nd try

Clearing flags on attachment: 44005

Committed r51515: <http://trac.webkit.org/changeset/51515>
Comment 6 WebKit Commit Bot 2009-11-30 15:20:16 PST
All reviewed patches have been landed.  Closing bug.