Bug 216177 - Web Inspector: Incorrect decimal separator in Network web inspector
Summary: Web Inspector: Incorrect decimal separator in Network web inspector
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Web Inspector (show other bugs)
Version: WebKit Nightly Build
Hardware: All All
: P2 Normal
Assignee: Patrick Angle
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2020-09-04 08:03 PDT by Patrick Angle
Modified: 2020-09-08 11:50 PDT (History)
8 users (show)

See Also:


Attachments
Patch (9.14 KB, patch)
2020-09-04 11:21 PDT, Patrick Angle
no flags Details | Formatted Diff | Diff
With Patch 408000 (43.84 KB, image/png)
2020-09-04 11:22 PDT, Patrick Angle
no flags Details
Patch (9.13 KB, patch)
2020-09-04 12:29 PDT, Patrick Angle
no flags Details | Formatted Diff | Diff
Patch (9.97 KB, patch)
2020-09-04 13:05 PDT, Patrick Angle
no flags Details | Formatted Diff | Diff
If we used full size letters. (47.45 KB, image/png)
2020-09-04 13:19 PDT, Patrick Angle
no flags Details
Patch (10.08 KB, patch)
2020-09-04 14:08 PDT, Patrick Angle
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Patrick Angle 2020-09-04 08:03:21 PDT
<rdar://51698385>
Comment 1 Patrick Angle 2020-09-04 11:21:49 PDT
Created attachment 408000 [details]
Patch
Comment 2 Patrick Angle 2020-09-04 11:22:34 PDT
Created attachment 408001 [details]
With Patch 408000
Comment 3 BJ Burg 2020-09-04 11:31:03 PDT
Comment on attachment 408000 [details]
Patch

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

r=me

> Source/WebInspectorUI/ChangeLog:9
> +            to be correctly formatted for the locale.

Nit: we don't indent into paragraphs in Changelogs.

> Source/WebInspectorUI/ChangeLog:19
> +            bytes into localized strings.

Ditto.

> Source/WebInspectorUI/UserInterface/Views/ResourceSizesContentView.css:85
> +    font-variant-caps: petite-caps;

Hah, nice.
Comment 4 Nikita Vasilyev 2020-09-04 11:34:49 PDT
Comment on attachment 408000 [details]
Patch

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

>> Source/WebInspectorUI/UserInterface/Views/ResourceSizesContentView.css:85
>> +    font-variant-caps: petite-caps;
> 
> Hah, nice.

Why are we doing this? I don't think macOS uses petite-caps anywhere.
Comment 5 Patrick Angle 2020-09-04 11:45:16 PDT
Comment on attachment 408000 [details]
Patch

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

>>> Source/WebInspectorUI/UserInterface/Views/ResourceSizesContentView.css:85
>>> +    font-variant-caps: petite-caps;
>> 
>> Hah, nice.
> 
> Why are we doing this? I don't think macOS uses petite-caps anywhere.

Keeps us much closer to consistent to how the size used to look before the change when it was in a separate element with `font-size: 15px`.
Comment 6 Joseph Pecoraro 2020-09-04 11:51:43 PDT
Honestly I hadn't heard of petite-caps. Having designed this part of the UI (for good or bad!) that definitely what what I was going for =)
Comment 7 Joseph Pecoraro 2020-09-04 12:28:43 PDT
Comment on attachment 408000 [details]
Patch

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

> Source/WebInspectorUI/UserInterface/Views/ResourceSizesContentView.js:171
> +            return WI.UIString("%.0f B").format(bytes).toLowerCase();

Seems this becomes a variant of `Number.bytesToString` in Utilities.js...

Perhaps we should extend Number.bytesToString to allow for a KB threshold or skipping B, which at a quick glance seems to be the only difference I see. This function pushes for "0.N KB" instead of "NNN B" in the 103-1024 range.

I believe I did this strategy of keeping to KB most of the time because if you up/down through a bunch of similarly sized resources around the 1KB range, having the UI change types between B / KB is not as nice as generally keeping it showing in terms of KB.
Comment 8 Patrick Angle 2020-09-04 12:29:45 PDT
Created attachment 408010 [details]
Patch
Comment 9 Nikita Vasilyev 2020-09-04 12:54:55 PDT
Comment on attachment 408000 [details]
Patch

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

>>>> Source/WebInspectorUI/UserInterface/Views/ResourceSizesContentView.css:85
>>>> +    font-variant-caps: petite-caps;
>>> 
>>> Hah, nice.
>> 
>> Why are we doing this? I don't think macOS uses petite-caps anywhere.
> 
> Keeps us much closer to consistent to how the size used to look before the change when it was in a separate element with `font-size: 15px`.

Oh, I see. I slightly misunderstood its purpose initially. Looks good!
Comment 10 Patrick Angle 2020-09-04 13:05:17 PDT
Created attachment 408011 [details]
Patch
Comment 11 Devin Rousso 2020-09-04 13:12:38 PDT
Comment on attachment 408011 [details]
Patch

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

> Source/WebInspectorUI/UserInterface/Base/Utilities.js:1356
> +        if (bytesThreshold === undefined)
> +            bytesThreshold = 1024;

you can use logical assignment operators to do this :)
```
   bytesThreshold ??= 1024;
```

alternatively you could use a default parameter
```
    value (bytes, higherResolution, bytesThreshold = 1024)
```
but we usually prefer doing the fallback inside the function itself

> Source/WebInspectorUI/UserInterface/Views/ResourceSizesContentView.js:170
> +        return Number.bytesToString(bytes, false, 103).toLowerCase();

I don't think `toLowerCase` is a good idea with a `WI.UIString`.  You're making this value `petite-caps` anyways, so why do we need to lowercase it?

NIT: when calling functions in other files with literal values, we usually like to create `const` variables right above so that it's easier to see the intended use of the literal value without having to find where the function is defined (which isn't always easy)
```
    const higherResolution = false;
    const bytesThreshold = 103;
    return Number.bytesToString(bytes, higherResolution, bytesThreshold);
```
Comment 12 Patrick Angle 2020-09-04 13:18:27 PDT
Comment on attachment 408011 [details]
Patch

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

>> Source/WebInspectorUI/UserInterface/Views/ResourceSizesContentView.js:170
>> +        return Number.bytesToString(bytes, false, 103).toLowerCase();
> 
> I don't think `toLowerCase` is a good idea with a `WI.UIString`.  You're making this value `petite-caps` anyways, so why do we need to lowercase it?
> 
> NIT: when calling functions in other files with literal values, we usually like to create `const` variables right above so that it's easier to see the intended use of the literal value without having to find where the function is defined (which isn't always easy)
> ```
>     const higherResolution = false;
>     const bytesThreshold = 103;
>     return Number.bytesToString(bytes, higherResolution, bytesThreshold);
> ```

`petite-caps` only affects lower case letters. The other possible petite option, `all-petite-caps` will affect uppercase and lowercase letters, but also the numbers... We would just use full sized letters here, and it wouldn't look too bad; just different.
Comment 13 Patrick Angle 2020-09-04 13:19:26 PDT
Created attachment 408013 [details]
If we used full size letters.
Comment 14 Patrick Angle 2020-09-04 14:08:32 PDT
Created attachment 408022 [details]
Patch
Comment 15 BJ Burg 2020-09-04 15:50:03 PDT
Comment on attachment 408022 [details]
Patch

r=me
Comment 16 EWS 2020-09-08 11:50:04 PDT
Committed r266735: <https://trac.webkit.org/changeset/266735>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 408022 [details].