Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2175003003:
Use - in place of N/A for not-available values in Task Manager. (Closed)
DescriptionUse - in place of N/A for not-available values in Task Manager.
If you have lots of processes running then N/A makes the available stats
stand out less clearly from the not-available or not-applicable fields.
Using a dash instead makes it easier to glance across the available
stats.
Committed: https://crrev.com/c2180ba7d0f89b8b4dbf47a2adee81794ec74906
Cr-Commit-Position: refs/heads/master@{#414874}
Patch Set 1 #
Total comments: 3
Patch Set 2 : Switch to en-dash and update comment #Messages
Total messages: 23 (12 generated)
wez@chromium.org changed reviewers: + afakhry@chromium.org
WDYT? Personally I find N/A distracting and would prefer - :)
The CQ bit was checked by wez@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
afakhry@chromium.org changed reviewers: + nick@chromium.org
+nick for old TM. https://codereview.chromium.org/2175003003/diff/1/chrome/app/generated_resour... File chrome/app/generated_resources.grd (right): https://codereview.chromium.org/2175003003/diff/1/chrome/app/generated_resour... chrome/app/generated_resources.grd:3421: + <message name="IDS_TASK_MANAGER_NA_CELL_TEXT" desc="The value displayed for network / webcache usage when the information is not available."> This is needed for the new task manager view. See: https://cs.chromium.org/chromium/src/chrome/browser/ui/task_manager/task_mana... If we will go ahead with this change. I don't think there's a need to localize "-"! :D You need to update the comments in: https://cs.chromium.org/chromium/src/chrome/browser/ui/task_manager/task_mana... and https://cs.chromium.org/chromium/src/chrome/browser/ui/task_manager/task_mana... also maybe rename the member |n_a_string_| to something else?
https://codereview.chromium.org/2175003003/diff/1/chrome/app/generated_resour... File chrome/app/generated_resources.grd (right): https://codereview.chromium.org/2175003003/diff/1/chrome/app/generated_resour... chrome/app/generated_resources.grd:3421: + <message name="IDS_TASK_MANAGER_NA_CELL_TEXT" desc="The value displayed for network / webcache usage when the information is not available."> On 2016/07/26 22:55:15, afakhry wrote: > This is needed for the new task manager view. See: > https://cs.chromium.org/chromium/src/chrome/browser/ui/task_manager/task_mana... > > If we will go ahead with this change. I don't think there's a need to localize > "-"! :D You need to update the comments in: > https://cs.chromium.org/chromium/src/chrome/browser/ui/task_manager/task_mana... > and > https://cs.chromium.org/chromium/src/chrome/browser/ui/task_manager/task_mana... > also maybe rename the member |n_a_string_| to something else? Are we sure that there's no need to localize "-" ? Honestly that would surprise me. Also, is this a hyphen-minus? Should it be some kind of fancy unicode dash?
https://codereview.chromium.org/2175003003/diff/1/chrome/app/generated_resour... File chrome/app/generated_resources.grd (right): https://codereview.chromium.org/2175003003/diff/1/chrome/app/generated_resour... chrome/app/generated_resources.grd:3421: + <message name="IDS_TASK_MANAGER_NA_CELL_TEXT" desc="The value displayed for network / webcache usage when the information is not available."> On 2016/07/26 23:18:22, ncarter wrote: > On 2016/07/26 22:55:15, afakhry wrote: > > This is needed for the new task manager view. See: > > > https://cs.chromium.org/chromium/src/chrome/browser/ui/task_manager/task_mana... > > > > If we will go ahead with this change. I don't think there's a need to localize > > "-"! :D You need to update the comments in: > > > https://cs.chromium.org/chromium/src/chrome/browser/ui/task_manager/task_mana... > > and > > > https://cs.chromium.org/chromium/src/chrome/browser/ui/task_manager/task_mana... > > also maybe rename the member |n_a_string_| to something else? It's not clear to me why the current impl localizes some of the N/As but not others - just oversight, perhaps? > Are we sure that there's no need to localize "-" ? Honestly that would surprise > me. Yeah, good question! > Also, is this a hyphen-minus? Should it be some kind of fancy unicode dash? Quite possibly, but which one?!? There are both hyphens (including plain-old HYPHEN (U+2010), SOFT HYPHEN (U+00AD)) and dashes (including EN DASH (U+2013), EM DASH (U+2014), HORIZONTAL BAR (U+2015), and FIGURE DASH (U+2012)). :-/
On 2016/08/01 22:12:27, Wez wrote: > https://codereview.chromium.org/2175003003/diff/1/chrome/app/generated_resour... > File chrome/app/generated_resources.grd (right): > > https://codereview.chromium.org/2175003003/diff/1/chrome/app/generated_resour... > chrome/app/generated_resources.grd:3421: + <message > name="IDS_TASK_MANAGER_NA_CELL_TEXT" desc="The value displayed for network / > webcache usage when the information is not available."> > On 2016/07/26 23:18:22, ncarter wrote: > > On 2016/07/26 22:55:15, afakhry wrote: > > > This is needed for the new task manager view. See: > > > > > > https://cs.chromium.org/chromium/src/chrome/browser/ui/task_manager/task_mana... > > > > > > If we will go ahead with this change. I don't think there's a need to > localize > > > "-"! :D You need to update the comments in: > > > > > > https://cs.chromium.org/chromium/src/chrome/browser/ui/task_manager/task_mana... > > > and > > > > > > https://cs.chromium.org/chromium/src/chrome/browser/ui/task_manager/task_mana... > > > also maybe rename the member |n_a_string_| to something else? > > It's not clear to me why the current impl localizes some of the N/As but not > others - just oversight, perhaps? > > > Are we sure that there's no need to localize "-" ? Honestly that would > surprise > > me. > > Yeah, good question! > > > Also, is this a hyphen-minus? Should it be some kind of fancy unicode dash? > > Quite possibly, but which one?!? There are both hyphens (including plain-old > HYPHEN (U+2010), SOFT HYPHEN (U+00AD)) and dashes (including EN DASH (U+2013), > EM DASH (U+2014), HORIZONTAL BAR (U+2015), and FIGURE DASH (U+2012)). :-/ I'd go with en dash unless em dash really looks right to you. Figure dash is for ranges (12-15 when it means "twelve to fifteen"). Technically there is the TWO EM DASH character that's listed as the 'omission dash' but that's for elisions as in "g---d---mn m---therf---er" and it would be way too wide for our purposes.
The CQ bit was checked by wez@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
On 2016/08/01 23:09:07, ncarter (ooo) wrote: > On 2016/08/01 22:12:27, Wez wrote: > > > https://codereview.chromium.org/2175003003/diff/1/chrome/app/generated_resour... > > File chrome/app/generated_resources.grd (right): > > > > > https://codereview.chromium.org/2175003003/diff/1/chrome/app/generated_resour... > > chrome/app/generated_resources.grd:3421: + <message > > name="IDS_TASK_MANAGER_NA_CELL_TEXT" desc="The value displayed for network / > > webcache usage when the information is not available."> > > On 2016/07/26 23:18:22, ncarter wrote: > > > On 2016/07/26 22:55:15, afakhry wrote: > > > > This is needed for the new task manager view. See: > > > > > > > > > > https://cs.chromium.org/chromium/src/chrome/browser/ui/task_manager/task_mana... > > > > > > > > If we will go ahead with this change. I don't think there's a need to > > localize > > > > "-"! :D You need to update the comments in: > > > > > > > > > > https://cs.chromium.org/chromium/src/chrome/browser/ui/task_manager/task_mana... > > > > and > > > > > > > > > > https://cs.chromium.org/chromium/src/chrome/browser/ui/task_manager/task_mana... > > > > also maybe rename the member |n_a_string_| to something else? > > > > It's not clear to me why the current impl localizes some of the N/As but not > > others - just oversight, perhaps? > > > > > Are we sure that there's no need to localize "-" ? Honestly that would > > surprise > > > me. > > > > Yeah, good question! > > > > > Also, is this a hyphen-minus? Should it be some kind of fancy unicode dash? > > > > Quite possibly, but which one?!? There are both hyphens (including plain-old > > HYPHEN (U+2010), SOFT HYPHEN (U+00AD)) and dashes (including EN DASH (U+2013), > > EM DASH (U+2014), HORIZONTAL BAR (U+2015), and FIGURE DASH (U+2012)). :-/ > > I'd go with en dash unless em dash really looks right to you. Figure dash is for > ranges (12-15 when it means "twelve to fifteen"). Technically there is the TWO > EM DASH character that's listed as the 'omission dash' but that's for elisions > as in "g---d---mn m---therf---er" and it would be way too wide for our purposes. OK, en-dash it is. PTAL; also tweaked the comment wording.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
afakhry: Pingy :)
lgtm
The CQ bit was checked by wez@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Committed patchset #2 (id:20001)
Message was sent while issue was closed.
Description was changed from ========== Use - in place of N/A for not-available values in Task Manager. If you have lots of processes running then N/A makes the available stats stand out less clearly from the not-available or not-applicable fields. Using a dash instead makes it easier to glance across the available stats. ========== to ========== Use - in place of N/A for not-available values in Task Manager. If you have lots of processes running then N/A makes the available stats stand out less clearly from the not-available or not-applicable fields. Using a dash instead makes it easier to glance across the available stats. Committed: https://crrev.com/c2180ba7d0f89b8b4dbf47a2adee81794ec74906 Cr-Commit-Position: refs/heads/master@{#414874} ==========
Message was sent while issue was closed.
Patchset 2 (id:??) landed as https://crrev.com/c2180ba7d0f89b8b4dbf47a2adee81794ec74906 Cr-Commit-Position: refs/heads/master@{#414874} |
