Print Preview: Adding support for localized margin units and format.

Print Preview: Adding support for localized margin units and format.

BUG=100416
TEST=The decimal point (dot vs comma) is displayed according to the current locale. Also the units used for the margins (mm vs inches) respect the locale. Try LANGUAGE=el_GR ./out/Release/chrome vs LANGUAGE=en_US ./out/Release/chrome.

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=106846

[dpapad, 2011-10-20 22:01:06]

@estade: please review js.
@thestig: please review c++.
Thank you.

http://codereview.chromium.org/8345025/diff/11001/chrome/browser/ui/webui/pri...
File chrome/browser/ui/webui/print_preview_handler.cc (right):

http://codereview.chromium.org/8345025/diff/11001/chrome/browser/ui/webui/pri...
chrome/browser/ui/webui/print_preview_handler.cc:51: #include
"third_party/icu/public/i18n/unicode/ulocdata.h"
@thestig
Is it ok to have this dependency?

http://codereview.chromium.org/8345025/diff/11001/chrome/browser/ui/webui/pri...
chrome/browser/ui/webui/print_preview_handler.cc:620: if (errorCode >
U_ZERO_ERROR || measurement_system == UMS_LIMIT)
Not sure what system UMS_LIMIT stands for so I am defaulting to the metric
system in that case.

[Lei Zhang, 2011-10-20 23:19:48]

http://codereview.chromium.org/8345025/diff/12008/chrome/browser/ui/webui/pri...
File chrome/browser/ui/webui/print_preview_handler.cc (right):

http://codereview.chromium.org/8345025/diff/12008/chrome/browser/ui/webui/pri...
chrome/browser/ui/webui/print_preview_handler.cc:51: #include
"third_party/icu/public/i18n/unicode/ulocdata.h"
it seems other parts of the code just do: #include "unicode/ulocdata.h"

http://codereview.chromium.org/8345025/diff/12008/chrome/browser/ui/webui/pri...
chrome/browser/ui/webui/print_preview_handler.cc:613: // Getting the measurement
system based on the lcoale.
typo: lcoale => locale

