Bug 34040 - Web Inspector: Use conditional CSS for defining monospace fonts instead of native code in clients.
Summary: Web Inspector: Use conditional CSS for defining monospace fonts instead of na...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Web Inspector (Deprecated) (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Alexander Pavlov (apavlov)
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2010-01-23 09:59 PST by Pavel Feldman
Modified: 2010-01-28 12:28 PST (History)
6 users (show)

See Also:


Attachments
[PATCH] Proposed solution (11.32 KB, patch)
2010-01-28 06:19 PST, Alexander Pavlov (apavlov)
pfeldman: review-
Details | Formatted Diff | Diff
[PATCH] Comments addressed (10.23 KB, patch)
2010-01-28 07:57 PST, Alexander Pavlov (apavlov)
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
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.