Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1989563002:
i18n of Zoom % to use locally correct numeric glyphs (Closed)
|
Created:
4 years, 7 months ago by Greg Levin Modified:
4 years, 7 months ago CC:
chromium-reviews, dbeam+watch-options_chromium.org, tfarina, michaelpg+watch-options_chromium.org, jshin+watch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
Descriptioni18n of Zoom % to use locally correct numeric glyphs
BUG=475228
TEST=Switch to language with non-Arabic numerals (e.g., Persian), check
that Zoom percentages are rendered with non-Arabic numerals. See issue
for list of places in UI where Zoom % appears.
Committed: https://crrev.com/945ea3ae79096e02e4a86dafc90e59a26609db59
Cr-Commit-Position: refs/heads/master@{#395841}
Patch Set 1 #
Total comments: 15
Patch Set 2 : Address reviews (and merge) #
Total comments: 13
Patch Set 3 : Address reviews, fix Mac compile #
Total comments: 3
Patch Set 4 : Add unit test for FormatPercent() #
Total comments: 1
Messages
Total messages: 35 (11 generated)
glevin@chromium.org changed reviewers: + jshin@chromium.org, pkasting@chromium.org, stevenjb@chromium.org
jshin@ - base/i18n/ stevenjb@ - chrome/browser/ui/webui/options/ pkasting@ - everything else Please have a look! https://codereview.chromium.org/1989563002/diff/1/base/i18n/number_formatting.cc File base/i18n/number_formatting.cc (right): https://codereview.chromium.org/1989563002/diff/1/base/i18n/number_formatting... base/i18n/number_formatting.cc:89: return FormatPercentFromDouble(static_cast<double>(number) / 100.0, 0); I don't totally understand why the NumberFormat objects are being cached, and separately. Is it okay that I have FormatPercent() and FormatPercentFromDouble() using the same cached formatter? I'm not sure that any code will need to access FormatPercentFromDouble() externally, but since I had to do the int->double conversion anyway, I thought I'd make it available. https://codereview.chromium.org/1989563002/diff/1/chrome/app/generated_resour... File chrome/app/generated_resources.grd (left): https://codereview.chromium.org/1989563002/diff/1/chrome/app/generated_resour... chrome/app/generated_resources.grd:7183: - Zoom: <ph name="VALUE">$1<ex>100</ex></ph>% zoom_decoration.mm also uses IDS_TOOLTIP_ZOOM: https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/ui/... I'm reaching out to the cocoa team to see how they want to deal with changing the string. https://codereview.chromium.org/1989563002/diff/1/chrome/browser/ui/toolbar/a... File chrome/browser/ui/toolbar/app_menu_model.cc (right): https://codereview.chromium.org/1989563002/diff/1/chrome/browser/ui/toolbar/a... chrome/browser/ui/toolbar/app_menu_model.cc:943: zoom_label_ = base::FormatPercent(zoom_percent); I don't know what this change does, or if it's necessary. The string in app_menu.cc is the one that actually appears in the menu. Does this string appear anywhere in the UI?
Our current use cases for percent format (we always use integer percent) does not require adding a helper function specifically for percent format. As I wrote below, one can just use |MessageFormatter| with a light-weight wrapper if you want to. BTW, I've just filed an upstream bug to make MessageFormat more flexible with percent format (again, we don't need that feature at the moment. ) https://codereview.chromium.org/1989563002/diff/1/base/i18n/number_formatting.cc File base/i18n/number_formatting.cc (right): https://codereview.chromium.org/1989563002/diff/1/base/i18n/number_formatting... base/i18n/number_formatting.cc:89: return FormatPercentFromDouble(static_cast<double>(number) / 100.0, 0); On 2016/05/17 18:17:44, Greg Levin wrote: > I don't totally understand why the NumberFormat objects are being cached, and > separately. Is it okay that I have FormatPercent() and > FormatPercentFromDouble() using the same cached formatter? I'm not sure that > any code will need to access FormatPercentFromDouble() externally, but since I > had to do the int->double conversion anyway, I thought I'd make it available. Well, none of percent format used in our UI has fractional percent (e.g. 35.7%), I don't see a need for FormatPercent helper. You can just use MessageFormatter, can't you (as I suggested and you did in the other CL)? Zoom level: {0,number,percent} will do the job. If you want, you can just add a wrapper over that to ui/base/l10n/l10n_util.cc (to simplify the call site). See GetSingleOrMultipleStringUTF16. https://codereview.chromium.org/1989563002/diff/1/chrome/app/generated_resour... File chrome/app/generated_resources.grd (left): https://codereview.chromium.org/1989563002/diff/1/chrome/app/generated_resour... chrome/app/generated_resources.grd:7183: - Zoom: <ph name="VALUE">$1<ex>100</ex></ph>% On 2016/05/17 18:17:44, Greg Levin wrote: > zoom_decoration.mm also uses IDS_TOOLTIP_ZOOM: > https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/ui/... > > I'm reaching out to the cocoa team to see how they want to deal with changing > the string. I don't think anything special is necessary for Mac in this particular case. zoom_decoration.mm can just call |base::SysUTF16ToNSString| over the result obtained from PercentFormat. https://codereview.chromium.org/1989563002/diff/1/chrome/browser/ui/toolbar/a... File chrome/browser/ui/toolbar/app_menu_model.cc (left): https://codereview.chromium.org/1989563002/diff/1/chrome/browser/ui/toolbar/a... chrome/browser/ui/toolbar/app_menu_model.cc:944: IDS_ZOOM_PERCENT, base::IntToString16(zoom_percent)); Anyway, please drop IDS_ZOOM_PERCENT from a grd file if it's not used anywhere else. https://codereview.chromium.org/1989563002/diff/1/chrome/browser/ui/views/too... File chrome/browser/ui/views/toolbar/app_menu.cc (left): https://codereview.chromium.org/1989563002/diff/1/chrome/browser/ui/views/too... chrome/browser/ui/views/toolbar/app_menu.cc:530: l10n_util::GetStringFUTF16Int(IDS_ZOOM_PERCENT, 100)); Please, drop IDS_ZOOM_PERCENT from a grd file.
On 2016/05/17 23:58:43, jshin (jungshik at google) wrote: > Our current use cases for percent format (we always use integer percent) does > not require adding a helper function specifically for percent format. > > As I wrote below, one can just use |MessageFormatter| with a light-weight > wrapper if you want to. > > BTW, I've just filed an upstream bug to make MessageFormat more flexible with > percent format (again, we don't need that feature at the moment. ) Ooops. forgot to give a link: it's http://bugs.icu-project.org/trac/ticket/12552
https://codereview.chromium.org/1989563002/diff/1/chrome/browser/ui/toolbar/a... File chrome/browser/ui/toolbar/app_menu_model.cc (right): https://codereview.chromium.org/1989563002/diff/1/chrome/browser/ui/toolbar/a... chrome/browser/ui/toolbar/app_menu_model.cc:943: zoom_label_ = base::FormatPercent(zoom_percent); On 2016/05/17 18:17:44, Greg Levin wrote: > I don't know what this change does, or if it's necessary. The string in > app_menu.cc is the one that actually appears in the menu. Does this string > appear anywhere in the UI? Code search says this is the label for IDC_ZOOM_PERCENT_DISPLAY, which is used on Mac (and only there). I didn't look deeper to figure out if this is actually reachable, how Mac really uses it, etc. Given the other comments in this CL, maybe the right first step is to try and make Mac and the other platforms work more similarly to each other, and then we can rip out some of the Mac-specific stuff? https://codereview.chromium.org/1989563002/diff/1/chrome/browser/ui/views/too... File chrome/browser/ui/views/toolbar/app_menu.cc (right): https://codereview.chromium.org/1989563002/diff/1/chrome/browser/ui/views/too... chrome/browser/ui/views/toolbar/app_menu.cc:697: base::FormatPercent(static_cast<int>(zoom * 100 + 0.5)), Use std::round().
On 2016/05/17 23:58:43, jshin (jungshik at google) wrote:
> Well, none of percent format used in our UI has fractional percent (e.g.
35.7%),
> I don't see a need for FormatPercent helper.
>
> You can just use MessageFormatter, can't you (as I suggested and you did in
the
> other CL)?
>
> Zoom level: {0,number,percent}
>
> will do the job.
>
Just to be clear, a msg (say, IDC_ZOOM_LEVEL_PERCENT) in grd would look like
this:
Zoom level: <ph name="PERCENT"><ex>45%</ex>{0,number,percent}</ph>
A call-site would look like:
base::MessageFormatter::FormatWithNumberedArgs(
l10n_util::GetStringUTF16(IDS_ZOOM_LEVEL_PERCENT), integer_percent / 100.0);
> If you want, you can just add a wrapper over that to ui/base/l10n/l10n_util.cc
> (to simplify the call site). See
> GetSingleOrMultipleStringUTF16.
If
base::MessageFormatter::FormatWithNumberedArgs(l10n_util::GetStringUTF16(IDS_FOO_BAR),
integer_percent / 100.0)
is regarded as too verbose (IMHO, it's reasonable), GetStringFWithPercentUTF16()
can be added to l10n_util.
Then, a call-site would be
l10n_util::GetStringFWithPercentUTF16(IDS_FOO_BAR, integer_percent);
glevin@chromium.org changed reviewers: + tapted@chromium.org
+tapted@ for chrome/browser/ui/cocoa/
(Please take a close look at these, since I'm not compiling or testing the .mm
files)
jshin@:
> Just to be clear, a msg (say, IDC_ZOOM_LEVEL_PERCENT) in grd would look like
> this:
>
> Zoom level: <ph name="PERCENT"><ex>45%</ex>{0,number,percent}</ph>
>
> A call-site would look like:
>
> base::MessageFormatter::FormatWithNumberedArgs(
> l10n_util::GetStringUTF16(IDS_ZOOM_LEVEL_PERCENT), integer_percent /
100.0);
Given that I now have FormatPercent(), is this preferable to:
Zoom level: <ph name="PERCENT">$1<ex>45%</ex></ph>
l10n_util::GetStringFUTF16(
IDS_TOOLTIP_ZOOM, base::FormatPercent(zoom_percent));
?
> > If you want, you can just add a wrapper over that to
ui/base/l10n/l10n_util.cc
> > (to simplify the call site). See
> > GetSingleOrMultipleStringUTF16.
>
> If
> base::MessageFormatter::FormatWithNumberedArgs(
> l10n_util::GetStringUTF16(IDS_FOO_BAR), integer_percent / 100.0)
>
> is regarded as too verbose (IMHO, it's reasonable),
GetStringFWithPercentUTF16()
> can be added to l10n_util.
>
> Then, a call-site would be
> l10n_util::GetStringFWithPercentUTF16(IDS_FOO_BAR, integer_percent);
Well, I could add
base::string16 GetStringWithPercentFUTF16(int message_id, int percent) {
return GetStringFUTF16(message_id, base::FormatPercent(percent));
}
but I'm fine with
l10n_util::GetStringFUTF16(
IDS_TOOLTIP_ZOOM, base::FormatPercent(zoom_percent));
Is this approach reasonable?
https://codereview.chromium.org/1989563002/diff/1/base/i18n/number_formatting.cc
File base/i18n/number_formatting.cc (right):
https://codereview.chromium.org/1989563002/diff/1/base/i18n/number_formatting...
base/i18n/number_formatting.cc:89: return
FormatPercentFromDouble(static_cast<double>(number) / 100.0, 0);
> You can just use MessageFormatter, can't you (as I suggested and you did in
> the other CL)?
Sure. This function can just be
return i18n::MessageFormatter::FormatWithNumberedArgs(
ASCIIToUTF16("{0,number,percent}"), static_cast<double>(number) / 100.0);
(I was planning on going back and using FormatPercent() in the battery code.)
Couldn't FormatNumber() do something similar? As I said, I don't know why the
other functions in this file are using this complex caching mechanism. Since
FormatPercent() seems like nearly the same thing, I started by copying them.
Would FormatPercent() make more sense in l10n_util.cc? If I go with the
MessageFormatter version, it's structurally more similar to those functions, but
API-wise, it seems more like FormatNumber() (for instance, it doesn't have a
message_id arg).
> Zoom level: {0,number,percent}
>
> will do the job.
>
> If you want, you can just add a wrapper over that to ui/base/l10n/l10n_util.cc
> (to simplify the call site). See GetSingleOrMultipleStringUTF16.
See reply to subsequent CL comment.
https://codereview.chromium.org/1989563002/diff/1/chrome/app/generated_resour...
File chrome/app/generated_resources.grd (left):
https://codereview.chromium.org/1989563002/diff/1/chrome/app/generated_resour...
chrome/app/generated_resources.grd:7183: - Zoom: <ph
name="VALUE">$1<ex>100</ex></ph>%
On 2016/05/17 23:58:42, jshin (jungshik at google) wrote:
> On 2016/05/17 18:17:44, Greg Levin wrote:
> > zoom_decoration.mm also uses IDS_TOOLTIP_ZOOM:
> >
>
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/ui/...
> >
> > I'm reaching out to the cocoa team to see how they want to deal with
changing
> > the string.
>
> I don't think anything special is necessary for Mac in this particular case.
>
> zoom_decoration.mm can just call |base::SysUTF16ToNSString| over the result
> obtained from PercentFormat.
Done.
https://codereview.chromium.org/1989563002/diff/1/chrome/browser/ui/toolbar/a...
File chrome/browser/ui/toolbar/app_menu_model.cc (left):
https://codereview.chromium.org/1989563002/diff/1/chrome/browser/ui/toolbar/a...
chrome/browser/ui/toolbar/app_menu_model.cc:944: IDS_ZOOM_PERCENT,
base::IntToString16(zoom_percent));
On 2016/05/17 23:58:43, jshin (jungshik at google) wrote:
> Anyway, please drop IDS_ZOOM_PERCENT from a grd file if it's not used
anywhere
> else.
Done.
https://codereview.chromium.org/1989563002/diff/1/chrome/browser/ui/toolbar/a...
File chrome/browser/ui/toolbar/app_menu_model.cc (right):
https://codereview.chromium.org/1989563002/diff/1/chrome/browser/ui/toolbar/a...
chrome/browser/ui/toolbar/app_menu_model.cc:943: zoom_label_ =
base::FormatPercent(zoom_percent);
On 2016/05/18 02:37:25, Peter Kasting wrote:
> On 2016/05/17 18:17:44, Greg Levin wrote:
> > I don't know what this change does, or if it's necessary. The string in
> > app_menu.cc is the one that actually appears in the menu. Does this string
> > appear anywhere in the UI?
>
> Code search says this is the label for IDC_ZOOM_PERCENT_DISPLAY, which is used
> on Mac (and only there). I didn't look deeper to figure out if this is
actually
> reachable, how Mac really uses it, etc.
>
> Given the other comments in this CL, maybe the right first step is to try and
> make Mac and the other platforms work more similarly to each other, and then
we
> can rip out some of the Mac-specific stuff?
Ok, after you pointed the way, it looks like the .mm code is just reaching in
here to get the label for its menu. The other .mm changes I'm making in this CL
are minimal, so I'm not inclined to do any more than necessary with mac code I
don't really understand.
https://codereview.chromium.org/1989563002/diff/1/chrome/browser/ui/views/too...
File chrome/browser/ui/views/toolbar/app_menu.cc (left):
https://codereview.chromium.org/1989563002/diff/1/chrome/browser/ui/views/too...
chrome/browser/ui/views/toolbar/app_menu.cc:530:
l10n_util::GetStringFUTF16Int(IDS_ZOOM_PERCENT, 100));
On 2016/05/17 23:58:43, jshin (jungshik at google) wrote:
> Please, drop IDS_ZOOM_PERCENT from a grd file.
Done.
https://codereview.chromium.org/1989563002/diff/1/chrome/browser/ui/views/too...
File chrome/browser/ui/views/toolbar/app_menu.cc (right):
https://codereview.chromium.org/1989563002/diff/1/chrome/browser/ui/views/too...
chrome/browser/ui/views/toolbar/app_menu.cc:697:
base::FormatPercent(static_cast<int>(zoom * 100 + 0.5)),
On 2016/05/18 02:37:25, Peter Kasting wrote:
> Use std::round().
Done.
https://codereview.chromium.org/1989563002/diff/20001/chrome/browser/ui/views...
File chrome/browser/ui/views/location_bar/zoom_bubble_view.cc (right):
https://codereview.chromium.org/1989563002/diff/20001/chrome/browser/ui/views...
chrome/browser/ui/views/location_bar/zoom_bubble_view.cc:196: this,
l10n_util::GetStringUTF16(IDS_ZOOM_SET_DEFAULT)));
This is just a merge
On 2016/05/18 21:33:45, Greg Levin wrote:
> +tapted@ for chrome/browser/ui/cocoa/
> (Please take a close look at these, since I'm not compiling or testing the .mm
> files)
>
> jshin@:
>
> > Just to be clear, a msg (say, IDC_ZOOM_LEVEL_PERCENT) in grd would look like
> > this:
> >
> > Zoom level: <ph name="PERCENT"><ex>45%</ex>{0,number,percent}</ph>
> >
> > A call-site would look like:
> >
> > base::MessageFormatter::FormatWithNumberedArgs(
> > l10n_util::GetStringUTF16(IDS_ZOOM_LEVEL_PERCENT), integer_percent /
> 100.0);
>
> Given that I now have FormatPercent(), is this preferable to:
>
> Zoom level: <ph name="PERCENT">$1<ex>45%</ex></ph>
>
> l10n_util::GetStringFUTF16(
> IDS_TOOLTIP_ZOOM, base::FormatPercent(zoom_percent));
>
> ?
Either way should be fine. It's up to you. :-)
My preference is the former because I want to promote
a practice of handling each message as a whole (regardless of values being
formatted) instead
of '1. format value to a string 2. insert the result to a msg template'.
As you found out, FormatPercent has an advantage of caching, but all the cases
in your CL
wouldn't be affected much (if at all) even if there's no caching. ICU caches
NumberingSystem (which is
a bit expensive, maybe, to pull off). It also caches the most common
numberformat (Decimal),
but not Percent, Currency or Scientific.
>
> > > If you want, you can just add a wrapper over that to
> ui/base/l10n/l10n_util.cc
> > > (to simplify the call site). See
> > > GetSingleOrMultipleStringUTF16.
> >
> > If
> > base::MessageFormatter::FormatWithNumberedArgs(
> > l10n_util::GetStringUTF16(IDS_FOO_BAR), integer_percent / 100.0)
> >
> > is regarded as too verbose (IMHO, it's reasonable),
> GetStringFWithPercentUTF16()
> > can be added to l10n_util.
> >
> > Then, a call-site would be
> > l10n_util::GetStringFWithPercentUTF16(IDS_FOO_BAR, integer_percent);
>
> Well, I could add
>
> base::string16 GetStringWithPercentFUTF16(int message_id, int percent) {
> return GetStringFUTF16(message_id, base::FormatPercent(percent));
> }
>
> but I'm fine with
>
> l10n_util::GetStringFUTF16(
> IDS_TOOLTIP_ZOOM, base::FormatPercent(zoom_percent));
>
> Is this approach reasonable?
Yup. That's all right except for my preference above. :-) (I'd not add a helper
for that).
Anyway, either way (your approach in the latest CL or mine) is fine.
>
https://codereview.chromium.org/1989563002/diff/1/base/i18n/number_formatting.cc
> File base/i18n/number_formatting.cc (right):
>
>
https://codereview.chromium.org/1989563002/diff/1/base/i18n/number_formatting...
> base/i18n/number_formatting.cc:89: return
> FormatPercentFromDouble(static_cast<double>(number) / 100.0, 0);
> > You can just use MessageFormatter, can't you (as I suggested and you did in
> > the other CL)?
>
> Sure. This function can just be
>
> return i18n::MessageFormatter::FormatWithNumberedArgs(
> ASCIIToUTF16("{0,number,percent}"), static_cast<double>(number) / 100.0);
>
> (I was planning on going back and using FormatPercent() in the battery code.)
> Couldn't FormatNumber() do something similar? As I said, I don't know why the
> other functions in this file are using this complex caching mechanism. Since
As for caching, I think FormatNumber wrapper was written a while ago when ICU
didn't cache. As I wrote above, ICU now does cache NumerFormat with 'Decimal' (
integer or float/double). So, our caching is not actually necessary for them.
Percent formatter instance is not cached by ICU, but our use cases would not
benefit a lot from caching (afaict).
> FormatPercent() seems like nearly the same thing, I started by copying them.
> Would FormatPercent() make more sense in l10n_util.cc? If I go with the
> MessageFormatter version, it's structurally more similar to those functions,
but
> API-wise, it seems more like FormatNumber() (for instance, it doesn't have a
> message_id arg).
I think FormatPercent() had better stay with FormatNumber. It does not need to
deal
with message ids.
> > Zoom level: {0,number,percent}
> >
> > will do the job.
> >
> > If you want, you can just add a wrapper over that to
ui/base/l10n/l10n_util.cc
> > (to simplify the call site). See GetSingleOrMultipleStringUTF16.
>
> See reply to subsequent CL comment.
Thanks a lot for this fix !
LGTM with nits below taken care of (+ an *optional* switch to msg fmt :-)) https://codereview.chromium.org/1989563002/diff/20001/base/i18n/number_format... File base/i18n/number_formatting.cc (right): https://codereview.chromium.org/1989563002/diff/20001/base/i18n/number_format... base/i18n/number_formatting.cc:98: return UTF8ToUTF16(StringPrintf("%d%%", number)); nit: ASCIIToUTF16() https://codereview.chromium.org/1989563002/diff/20001/chrome/app/generated_re... File chrome/app/generated_resources.grd (right): https://codereview.chromium.org/1989563002/diff/20001/chrome/app/generated_re... chrome/app/generated_resources.grd:7208: + Zoom: <ph name="VALUE">$1<ex>100%</ex></ph> nit: Now it has '%' sign. ph name might as well be 'Percentage value' or sth. like that?
The CQ bit was checked by jshin@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1989563002/20001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1989563002/20001
Just for the record. ICU caches NumberFormat (DECIMAL) here: https://code.google.com/p/chromium/codesearch#chromium/src/third_party/icu/so...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: mac_chromium_compile_dbg_ng on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
cocoa lgtm when it compiles :) I patched it in - looks good! - http://imgur.com/SnrJpCq - the "%" in the tooltip should flip around once IDS_TOOLTIP_ZOOM is retranslated and the tooltip gets an RTL paragraph direction. https://codereview.chromium.org/1989563002/diff/20001/chrome/browser/ui/cocoa... File chrome/browser/ui/cocoa/browser/zoom_bubble_controller.mm (right): https://codereview.chromium.org/1989563002/diff/20001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/browser/zoom_bubble_controller.mm:10: #import "chrome/browser/ui/cocoa/info_bubble_view.h" insert `#include "base/strings/sys_string_conversions.h"`
LGTM https://codereview.chromium.org/1989563002/diff/20001/base/i18n/number_format... File base/i18n/number_formatting.cc (right): https://codereview.chromium.org/1989563002/diff/20001/base/i18n/number_format... base/i18n/number_formatting.cc:95: Nit: I'd remove this blank line and instead add one below the conditional. https://codereview.chromium.org/1989563002/diff/20001/base/i18n/number_format... File base/i18n/number_formatting.h (right): https://codereview.chromium.org/1989563002/diff/20001/base/i18n/number_format... base/i18n/number_formatting.h:25: // Return a percent formatted with space and symbol in the user's locale. Nit: percent -> percentage (to distinguish from "a percent sign") https://codereview.chromium.org/1989563002/diff/20001/chrome/browser/ui/views... File chrome/browser/ui/views/toolbar/app_menu.cc (right): https://codereview.chromium.org/1989563002/diff/20001/chrome/browser/ui/views... chrome/browser/ui/views/toolbar/app_menu.cc:710: base::FormatPercent(std::round(zoom * 100)), font_list); Nit: Still should static_cast<int>() the result of std::round() to avoid implicit float truncation
The CQ bit was checked by glevin@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1989563002/40001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1989563002/40001
jshin@ : >> Given that I now have FormatPercent(), is this preferable to: >> >> Zoom level: <ph name="PERCENT">$1<ex>45%</ex></ph> >> >> l10n_util::GetStringFUTF16( >> IDS_TOOLTIP_ZOOM, base::FormatPercent(zoom_percent)); >> >> ? > > Either way should be fine. It's up to you. :-) > My preference is the former because I want to promote > a practice of handling each message as a whole (regardless of values being > formatted) instead > of '1. format value to a string 2. insert the result to a msg template'. That makes sense, thanks! I'll try to stick with that approach in the future. For this CL, I'm going to leave it alone, to prevent more involved changes to zoom_decoration.mm, and so tapted@ doesn't have to test any further changes :-) > As for caching, I think FormatNumber wrapper was written a while ago when > ICU didn't cache. As I wrote above, ICU now does cache NumberFormat with > 'Decimal' (integer or float/double). So, our caching is not actually > necessary for them. > > Percent formatter instance is not cached by ICU, but our use cases would > not benefit a lot from caching (afaict). Thanks for the info! Would it be worthwhile to remove caching and simplify the code in number_formatting.cc (in a separate CL)? https://codereview.chromium.org/1989563002/diff/20001/base/i18n/number_format... File base/i18n/number_formatting.cc (right): https://codereview.chromium.org/1989563002/diff/20001/base/i18n/number_format... base/i18n/number_formatting.cc:95: On 2016/05/19 09:35:52, Peter Kasting wrote: > Nit: I'd remove this blank line and instead add one below the conditional. Function rewritten, no blank lines. https://codereview.chromium.org/1989563002/diff/20001/base/i18n/number_format... base/i18n/number_formatting.cc:98: return UTF8ToUTF16(StringPrintf("%d%%", number)); On 2016/05/18 22:35:03, jshin (jungshik at google) wrote: > nit: ASCIIToUTF16() Function no longer uses cached lazy instance, so no fallback needed, right? I changed the other two instances in the file, since the usage is the same. https://codereview.chromium.org/1989563002/diff/20001/base/i18n/number_format... File base/i18n/number_formatting.h (right): https://codereview.chromium.org/1989563002/diff/20001/base/i18n/number_format... base/i18n/number_formatting.h:25: // Return a percent formatted with space and symbol in the user's locale. On 2016/05/19 09:35:52, Peter Kasting wrote: > Nit: percent -> percentage (to distinguish from "a percent sign") Done. https://codereview.chromium.org/1989563002/diff/20001/chrome/app/generated_re... File chrome/app/generated_resources.grd (right): https://codereview.chromium.org/1989563002/diff/20001/chrome/app/generated_re... chrome/app/generated_resources.grd:7208: + Zoom: <ph name="VALUE">$1<ex>100%</ex></ph> On 2016/05/18 22:35:03, jshin (jungshik at google) wrote: > nit: Now it has '%' sign. ph name might as well be 'Percentage value' or sth. > like that? Done. https://codereview.chromium.org/1989563002/diff/20001/chrome/browser/ui/cocoa... File chrome/browser/ui/cocoa/browser/zoom_bubble_controller.mm (right): https://codereview.chromium.org/1989563002/diff/20001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/browser/zoom_bubble_controller.mm:10: #import "chrome/browser/ui/cocoa/info_bubble_view.h" On 2016/05/19 03:08:42, tapted wrote: > insert `#include "base/strings/sys_string_conversions.h"` Done. Thanks!! https://codereview.chromium.org/1989563002/diff/20001/chrome/browser/ui/views... File chrome/browser/ui/views/toolbar/app_menu.cc (right): https://codereview.chromium.org/1989563002/diff/20001/chrome/browser/ui/views... chrome/browser/ui/views/toolbar/app_menu.cc:710: base::FormatPercent(std::round(zoom * 100)), font_list); On 2016/05/19 09:35:52, Peter Kasting wrote: > Nit: Still should static_cast<int>() the result of std::round() to avoid > implicit float truncation Done.
lgtm https://codereview.chromium.org/1989563002/diff/40001/base/i18n/number_format... File base/i18n/number_formatting.h (right): https://codereview.chromium.org/1989563002/diff/40001/base/i18n/number_format... base/i18n/number_formatting.h:27: // => "12%" in English, "12 %" in Romanian nit: combine lines
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by glevin@chromium.org to run a CQ dry run
jshin@ - Please have a quick look at FormatPercent() unit test https://codereview.chromium.org/1989563002/diff/40001/base/i18n/number_format... File base/i18n/number_formatting.h (right): https://codereview.chromium.org/1989563002/diff/40001/base/i18n/number_format... base/i18n/number_formatting.h:17: // => "1,234,567" in English, "1.234.567" in German ... and here too, I guess? https://codereview.chromium.org/1989563002/diff/40001/base/i18n/number_format... base/i18n/number_formatting.h:27: // => "12%" in English, "12 %" in Romanian On 2016/05/19 15:57:08, stevenjb wrote: > nit: combine lines Done.
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1989563002/60001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1989563002/60001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Oh. I was wondering why this CL was not landed. Apparently, you're waiting for my second LGTM. Sorry about missing that. LGTM ! I'll add to CQ https://codereview.chromium.org/1989563002/diff/60001/base/i18n/number_format... File base/i18n/number_formatting_unittest.cc (right): https://codereview.chromium.org/1989563002/diff/60001/base/i18n/number_format... base/i18n/number_formatting_unittest.cc:101: const wchar_t* expected_german; // Note: Space before % isn't \x20. I didn't know that U+00A0 is used, but it kinda makes sense.
The CQ bit was checked by jshin@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from pkasting@chromium.org, tapted@chromium.org, stevenjb@chromium.org Link to the patchset: https://codereview.chromium.org/1989563002/#ps60001 (title: "Add unit test for FormatPercent()")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1989563002/60001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1989563002/60001
Message was sent while issue was closed.
Committed patchset #4 (id:60001)
Message was sent while issue was closed.
Description was changed from ========== i18n of Zoom % to use locally correct numeric glyphs BUG=475228 TEST=Switch to language with non-Arabic numerals (e.g., Persian), check that Zoom percentages are rendered with non-Arabic numerals. See issue for list of places in UI where Zoom % appears. ========== to ========== i18n of Zoom % to use locally correct numeric glyphs BUG=475228 TEST=Switch to language with non-Arabic numerals (e.g., Persian), check that Zoom percentages are rendered with non-Arabic numerals. See issue for list of places in UI where Zoom % appears. Committed: https://crrev.com/945ea3ae79096e02e4a86dafc90e59a26609db59 Cr-Commit-Position: refs/heads/master@{#395841} ==========
Message was sent while issue was closed.
Patchset 4 (id:??) landed as https://crrev.com/945ea3ae79096e02e4a86dafc90e59a26609db59 Cr-Commit-Position: refs/heads/master@{#395841} |
