Bug 34040

Summary: Web Inspector: Use conditional CSS for defining monospace fonts instead of native code in clients.
Product: WebKit Reporter: Pavel Feldman <pfeldman>
Component: Web Inspector (Deprecated)Assignee: Alexander Pavlov (apavlov) <apavlov>
Status: RESOLVED FIXED    
Severity: Normal CC: bweinstein, commit-queue, joepeck, mitz, rik, timothy
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: All   
OS: All   
Attachments:
Description Flags
[PATCH] Proposed solution
pfeldman: review-
[PATCH] Comments addressed none

Description Pavel Feldman 2010-01-23 09:59:50 PST
That would allow inspector to define its looks for platforms and would make sure inspectors on all macs look alike (no matter whether it is WebKit/Safari/Chromium/etc.
Comment 1 Alexander Pavlov (apavlov) 2010-01-28 06:19:29 PST
Created attachment 47614 [details]
[PATCH] Proposed solution
Comment 2 Pavel Feldman 2010-01-28 06:50:28 PST
Comment on attachment 47614 [details]
[PATCH] Proposed solution

>  
> -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?
Comment 3 Alexander Pavlov (apavlov) 2010-01-28 07:57:35 PST
Created attachment 47619 [details]
[PATCH] Comments addressed
Comment 4 mitz 2010-01-28 08:23:59 PST
Comment on attachment 47619 [details]
[PATCH] Comments addressed

Isn’t this going to result in the wrong font size being used on Mac OS X versions later than 10.6?
Comment 5 WebKit Commit Bot 2010-01-28 08:28:47 PST
Comment on attachment 47619 [details]
[PATCH] Comments addressed

Clearing flags on attachment: 47619

Committed r54001: <http://trac.webkit.org/changeset/54001>
Comment 6 WebKit Commit Bot 2010-01-28 08:28:54 PST
All reviewed patches have been landed.  Closing bug.
Comment 7 Timothy Hatcher 2010-01-28 12:25:54 PST
(In reply to comment #4)
> (From update of attachment 47619 [details])
> Isn’t this going to result in the wrong font size being used on Mac OS X
> versions later than 10.6?

No.

body.platform-mac-snowleopard .monospace,
body.platform-mac-snowleopard .source-code {
    font-size: 11px;
    font-family: Menlo, monospace;
}
Comment 8 mitz 2010-01-28 12:28:21 PST
(In reply to comment #7)
> (In reply to comment #4)
> > (From update of attachment 47619 [details] [details])
> > Isn’t this going to result in the wrong font size being used on Mac OS X
> > versions later than 10.6?
> 
> No.
> 
> body.platform-mac-snowleopard .monospace,
> body.platform-mac-snowleopard .source-code {
>     font-size: 11px;
>     font-family: Menlo, monospace;
> }

I was talking about the earlier version of the patch that sent anything above 10.6 to the “Mac unknown” path, which was resulting in 10px.