Bug 224900 - Web Inspector: Network: rename "XHR" to "XHR/Fetch"
Summary: Web Inspector: Network: rename "XHR" to "XHR/Fetch"
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: Devin Rousso
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2021-04-21 15:32 PDT by Maximiliano Firtman
Modified: 2021-05-10 13:35 PDT (History)
7 users (show)

See Also:


Attachments
Patch (12.14 KB, patch)
2021-04-22 20:00 PDT, Devin Rousso
no flags Details | Formatted Diff | Diff
Patch (12.12 KB, patch)
2021-05-03 19:26 PDT, Devin Rousso
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Maximiliano Firtman 2021-04-21 15:32:37 PDT
When you want to filter network requests in the Network panel and see only requests done by the fetch API you have to select the filter "XHR"

While XMLHttpRequest is a big part of front-end history and it's still available, the Fetch API is now replacing it. In my experience as a trainer, I've found lately that many web developers with 2 years of experience in the field don't recognize XHR as something they understand as they've never dealt with XHR requests using that API.

It may be a good time to rename the filter to "Fetch" or at least something like "XHR/Fetch"
Comment 1 Devin Rousso 2021-04-22 20:00:12 PDT
Created attachment 426883 [details]
Patch
Comment 2 Radar WebKit Bug Importer 2021-04-28 15:33:31 PDT
<rdar://problem/77288691>
Comment 3 Patrick Angle 2021-04-28 15:42:37 PDT
Comment on attachment 426883 [details]
Patch

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

