[webkit-reviews] review denied: [Bug 31961] [Qt][Symbian] Report SymbianOS in user agent string for Symbian : [Attachment 43991] 1st try

[bugzilla-daemon at webkit.org]

Simon Hausmann <hausmann at webkit.org> has denied Laszlo Gombos
<laszlo.1.gombos at nokia.com>'s request for review:
Bug 31961: [Qt][Symbian] Report SymbianOS in user agent string for Symbian
https://bugs.webkit.org/show_bug.cgi?id=31961

Attachment 43991: 1st try
https://bugs.webkit.org/attachment.cgi?id=43991&action=review

------- Additional Comments from Simon Hausmann <hausmann at webkit.org>
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.