Bug 216177 – Web Inspector: Incorrect decimal separator in Network web inspector

• Component: Web Inspector
• Status: RESOLVED FIXED

[Patrick Angle]

<rdar://51698385>

[Patrick Angle]

Created attachment 408000 [details]
Patch

[Patrick Angle]

Created attachment 408001 [details]
With Patch 408000

[Blaze Burg]

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.

[Nikita Vasilyev]

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.

[Patrick Angle]

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`.

[Joseph Pecoraro]

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 =)

[Joseph Pecoraro]

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.

[Patrick Angle]

Created attachment 408010 [details]
Patch

[Nikita Vasilyev]

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!

[Patrick Angle]

Created attachment 408011 [details]
Patch

[Devin Rousso]

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);
```

[Patrick Angle]

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.

[Patrick Angle]

Created attachment 408013 [details]
If we used full size letters.

[Patrick Angle]

Created attachment 408022 [details]
Patch

[Blaze Burg]

Comment on attachment 408022 [details]
Patch

r=me

[EWS]

Committed r266735: <https://trac.webkit.org/changeset/266735>

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