<rdar://67800281>
Created attachment 407906 [details] Patch
Comment on attachment 407906 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=407906&action=review > Source/WebInspectorUI/Localizations/en.lproj/localizedStrings.js:685 > +localizedStrings["High @ Energy Impact"] = "High"; Can we have a more descriptive keys/comments that includes where in the UI these are found and more of a description of what it's supposed to mean? e.g. ``` WI.UIString("Low", "Low @ CPU Timeline Energy Impact", "Label indicating that the CPU activity was low during the selected range of the selected timeline recording."); ``` > Source/WebInspectorUI/UserInterface/Views/CPUTimelineView.js:1114 > + if (average === 0) > + this._energyChart.value = 0; > + else > + this._energyChart.value = mapWithBias(average, 0, CPUTimelineView.lowEnergyThreshold, 0, CPUTimelineView.lowEnergyGraphBoundary, 0.85); NIT: i think we can just drop the `if (average === 0)` branch, as `mapWithBias` should return `0` if `average` is `0`
Comment on attachment 407906 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=407906&action=review >> Source/WebInspectorUI/Localizations/en.lproj/localizedStrings.js:685 >> +localizedStrings["High @ Energy Impact"] = "High"; > > Can we have a more descriptive keys/comments that includes where in the UI these are found and more of a description of what it's supposed to mean? > > e.g. > ``` > WI.UIString("Low", "Low @ CPU Timeline Energy Impact", "Label indicating that the CPU activity was low during the selected range of the selected timeline recording."); > ``` The goal of this patch is to provide the exact noun the adjective is used with. I'll provide an example for Russian, which has a concept of grammatical gender (https://en.wikipedia.org/wiki/Grammatical_gender): Impact - влияние (оно, neutral grammatical gender) high impact - высокое влияние priority - приоритет (он, masculine grammatical gender) high priority - высокий приоритет The important part here is that "high" is translated differently, "высокОЕ" vs "высокИЙ". --- The description you provided looks like a documentation. I wouldn't know what grammatical gender to use from it. "Energy Impact: High" is slightly excessive already. All required is a noun — "Impact".
Comment on attachment 407906 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=407906&action=review >>> Source/WebInspectorUI/Localizations/en.lproj/localizedStrings.js:685 >>> +localizedStrings["High @ Energy Impact"] = "High"; >> >> Can we have a more descriptive keys/comments that includes where in the UI these are found and more of a description of what it's supposed to mean? >> >> e.g. >> ``` >> WI.UIString("Low", "Low @ CPU Timeline Energy Impact", "Label indicating that the CPU activity was low during the selected range of the selected timeline recording."); >> ``` > > The goal of this patch is to provide the exact noun the adjective is used with. > > I'll provide an example for Russian, which has a concept of grammatical gender (https://en.wikipedia.org/wiki/Grammatical_gender): > > Impact - влияние (оно, neutral grammatical gender) > high impact - высокое влияние > > priority - приоритет (он, masculine grammatical gender) > high priority - высокий приоритет > > The important part here is that "high" is translated differently, "высокОЕ" vs "высокИЙ". > > --- > > The description you provided looks like a documentation. I wouldn't know what grammatical gender to use from it. > > "Energy Impact: High" is slightly excessive already. All required is a noun — "Impact". While I understand Nikita's point, the reason for the key to exist is for localizers to be able to find the string in the user interface. A comment would be more appropriate to describe the nouns it should be used with. For example, the comment here could be "Label for timeline samples that have High energy impact." This matches the approach used for other localizable apps / localizable.strings files.
Comment on attachment 407906 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=407906&action=review >>>> Source/WebInspectorUI/Localizations/en.lproj/localizedStrings.js:685 >>>> +localizedStrings["High @ Energy Impact"] = "High"; >>> >>> Can we have a more descriptive keys/comments that includes where in the UI these are found and more of a description of what it's supposed to mean? >>> >>> e.g. >>> ``` >>> WI.UIString("Low", "Low @ CPU Timeline Energy Impact", "Label indicating that the CPU activity was low during the selected range of the selected timeline recording."); >>> ``` >> >> The goal of this patch is to provide the exact noun the adjective is used with. >> >> I'll provide an example for Russian, which has a concept of grammatical gender (https://en.wikipedia.org/wiki/Grammatical_gender): >> >> Impact - влияние (оно, neutral grammatical gender) >> high impact - высокое влияние >> >> priority - приоритет (он, masculine grammatical gender) >> high priority - высокий приоритет >> >> The important part here is that "high" is translated differently, "высокОЕ" vs "высокИЙ". >> >> --- >> >> The description you provided looks like a documentation. I wouldn't know what grammatical gender to use from it. >> >> "Energy Impact: High" is slightly excessive already. All required is a noun — "Impact". > > While I understand Nikita's point, the reason for the key to exist is for localizers to be able to find the string in the user interface. A comment would be more appropriate to describe the nouns it should be used with. For example, the comment here could be "Label for timeline samples that have High energy impact." This matches the approach used for other localizable apps / localizable.strings files. So... I think your comments and string values are good, just the key should be more like Devin suggested. Thanks for fixing this!
(In reply to Brian Burg from comment #4) > While I understand Nikita's point, the reason for the key to exist is for > localizers to be able to find the string in the user interface. I didn't consider that. What about the one I named "High @ Network Priority"? It's used in several places: Sources details sidebar, Network tab, and Timelines tab - Network Requests. I was using keys just specific enough to distinguish the same UIStrings from each other.
Created attachment 407945 [details] Patch
Comment on attachment 407945 [details] Patch r=me
Committed r266622: <https://trac.webkit.org/changeset/266622> All reviewed patches have been landed. Closing bug and clearing flags on attachment 407945 [details].