[webkit-reviews] review denied: [Bug 34040] Web Inspector: Use conditional CSS for defining monospace fonts instead of native code in clients. : [Attachment 47614] [PATCH] Proposed solution

[bugzilla-daemon at webkit.org]

Pavel Feldman <pfeldman at chromium.org> has denied Alexander Pavlov (apavlov)
<apavlov at chromium.org>'s request for review:
Bug 34040: Web Inspector: Use conditional CSS for defining monospace fonts
instead of native code in clients.
https://bugs.webkit.org/show_bug.cgi?id=34040

Attachment 47614: [PATCH] Proposed solution
https://bugs.webkit.org/attachment.cgi?id=47614&action=review

------- Additional Comments from Pavel Feldman <pfeldman at chromium.org>
>  
> -body.platform-windows .source-code {
> -    font-family: Consolas, Lucida Console, monospace;
> +body.platform-mac-tiger .source-code, body.platform-mac-leopard .source-code
{
> +    font-size: 10px;
> +    font-family: Monaco, monospace;
> +}

Could you group definitions for monospace and source-code?

>	   if (!("_platform" in this))
> -	       this._platform = InspectorFrontendHost.platform();
> -
> +	       this._detectPlatform();

I liked the lazy init pattern, should detectPlatform return a value?

>	   return this._platform;
>      },
>  
> +    _detectPlatform: function()
> +    {

> +
> +	   function getOSFromUserAgent(userAgent)
> +	   {
> +	       if (userAgent) {
> +		   var platformInfo =
userAgent.match(/\(([^\)]+);\s*([^\)]+);\s*([^\)]+);\s*([^\)]+)\s*\)/);
> +		   if (!platformInfo)
> +		       return;
> +		   return platformInfo[3].trim();
> +	       }
> +	       return null;
> +	   }
> +

I am not really sure whether this is needed. Why not to match Mac/Windows
against original user agent?