> Source/WebInspectorUI/UserInterface/Views/NetworkTableContentView.js:66
> +        function createScopeBarItem(id, label, checker) {

I think `pushScopeBarItem` or `addScopeBarItem` would be a more accurate name for this function. `create...` makes me think it's going to only create the item, not also add it to the scope bar.

> Source/WebInspectorUI/UserInterface/Views/NetworkTableContentView.js:76
> +        createScopeBarItem("xhr-fetch", WI.UIString("%s/Fetch", "%s/Fetch @ Network Tab Table Filter", "Scope bar button that filter for dynamic resource loads, like from the 'fetch' method.").format(WI.unlocalizedString("XHR")), (type) => type === WI.Resource.Type.XHR || type === WI.Resource.Type.Fetch);

Should `Fetch` really be localized since it's the name of an API?
Comment 4 Patrick Angle 2021-04-28 16:01:17 PDT
Comment on attachment 426883 [details]
Patch

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

>> Source/WebInspectorUI/UserInterface/Views/NetworkTableContentView.js:76
>> +        createScopeBarItem("xhr-fetch", WI.UIString("%s/Fetch", "%s/Fetch @ Network Tab Table Filter", "Scope bar button that filter for dynamic resource loads, like from the 'fetch' method.").format(WI.unlocalizedString("XHR")), (type) => type === WI.Resource.Type.XHR || type === WI.Resource.Type.Fetch);
> 
> Should `Fetch` really be localized since it's the name of an API?

I realize my question here is slightly ambiguous... Rather, shouldn't we not localize `fetch` because it is API, similar to `XHR`? It seems odd to localize one and not the other given how much they have in common.

> Source/WebInspectorUI/UserInterface/Views/NetworkTableContentView.js:221
> +            return WI.UIString("fetch", "fetch @ Network Tab Resource Type Column Value", "Shown in the 'Type' column of the Network Table for resources loaded via the 'fetch' method.");

Ditto :76
Comment 5 Devin Rousso 2021-05-03 19:24:45 PDT
Comment on attachment 426883 [details]
Patch

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

>>> Source/WebInspectorUI/UserInterface/Views/NetworkTableContentView.js:76
>>> +        createScopeBarItem("xhr-fetch", WI.UIString("%s/Fetch", "%s/Fetch @ Network Tab Table Filter", "Scope bar button that filter for dynamic resource loads, like from the 'fetch' method.").format(WI.unlocalizedString("XHR")), (type) => type === WI.Resource.Type.XHR || type === WI.Resource.Type.Fetch);
>> 
>> Should `Fetch` really be localized since it's the name of an API?
> 
> I realize my question here is slightly ambiguous... Rather, shouldn't we not localize `fetch` because it is API, similar to `XHR`? It seems odd to localize one and not the other given how much they have in common.

Hmm, that's a valid question.  I'd personally rather change that in a separate patch tho, as there's probably similar things like this elsewhere in Web Inspector that we could/should localize.
Comment 6 Devin Rousso 2021-05-03 19:26:19 PDT
Created attachment 427631 [details]
Patch
Comment 7 Patrick Angle 2021-05-03 20:17:51 PDT
Comment on attachment 427631 [details]
Patch

LGTM, provisional r+.
Comment 8 Timothy Hatcher 2021-05-10 11:33:21 PDT
Comment on attachment 427631 [details]
Patch

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

> Source/WebInspectorUI/UserInterface/Views/NetworkTableContentView.js:206
> +            return WI.unlocalizedString("css");

Why lowercase on all these? I wouldn't change the case.
Comment 9 Devin Rousso 2021-05-10 11:34:55 PDT
Comment on attachment 427631 [details]
Patch

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

>> Source/WebInspectorUI/UserInterface/Views/NetworkTableContentView.js:206
>> +            return WI.unlocalizedString("css");
> 
> Why lowercase on all these? I wouldn't change the case.

These we're all made lowercase at the callsite (old line 204).  I feel like it's safer to have localizers do a translation of the lowercase value rather than rely on JavaScript to do it for us.
Comment 10 Timothy Hatcher 2021-05-10 11:36:32 PDT
Comment on attachment 427631 [details]
Patch

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

> Source/WebInspectorUI/UserInterface/Views/NetworkTableContentView.js:76
> +        addScopeBarItem("xhr-fetch", WI.UIString("%s/Fetch", "%s/Fetch @ Network Tab Table Filter", "Scope bar button that filter for dynamic resource loads, like from the 'fetch' method.").format(WI.unlocalizedString("XHR")), (type) => type === WI.Resource.Type.XHR || type === WI.Resource.Type.Fetch);

Localization would just prefer this to not be a format string, I think. Why not just make this "XHR/Fetch" in the loc?
Comment 11 Timothy Hatcher 2021-05-10 11:37:28 PDT
Comment on attachment 427631 [details]
Patch

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

>>> Source/WebInspectorUI/UserInterface/Views/NetworkTableContentView.js:206
>>> +            return WI.unlocalizedString("css");
>> 
>> Why lowercase on all these? I wouldn't change the case.
> 
> These we're all made lowercase at the callsite (old line 204).  I feel like it's safer to have localizers do a translation of the lowercase value rather than rely on JavaScript to do it for us.

Ah, eww.
Comment 12 Devin Rousso 2021-05-10 11:38:30 PDT
Comment on attachment 427631 [details]
Patch

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

>> Source/WebInspectorUI/UserInterface/Views/NetworkTableContentView.js:76
>> +        addScopeBarItem("xhr-fetch", WI.UIString("%s/Fetch", "%s/Fetch @ Network Tab Table Filter", "Scope bar button that filter for dynamic resource loads, like from the 'fetch' method.").format(WI.unlocalizedString("XHR")), (type) => type === WI.Resource.Type.XHR || type === WI.Resource.Type.Fetch);
> 
> Localization would just prefer this to not be a format string, I think. Why not just make this "XHR/Fetch" in the loc?

I was trying to match the existing behavior of "XHR" not being localized.  I'm fine to change this, but I'd personally rather have that be a followup as I'm sure there's other places this (or things like it) are used that we may want to localize as well.
Comment 13 Timothy Hatcher 2021-05-10 11:38:41 PDT
Comment on attachment 427631 [details]
Patch

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

>> Source/WebInspectorUI/UserInterface/Views/NetworkTableContentView.js:76
>> +        addScopeBarItem("xhr-fetch", WI.UIString("%s/Fetch", "%s/Fetch @ Network Tab Table Filter", "Scope bar button that filter for dynamic resource loads, like from the 'fetch' method.").format(WI.unlocalizedString("XHR")), (type) => type === WI.Resource.Type.XHR || type === WI.Resource.Type.Fetch);
> 
> Localization would just prefer this to not be a format string, I think. Why not just make this "XHR/Fetch" in the loc?

If the reason is to not localize XHR and allow Fetch to be localized, Fetch is a technical term too, and likely would be unlocalized.
Comment 14 Devin Rousso 2021-05-10 13:31:34 PDT
Comment on attachment 427631 [details]
Patch

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

>>>> Source/WebInspectorUI/UserInterface/Views/NetworkTableContentView.js:76
>>>> +        addScopeBarItem("xhr-fetch", WI.UIString("%s/Fetch", "%s/Fetch @ Network Tab Table Filter", "Scope bar button that filter for dynamic resource loads, like from the 'fetch' method.").format(WI.unlocalizedString("XHR")), (type) => type === WI.Resource.Type.XHR || type === WI.Resource.Type.Fetch);
>>> 
>>> Localization would just prefer this to not be a format string, I think. Why not just make this "XHR/Fetch" in the loc?
>> 
>> I was trying to match the existing behavior of "XHR" not being localized.  I'm fine to change this, but I'd personally rather have that be a followup as I'm sure there's other places this (or things like it) are used that we may want to localize as well.
> 
> If the reason is to not localize XHR and allow Fetch to be localized, Fetch is a technical term too, and likely would be unlocalized.

We localize "Fetch" elsewhere in Web Inspector (e.g. `WI.repeatedUIString.fetch`).  I was trying to preserve that behavior too.  tbh all of the situations like this probably need to be reexamined <https://webkit.org/b/225608>
Comment 15 EWS 2021-05-10 13:35:18 PDT
Committed r277289 (237552@main): <https://commits.webkit.org/237552@main>

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