Print Preview: Fixing parsing of numberFormat for several locales.

Print Preview: Fixing parsing of numberFormat for several locales.

BUG=104858
TEST=NONE


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

[dpapad, 2011-12-12 23:58:06]

I am planning to add some tests for the regular expression used before landing
this CL.

http://codereview.chromium.org/8922021/diff/4001/chrome/browser/resources/pri...
File chrome/browser/resources/print_preview/margin_settings.js (right):

http://codereview.chromium.org/8922021/diff/4001/chrome/browser/resources/pri...
chrome/browser/resources/print_preview/margin_settings.js:179: var matches =
numberFormat.match(regex) || ['','','.','',','];
The fix has 2 parts.

1) Changing the regex to allow any kind of symbom for thousands point and
decimal point.
2) In the unexpected case that parsing fails, defaulting to '.' for thousands
point and ',' for decimal point.

[Lei Zhang, 2011-12-13 00:16:02]

http://codereview.chromium.org/8922021/diff/4001/chrome/browser/resources/pri...
File chrome/browser/resources/print_preview/margin_settings.js (right):

http://codereview.chromium.org/8922021/diff/4001/chrome/browser/resources/pri...
chrome/browser/resources/print_preview/margin_settings.js:179: var matches =
numberFormat.match(regex) || ['','','.','',','];
On 2011/12/12 23:58:06, dpapad wrote:
> The fix has 2 parts.
> 
> 1) Changing the regex to allow any kind of symbom for thousands point and
> decimal point.
> 2) In the unexpected case that parsing fails, defaulting to '.' for thousands
> point and ',' for decimal point. 

Isn't this reversed? ',' for thousands and '.' for decimal.

[dpapad, 2011-12-13 01:00:15]

Added unit tests and made the regex more general.

[dpapad1, 2011-12-13 01:01:19]

http://codereview.chromium.org/8922021/diff/4001/chrome/browser/resources/pri...
File chrome/browser/resources/print_preview/margin_settings.js (right):

http://codereview.chromium.org/8922021/diff/4001/chrome/browser/resources/pri...
chrome/browser/resources/print_preview/margin_settings.js:179: var matches =
numberFormat.match(regex) || ['','','.','',','];
On 2011/12/13 00:16:02, Lei Zhang wrote:
> On 2011/12/12 23:58:06, dpapad wrote:
> > The fix has 2 parts.
> > 
> > 1) Changing the regex to allow any kind of symbom for thousands point and
> > decimal point.
> > 2) In the unexpected case that parsing fails, defaulting to '.' for
thousands
> > point and ',' for decimal point. 
> 
> Isn't this reversed? ',' for thousands and '.' for decimal.

Well, I am not sure what the fallback should be. In some locales  (e.g Greek)
"." is for thousands and "," for decimal. I guess the fallback should be the
most commonly used option? Ideally we should be able to test in all locales and
make sure that the fallback is never needed.

[Lei Zhang, 2011-12-13 01:06:20]

On 2011/12/13 01:01:19, dpapad1 wrote:
>
http://codereview.chromium.org/8922021/diff/4001/chrome/browser/resources/pri...
> File chrome/browser/resources/print_preview/margin_settings.js (right):
> 
>
http://codereview.chromium.org/8922021/diff/4001/chrome/browser/resources/pri...
> chrome/browser/resources/print_preview/margin_settings.js:179: var matches =
> numberFormat.match(regex) || ['','','.','',','];
> On 2011/12/13 00:16:02, Lei Zhang wrote:
> > On 2011/12/12 23:58:06, dpapad wrote:
> > > The fix has 2 parts.
> > > 
> > > 1) Changing the regex to allow any kind of symbom for thousands point and
> > > decimal point.
> > > 2) In the unexpected case that parsing fails, defaulting to '.' for
> thousands
> > > point and ',' for decimal point. 
> > 
> > Isn't this reversed? ',' for thousands and '.' for decimal.
> 
> Well, I am not sure what the fallback should be. In some locales  (e.g Greek)
> "." is for thousands and "," for decimal. I guess the fallback should be the
> most commonly used option? Ideally we should be able to test in all locales
and
> make sure that the fallback is never needed.

It's the other way around in English.

[dpapad1, 2011-12-13 01:11:30]

http://codereview.chromium.org/8922021/diff/4001/chrome/browser/resources/pri...
File chrome/browser/resources/print_preview/margin_settings.js (right):

http://codereview.chromium.org/8922021/diff/4001/chrome/browser/resources/pri...
chrome/browser/resources/print_preview/margin_settings.js:179: var matches =
numberFormat.match(regex) || ['','','.','',','];
On 2011/12/13 01:01:19, dpapad1 wrote:
> On 2011/12/13 00:16:02, Lei Zhang wrote:
> > On 2011/12/12 23:58:06, dpapad wrote:
> > > The fix has 2 parts.
> > > 
> > > 1) Changing the regex to allow any kind of symbom for thousands point and
> > > decimal point.
> > > 2) In the unexpected case that parsing fails, defaulting to '.' for
> thousands
> > > point and ',' for decimal point. 
> > 
> > Isn't this reversed? ',' for thousands and '.' for decimal.
> 
> Well, I am not sure what the fallback should be. In some locales  (e.g Greek)
> "." is for thousands and "," for decimal. I guess the fallback should be the
> most commonly used option? Ideally we should be able to test in all locales
and
> make sure that the fallback is never needed.

Done.

[Lei Zhang, 2011-12-13 01:15:44]

lgtm

[commit-bot: I haz the power, 2011-12-13 01:17:14]

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

[commit-bot: I haz the power, 2011-12-13 01:19:13]

Try job failure for 8922021-4007 on win_rel for steps "update_scripts, update".
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...

Step "update" is always a major failure.
Look at the try server FAQ for more details.

[commit-bot: I haz the power, 2011-12-13 01:22:24]

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

[commit-bot: I haz the power, 2011-12-13 01:40:18]

I fixed the slave, it was dead.

Le 12 décembre 2011 20:22, <commit-bot@chromium.org> a écrit :

> CQ is trying da patch. Follow status at
> https://chromium-status.**appspot.com/cq/dpapad@**
>
chromium.org/8922021/4007<https://chromium-status.appspot.com/cq/dpapad@chromium.org/8922021/4007>
>
>
>
http://codereview.chromium.**org/8922021/<http://codereview.chromium.org/8922...
>

[commit-bot: I haz the power, 2011-12-13 03:05:22]

Change committed as 114167