http://codereview.chromium.org/8345025/diff/12008/chrome/browser/ui/webui/pri...
chrome/browser/ui/webui/print_preview_handler.cc:614: const std::string
application_locale(
unused variable?

http://codereview.chromium.org/8345025/diff/12008/chrome/browser/ui/webui/pri...
chrome/browser/ui/webui/print_preview_handler.cc:620: if (errorCode >
U_ZERO_ERROR || measurement_system == UMS_LIMIT)
Change "> U_ZERO_ERROR" to !=

[dpapad, 2011-10-20 23:41:28]

Addressed thestig's comments.

http://codereview.chromium.org/8345025/diff/12008/chrome/browser/ui/webui/pri...
File chrome/browser/ui/webui/print_preview_handler.cc (right):

http://codereview.chromium.org/8345025/diff/12008/chrome/browser/ui/webui/pri...
chrome/browser/ui/webui/print_preview_handler.cc:51: #include
"third_party/icu/public/i18n/unicode/ulocdata.h"
On 2011/10/20 23:19:48, Lei Zhang wrote:
> it seems other parts of the code just do: #include "unicode/ulocdata.h"

Done.

http://codereview.chromium.org/8345025/diff/12008/chrome/browser/ui/webui/pri...
chrome/browser/ui/webui/print_preview_handler.cc:613: // Getting the measurement
system based on the lcoale.
On 2011/10/20 23:19:48, Lei Zhang wrote:
> typo: lcoale => locale

Done.

http://codereview.chromium.org/8345025/diff/12008/chrome/browser/ui/webui/pri...
chrome/browser/ui/webui/print_preview_handler.cc:614: const std::string
application_locale(
On 2011/10/20 23:19:48, Lei Zhang wrote:
> unused variable?

Oops, forgot to remove it, done.

http://codereview.chromium.org/8345025/diff/12008/chrome/browser/ui/webui/pri...
chrome/browser/ui/webui/print_preview_handler.cc:620: if (errorCode >
U_ZERO_ERROR || measurement_system == UMS_LIMIT)
On 2011/10/20 23:19:48, Lei Zhang wrote:
> Change "> U_ZERO_ERROR" to !=

Can't do this. According to the documentation, values less than 0 indicate some
information/warnings but they are not errors. Only values greater than 0
indicate errors.

[Evan Stade, 2011-10-21 01:33:02]

http://codereview.chromium.org/8345025/diff/11004/chrome/browser/resources/pr...
File chrome/browser/resources/print_preview/margin_settings.js (right):

http://codereview.chromium.org/8345025/diff/11004/chrome/browser/resources/pr...
chrome/browser/resources/print_preview/margin_settings.js:362:
print_preview.convertPointsToUserLocaleUnitsAndBack(
nit: s/UserLocale/Locale/

here and elsewhere

http://codereview.chromium.org/8345025/diff/11004/chrome/browser/resources/pr...
File chrome/browser/resources/print_preview/margin_textbox.js (right):

http://codereview.chromium.org/8345025/diff/11004/chrome/browser/resources/pr...
chrome/browser/resources/print_preview/margin_textbox.js:52: this.value =
print_preview.convertPointsToUserLocaleUnitsText(
better to break on a whitespace (i.e. right after the =)

http://codereview.chromium.org/8345025/diff/11004/chrome/browser/resources/pr...
File chrome/browser/resources/print_preview/margin_utils.js (right):

http://codereview.chromium.org/8345025/diff/11004/chrome/browser/resources/pr...
chrome/browser/resources/print_preview/margin_utils.js:42:
print_preview.MarginSettings.thousandsPoint +'\\d{3})*(\\'+
spaces

http://codereview.chromium.org/8345025/diff/11004/chrome/browser/resources/pr...
chrome/browser/resources/print_preview/margin_utils.js:133:
inUserLocaleUnitsText + '"' : inUserLocaleUnitsText + 'mm';
I don't think "mm" is always correct. For example, we localize "kB" (as when
displaying file size).

http://codereview.chromium.org/8345025/diff/11004/chrome/browser/resources/pr...
File chrome/browser/resources/print_preview/print_preview.js (right):

http://codereview.chromium.org/8345025/diff/11004/chrome/browser/resources/pr...
chrome/browser/resources/print_preview/print_preview.js:1060: function
setNumberFormatAndMeasurementSystem(
what is the point of this thunk? just call the function below from C++ (I would
export the function as well so it's just print_preview.setNumberasdfasdfasdf)

http://codereview.chromium.org/8345025/diff/11004/chrome/browser/resources/pr...
chrome/browser/resources/print_preview/print_preview.js:1061: numberFormat,
measurementSystem) {
no wrap

http://codereview.chromium.org/8345025/diff/11004/chrome/browser/resources/pr...
File chrome/browser/resources/print_preview/print_preview_utils.js (right):

http://codereview.chromium.org/8345025/diff/11004/chrome/browser/resources/pr...
chrome/browser/resources/print_preview/print_preview_utils.js:209:
POINTS_PER_MILLIMETER = 2.83464567;
technically should start with var

http://codereview.chromium.org/8345025/diff/11004/chrome/browser/resources/pr...
chrome/browser/resources/print_preview/print_preview_utils.js:234: function
convertMillimetersToPoints(value) {
I would not create these functions. Just multiply by the vars whenever you need
them.

http://codereview.chromium.org/8345025/diff/11004/chrome/browser/resources/pr...
chrome/browser/resources/print_preview/print_preview_utils.js:239: * Converts
|value| from points to centimeters..
not cm

http://codereview.chromium.org/8345025/diff/11004/chrome/browser/ui/webui/pri...
File chrome/browser/ui/webui/print_preview_handler.cc (right):

http://codereview.chromium.org/8345025/diff/11004/chrome/browser/ui/webui/pri...
chrome/browser/ui/webui/print_preview_handler.cc:611: const ListValue* /*args*/)
{
why is args commented out?

http://codereview.chromium.org/8345025/diff/11004/chrome/browser/ui/webui/pri...
chrome/browser/ui/webui/print_preview_handler.cc:615: const char *locale =
g_browser_process->GetApplicationLocale().c_str();
asterisk on left

http://codereview.chromium.org/8345025/diff/11004/chrome/browser/ui/webui/pri...
chrome/browser/ui/webui/print_preview_handler.cc:625: PrintPreviewUI*
print_preview_ui = static_cast<PrintPreviewUI*>(web_ui_);
what is this

[Evan Stade, 2011-10-21 01:33:33]

http://codereview.chromium.org/8345025/diff/11004/chrome/browser/ui/webui/pri...
File chrome/browser/ui/webui/print_preview_handler.cc (right):

http://codereview.chromium.org/8345025/diff/11004/chrome/browser/ui/webui/pri...
chrome/browser/ui/webui/print_preview_handler.cc:611: const ListValue* /*args*/)
{
On 2011/10/21 01:33:02, Evan Stade wrote:
> why is args commented out?

(ignore)

[Lei Zhang, 2011-10-21 01:40:46]

http://codereview.chromium.org/8345025/diff/11004/chrome/browser/ui/webui/pri...
File chrome/browser/ui/webui/print_preview_handler.cc (right):

http://codereview.chromium.org/8345025/diff/11004/chrome/browser/ui/webui/pri...
chrome/browser/ui/webui/print_preview_handler.cc:625: PrintPreviewUI*
print_preview_ui = static_cast<PrintPreviewUI*>(web_ui_);
On 2011/10/21 01:33:02, Evan Stade wrote:
> what is this

Not needed, just call web_ui_->CallJavascriptFunction()

[dpapad, 2011-10-21 16:12:48]

Addressed comments

http://codereview.chromium.org/8345025/diff/11004/chrome/browser/resources/pr...
File chrome/browser/resources/print_preview/margin_settings.js (right):

http://codereview.chromium.org/8345025/diff/11004/chrome/browser/resources/pr...
chrome/browser/resources/print_preview/margin_settings.js:362:
print_preview.convertPointsToUserLocaleUnitsAndBack(
On 2011/10/21 01:33:02, Evan Stade wrote:
> nit: s/UserLocale/Locale/
> 
> here and elsewhere

Done.

http://codereview.chromium.org/8345025/diff/11004/chrome/browser/resources/pr...
File chrome/browser/resources/print_preview/margin_textbox.js (right):

http://codereview.chromium.org/8345025/diff/11004/chrome/browser/resources/pr...
chrome/browser/resources/print_preview/margin_textbox.js:52: this.value =
print_preview.convertPointsToUserLocaleUnitsText(
On 2011/10/21 01:33:02, Evan Stade wrote:
> better to break on a whitespace (i.e. right after the =)

Done.

http://codereview.chromium.org/8345025/diff/11004/chrome/browser/resources/pr...
File chrome/browser/resources/print_preview/margin_utils.js (right):

http://codereview.chromium.org/8345025/diff/11004/chrome/browser/resources/pr...
chrome/browser/resources/print_preview/margin_utils.js:42:
print_preview.MarginSettings.thousandsPoint +'\\d{3})*(\\'+
On 2011/10/21 01:33:02, Evan Stade wrote:
> spaces

Done.

http://codereview.chromium.org/8345025/diff/11004/chrome/browser/resources/pr...
chrome/browser/resources/print_preview/margin_utils.js:133:
inUserLocaleUnitsText + '"' : inUserLocaleUnitsText + 'mm';
On 2011/10/21 01:33:02, Evan Stade wrote:
> I don't think "mm" is always correct. For example, we localize "kB" (as when
> displaying file size).

I am planning to merge these changes to M16, so I can't add strings. On the
other hand cm, mm, kg etc are sort of standard symbols in all languages that use
the SI system and will not be a problem in my opinion, until localization
happens.

http://codereview.chromium.org/8345025/diff/11004/chrome/browser/resources/pr...
File chrome/browser/resources/print_preview/print_preview.js (right):

http://codereview.chromium.org/8345025/diff/11004/chrome/browser/resources/pr...
chrome/browser/resources/print_preview/print_preview.js:1060: function
setNumberFormatAndMeasurementSystem(
On 2011/10/21 01:33:02, Evan Stade wrote:
> what is the point of this thunk? just call the function below from C++ (I
would
> export the function as well so it's just print_preview.setNumberasdfasdfasdf)

Done.

http://codereview.chromium.org/8345025/diff/11004/chrome/browser/resources/pr...
File chrome/browser/resources/print_preview/print_preview_utils.js (right):

http://codereview.chromium.org/8345025/diff/11004/chrome/browser/resources/pr...
chrome/browser/resources/print_preview/print_preview_utils.js:209:
POINTS_PER_MILLIMETER = 2.83464567;
On 2011/10/21 01:33:02, Evan Stade wrote:
> technically should start with var

Done.

http://codereview.chromium.org/8345025/diff/11004/chrome/browser/resources/pr...
chrome/browser/resources/print_preview/print_preview_utils.js:234: function
convertMillimetersToPoints(value) {
On 2011/10/21 01:33:02, Evan Stade wrote:
> I would not create these functions. Just multiply by the vars whenever you
need
> them.

I prefer to keep them (along with convertInchesToPoints and vice versa). This
way the conversions can be tested in print_preview_utils_test.html and it also
improves readability.

http://codereview.chromium.org/8345025/diff/11004/chrome/browser/resources/pr...
chrome/browser/resources/print_preview/print_preview_utils.js:239: * Converts
|value| from points to centimeters..
On 2011/10/21 01:33:02, Evan Stade wrote:
> not cm

Done.

http://codereview.chromium.org/8345025/diff/11004/chrome/browser/ui/webui/pri...
File chrome/browser/ui/webui/print_preview_handler.cc (right):

http://codereview.chromium.org/8345025/diff/11004/chrome/browser/ui/webui/pri...
chrome/browser/ui/webui/print_preview_handler.cc:615: const char *locale =
g_browser_process->GetApplicationLocale().c_str();
On 2011/10/21 01:33:02, Evan Stade wrote:
> asterisk on left

Done.

http://codereview.chromium.org/8345025/diff/11004/chrome/browser/ui/webui/pri...
chrome/browser/ui/webui/print_preview_handler.cc:625: PrintPreviewUI*
print_preview_ui = static_cast<PrintPreviewUI*>(web_ui_);
On 2011/10/21 01:40:46, Lei Zhang wrote:
> On 2011/10/21 01:33:02, Evan Stade wrote:
> > what is this
> 
> Not needed, just call web_ui_->CallJavascriptFunction()

Done. Initially I was planning to add a new method in print_preview_ui.cc and
call it from here, then this cast would be needed.

[Evan Stade, 2011-10-22 00:30:45]

lgtm with comment

http://codereview.chromium.org/8345025/diff/9010/chrome/browser/resources/pri...
File chrome/browser/resources/print_preview/print_preview_utils_test.html
(right):

http://codereview.chromium.org/8345025/diff/9010/chrome/browser/resources/pri...
chrome/browser/resources/print_preview/print_preview_utils_test.html:121:
assertEquals(convertMillimetersToPoints(10), 28.3464567);
I don't think these tests are super useful, you are basically just asserting
that a constant somewhere has a particular value. If for some mystifying reason
you ever did want to change that constant, then you'd have to change these tests
as well, so all you've done is added work. These tests would make sense if they
were testing something more complicated than the multiplication operator.

[Lei Zhang, 2011-10-22 00:42:38]

LGTM on the C++ side.

[dpapad, 2011-10-22 00:47:29]

http://codereview.chromium.org/8345025/diff/9010/chrome/browser/resources/pri...
File chrome/browser/resources/print_preview/print_preview_utils_test.html
(right):

http://codereview.chromium.org/8345025/diff/9010/chrome/browser/resources/pri...
chrome/browser/resources/print_preview/print_preview_utils_test.html:121:
assertEquals(convertMillimetersToPoints(10), 28.3464567);
On 2011/10/22 00:30:45, Evan Stade wrote:
> I don't think these tests are super useful, you are basically just asserting
> that a constant somewhere has a particular value. If for some mystifying
reason
> you ever did want to change that constant, then you'd have to change these
tests
> as well, so all you've done is added work. These tests would make sense if
they
> were testing something more complicated than the multiplication operator.

I agree. Basically these tests only cover the case where on of those 2 constants
is changed accidentally. I can remove if this is not enough value to justify
adding them.

[Evan Stade, 2011-10-22 00:51:21]

On Fri, Oct 21, 2011 at 5:47 PM,  <dpapad@chromium.org> wrote:
>
>
http://codereview.chromium.org/8345025/diff/9010/chrome/browser/resources/pri...
> File
> chrome/browser/resources/print_preview/print_preview_utils_test.html
> (right):
>
>
http://codereview.chromium.org/8345025/diff/9010/chrome/browser/resources/pri...
> chrome/browser/resources/print_preview/print_preview_utils_test.html:121:
> assertEquals(convertMillimetersToPoints(10), 28.3464567);
> On 2011/10/22 00:30:45, Evan Stade wrote:
>>
>> I don't think these tests are super useful, you are basically just
>
> asserting
>>
>> that a constant somewhere has a particular value. If for some
>
> mystifying reason
>>
>> you ever did want to change that constant, then you'd have to change
>
> these tests
>>
>> as well, so all you've done is added work. These tests would make
>
> sense if they
>>
>> were testing something more complicated than the multiplication
>
> operator.
>
> I agree. Basically these tests only cover the case where on of those 2
> constants is changed accidentally. I can remove if this is not enough
> value to justify adding them.

yea... we have lots of constants throughout chrome and we don't
generally write runtime tests to make sure they aren't changed by typo

>
> http://codereview.chromium.org/8345025/
>

[commit-bot: I haz the power, 2011-10-22 01:38:14]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/dpapad@chromium.org/8345025/20001

[commit-bot: I haz the power, 2011-10-22 02:53:41]

Change committed as 106846