Bug 113047 – Web Inspector: Fonts refactoring

• Status: RESOLVED
• Resolution: FIXED
• Component: Web Inspector (Deprecated)

[Alexei Filippov]

Unify fonts usage across inspector.
Make inspector default font depend on platform.

[Alexei Filippov]

Created attachment 194520 [details]
Patch

[Pavel Feldman]

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.

[Alexei Filippov]

Created attachment 194594 [details]
Patch

[Pavel Feldman]

Comment on attachment 194594 [details]
Patch

I don't see where you addressed the comments.

[Alexei Filippov]

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

[WebKit Review Bot]

Comment on attachment 194594 [details]
Patch

Clearing flags on attachment: 194594

Committed r146767: <http://trac.webkit.org/changeset/146767>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[WebKit Review Bot]

Re-opened since this is blocked by bug 113295

[Alexei Filippov]

Created attachment 195761 [details]
Patch

[Alexei Filippov]

Created attachment 195775 [details]
Patch

[Alexei Filippov]

Created attachment 195780 [details]
Patch

[WebKit Review Bot]

Comment on attachment 195780 [details]
Patch

Clearing flags on attachment: 195780

Committed r147275: <http://trac.webkit.org/changeset/147275>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Timothy Hatcher]

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.

[Timothy Hatcher]

Sorry, I didn't see the follow up yet. http://trac.webkit.org/changeset/146871

[Timothy Hatcher]

Actually no, r147275 has the same issue and didn't fix things after the first roll out.

[Timothy Hatcher]

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.

[Timothy Hatcher]

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.