Internationalize the prefix of unconfirmed download file.

Internationalize the prefix of unconfirmed download file.

BUG=3563
TEST=manual

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

[tfarina, 2011-01-19 17:17:25]

Hi Randy, could you review this to me?

[tfarina, 2011-01-20 12:42:24]

Pawel, could you review this to me instead?

[Paweł Hajdan Jr., 2011-01-20 12:49:34]

The patch looks generally fine, but the OS_WIN pre-processor logic is
suspicious. I wonder if we have some better way to choose between UTF16 and
UTF8.

Brett, could you take a look?

[Randy Smith (Not in Mondays), 2011-01-20 16:41:13]

Sorry to not get to this yesterday ....

http://codereview.chromium.org/6247015/diff/21001/chrome/app/generated_resour...
File chrome/app/generated_resources.grd (right):

http://codereview.chromium.org/6247015/diff/21001/chrome/app/generated_resour...
chrome/app/generated_resources.grd:2126: Unconfirmed
It looks like you're changing the capitalization for the english case.  Is this
true?  If so, any reason for it?  I'm wary of doing it without a reason, just
based on general conservative instincts.

http://codereview.chromium.org/6247015/diff/21001/chrome/browser/download/dow...
File chrome/browser/download/download_manager.cc (right):

http://codereview.chromium.org/6247015/diff/21001/chrome/browser/download/dow...
chrome/browser/download/download_manager.cc:349: FILE_PATH_LITERAL("
%d.crdownload")).c_str(),
I don't know enough about UTF-8/16 string/wstring interactions to feel
completely comfortable with this--could you get someone who has a good sense of
how wstring would interact with SStringPrintf to take a look at this?  That
might be Brett, and to be fair, if this runs properly on windows that's also a
pretty good sign you've gotten this right.  But don't rely on the tests; make
sure you do an actual dangerous download and look at what the filename looks
like.

[tfarina, 2011-01-20 16:45:29]

On 2011/01/20 16:41:13, rdsmith wrote:
> Sorry to not get to this yesterday ....
> 
>
http://codereview.chromium.org/6247015/diff/21001/chrome/app/generated_resour...
> File chrome/app/generated_resources.grd (right):
> 
>
http://codereview.chromium.org/6247015/diff/21001/chrome/app/generated_resour...
> chrome/app/generated_resources.grd:2126: Unconfirmed
> It looks like you're changing the capitalization for the english case.  Is
this
> true?  If so, any reason for it?  I'm wary of doing it without a reason, just
> based on general conservative instincts.
> 
>
http://codereview.chromium.org/6247015/diff/21001/chrome/browser/download/dow...
> File chrome/browser/download/download_manager.cc (right):
> 
>
http://codereview.chromium.org/6247015/diff/21001/chrome/browser/download/dow...
> chrome/browser/download/download_manager.cc:349: FILE_PATH_LITERAL("
> %d.crdownload")).c_str(),
> I don't know enough about UTF-8/16 string/wstring interactions to feel
> completely comfortable with this--could you get someone who has a good sense
of
> how wstring would interact with SStringPrintf to take a look at this?  That
> might be Brett, and to be fair, if this runs properly on windows that's also a
> pretty good sign you've gotten this right.  But don't rely on the tests; make
> sure you do an actual dangerous download and look at what the filename looks
> like.

Sure, I will try this later tonight. If Brett don't get to this, I think either
Evan Martin, Tony or Avi could review this instead.

[tfarina, 2011-01-20 16:48:18]

http://codereview.chromium.org/6247015/diff/21001/chrome/app/generated_resour...
File chrome/app/generated_resources.grd (right):

http://codereview.chromium.org/6247015/diff/21001/chrome/app/generated_resour...
chrome/app/generated_resources.grd:2126: Unconfirmed
On 2011/01/20 16:41:14, rdsmith wrote:
> It looks like you're changing the capitalization for the english case.  Is
this
> true?
Yes.

>  If so, any reason for it?

I changed it based on the mal's comment:
"While we're at it, the prefix should be capitalized in English 
('Unconfirmed 2345.download')."

[Randy Smith (Not in Mondays), 2011-01-20 17:42:12]

Beyond the below and getting appropriate UTF-8/16/string/wstring eyes on it,
LGTM.

http://codereview.chromium.org/6247015/diff/21001/chrome/app/generated_resour...
File chrome/app/generated_resources.grd (right):

http://codereview.chromium.org/6247015/diff/21001/chrome/app/generated_resour...
chrome/app/generated_resources.grd:2126: Unconfirmed
On 2011/01/20 16:48:18, tfarina wrote:
> On 2011/01/20 16:41:14, rdsmith wrote:
> > It looks like you're changing the capitalization for the english case.  Is
> this
> > true?
> Yes.
> 
> >  If so, any reason for it?
> 
> I changed it based on the mal's comment:
> "While we're at it, the prefix should be capitalized in English 
> ('Unconfirmed 2345.download')."

Ok.  Do a grep in the source for other instances of "unconfirmed" and make sure
they are changed as well.  There are a couple of comments, and two unit test
cases (it'd probably be worthwhile making sure that the unit tests cases
actually ran--I'm not sure why your testing hadn't caught thouse).

[tfarina, 2011-01-20 17:49:01]

On 2011/01/20 17:42:12, rdsmith wrote:
> Beyond the below and getting appropriate UTF-8/16/string/wstring eyes on it,
> LGTM.
> 
>
http://codereview.chromium.org/6247015/diff/21001/chrome/app/generated_resour...
> File chrome/app/generated_resources.grd (right):
> 
>
http://codereview.chromium.org/6247015/diff/21001/chrome/app/generated_resour...
> chrome/app/generated_resources.grd:2126: Unconfirmed
> On 2011/01/20 16:48:18, tfarina wrote:
> > On 2011/01/20 16:41:14, rdsmith wrote:
> > > It looks like you're changing the capitalization for the english case.  Is
> > this
> > > true?
> > Yes.
> > 
> > >  If so, any reason for it?
> > 
> > I changed it based on the mal's comment:
> > "While we're at it, the prefix should be capitalized in English 
> > ('Unconfirmed 2345.download')."
> 
> Ok.  Do a grep in the source for other instances of "unconfirmed" and make
sure
> they are changed as well.
Sure, I'll do.

>  There are a couple of comments, and two unit test
> cases (it'd probably be worthwhile making sure that the unit tests cases
> actually ran--I'm not sure why your testing hadn't caught thouse).

Because we'are not asserting anything on suggested_path.

[Randy Smith (Not in Mondays), 2011-01-20 17:58:55]

On 2011/01/20 17:49:01, tfarina wrote:
> Because we'are not asserting anything on suggested_path.

Huh.  This is more my problem than yours, but it looks to me as if we
are--new_path is initialized from suggested_path, and we've got an EXPECT_CALL
based on new_path.  But I'm not clear on how the EXPECTs work in that context,
so I may be misreading--if you're certain you've run that unit test and it's
passed, I'm happy.

[brettw, 2011-01-20 18:49:22]

LGTM

http://codereview.chromium.org/6247015/diff/21001/chrome/browser/download/dow...
File chrome/browser/download/download_manager.cc (right):

http://codereview.chromium.org/6247015/diff/21001/chrome/browser/download/dow...
chrome/browser/download/download_manager.cc:338: std::wstring unconfirmed_prefix
= UTF16ToWideHack(
You can just use a string16 here and skip the UTF16ToWideHack. On Windows
wstring == string16, and I would like to transfer everything over at some point.

http://codereview.chromium.org/6247015/diff/21001/chrome/browser/download/dow...
chrome/browser/download/download_manager.cc:349: FILE_PATH_LITERAL("
%d.crdownload")).c_str(),
This looks like it would work to me. I tried to come up with a variant doing
with with IntToString[16] and appending rather than using a printf. Normally
this would look better than this printf, but it requires an ifdef windows which
makes it super ugly, so I guess just keep this the way it is.

[tfarina, 2011-01-20 18:55:31]

http://codereview.chromium.org/6247015/diff/21001/chrome/browser/download/dow...
File chrome/browser/download/download_manager.cc (right):

http://codereview.chromium.org/6247015/diff/21001/chrome/browser/download/dow...
chrome/browser/download/download_manager.cc:338: std::wstring unconfirmed_prefix
= UTF16ToWideHack(
On 2011/01/20 18:49:22, brettw wrote:
> You can just use a string16 here and skip the UTF16ToWideHack. On Windows
> wstring == string16, and I would like to transfer everything over at some
point.

Ah, yeah. I've used string16 on patchset 2, but without FILE_PATH_LITERAL (that
is why win bot failed). Thanks, fixed.