Bug 113047

Summary: Web Inspector: Fonts refactoring
Product: WebKit Reporter: Alexei Filippov <alph>
Component: Web Inspector (Deprecated)Assignee: Alexei Filippov <alph>
Status: RESOLVED FIXED    
Severity: Normal CC: apavlov, keishi, loislo, pfeldman, pmuellr, timothy, vsevik, web-inspector-bugs, webkit.review.bot, yurys
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: All   
OS: All   
Bug Depends on: 113295    
Bug Blocks:    
Attachments:
Description Flags
Patch
none
Patch
none
Patch
none
Patch
none
Patch none

Description Alexei Filippov 2013-03-22 05:32:57 PDT
Unify fonts usage across inspector.
Make inspector default font depend on platform.
Comment 1 Alexei Filippov 2013-03-22 05:35:18 PDT
Created attachment 194520 [details]
Patch
Comment 2 Pavel Feldman 2013-03-22 08:21:09 PDT
Comment on attachment 194520 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=194520&action=review

> Source/WebCore/inspector/front-end/dataGrid.css:104
> +    padding: 1px 4px;

Did you test alignment on Mac and Windows/Linux?

> Source/WebCore/inspector/front-end/networkLogView.css:16
> +    -webkit-background-size: 1px 38px;

Sounds like a significant change. Why?

> Source/WebKit/chromium/src/js/devTools.css:15
> +body.platform-linux {

You remove font size settings from all platforms, but only add default values for Chromium.
Comment 3 Alexei Filippov 2013-03-22 10:47:07 PDT
Created attachment 194594 [details]
Patch
Comment 4 Pavel Feldman 2013-03-23 01:15:27 PDT
Comment on attachment 194594 [details]
Patch

I don't see where you addressed the comments.
Comment 5 Alexei Filippov 2013-03-25 05:00:44 PDT
Comment on attachment 194520 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=194520&action=review

>> Source/WebCore/inspector/front-end/dataGrid.css:104
>> +    padding: 1px 4px;
> 
> Did you test alignment on Mac and Windows/Linux?

Yes.

>> Source/WebCore/inspector/front-end/networkLogView.css:16
>> +    -webkit-background-size: 1px 38px;
> 
> Sounds like a significant change. Why?

I decided to make it more compact... but you're right it's unrelated to this patch. Reverted.

>> Source/WebKit/chromium/src/js/devTools.css:15
>> +body.platform-linux {
> 
> You remove font size settings from all platforms, but only add default values for Chromium.

I don't. There is:
body {
  font-family: Lucida Grande, sans-serif;
  font-size: 11px;
}
in inspectorCommon.css
Comment 6 WebKit Review Bot 2013-03-25 06:59:13 PDT
Comment on attachment 194594 [details]
Patch

Clearing flags on attachment: 194594

Committed r146767: <http://trac.webkit.org/changeset/146767>
Comment 7 WebKit Review Bot 2013-03-25 06:59:17 PDT
All reviewed patches have been landed.  Closing bug.
Comment 8 WebKit Review Bot 2013-03-26 04:59:25 PDT
Re-opened since this is blocked by bug 113295
Comment 9 Alexei Filippov 2013-03-29 10:22:19 PDT
Created attachment 195761 [details]
Patch
Comment 10 Alexei Filippov 2013-03-29 12:01:51 PDT
Created attachment 195775 [details]
Patch
Comment 11 Alexei Filippov 2013-03-29 12:32:36 PDT
Created attachment 195780 [details]
Patch
Comment 12 WebKit Review Bot 2013-03-30 01:22:59 PDT
Comment on attachment 195780 [details]
Patch

Clearing flags on attachment: 195780

Committed r147275: <http://trac.webkit.org/changeset/147275>
Comment 13 WebKit Review Bot 2013-03-30 01:23:03 PDT
All reviewed patches have been landed.  Closing bug.
Comment 14 Timothy Hatcher 2013-04-01 06:36:31 PDT
Comment on attachment 195780 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=195780&action=review

> Source/WebKit/chromium/src/js/devTools.css:30
> +body.platform-linux {
> +  color: rgb(48, 57, 66);
> +  font-family: Ubuntu, Arial, sans-serif;
> +  font-size: 12px;
> +}
> +
> +body.platform-mac {
> +  color: rgb(48, 57, 66);
> +  font-family: 'Lucida Grande', sans-serif;
> +  font-size: 12px;
> +}
> +
> +body.platform-windows {
> +  font-family: 'Segoe UI', Tahoma, sans-serif;
> +  font-size: 12px;
> +}

This should not be in a Chromium file, it should be in inspector.css. You removed font-family from all files and only declare them here. Now the Inspector is likely going to use Times on all non-chromium platforms. Please fix in a follow up, I'm sure Qt and GTK will care.
Comment 15 Timothy Hatcher 2013-04-01 06:38:46 PDT
Sorry, I didn't see the follow up yet. http://trac.webkit.org/changeset/146871
Comment 16 Timothy Hatcher 2013-04-01 06:40:35 PDT
Actually no, r147275 has the same issue and didn't fix things after the first roll out.
Comment 17 Timothy Hatcher 2013-04-01 06:51:00 PDT
The change you say was in inspectorCommon.css never was in any of the attached patches. Yet this was r+ed. Preparing a roll out.
Comment 18 Timothy Hatcher 2013-04-01 07:01:02 PDT
As Pavel pointed out on IRC, inspectorCommon.css has always had the base font. I was confused and thought the patch was suppose to contain it was wasn't seeing it.

Sorry for the noise.