sync i18n support for gaia_login.html and merge_and_sync.html...

sync i18n support for gaia_login.html and merge_and_sync.htmlMove resources in generated_resources.grd.Move the google logo depending of the order of words in the translation of "Google Account"Add support for rtl locales
BUG=19896
TEST=Make sure the layout of gaia_login.html and merge_and_sync.html is correct in ltr and rtl

[tim (not reviewing), 2009-09-15 13:28:00]

Hey Bruno,

Thanks for the patch!  You need to sync up as I landed the patch from the review
I cc'ed you on last week, I moved some of the JS assignments of DOM values into
the actual HTML to use i18n-content.  I didn't touch mirroring or the GRD,
though.

[tim (not reviewing), 2009-09-15 20:52:47]

Your patch is looking quite good.

http://codereview.chromium.org/195082/diff/2001/3001
File chrome/app/generated_resources.grd (right):

http://codereview.chromium.org/195082/diff/2001/3001#newcode316
Line 316: Success!
I think I agree with you on 'Abort', but I feel like success and all done should
be qualified with IDS_SYNC_SETUP_.

http://codereview.chromium.org/195082/diff/2001/3001#newcode4760
Line 4760: <message name="IDS_SYNC_CANNOTBE_BLANK" desc="The input error message
when the user hasn't entered a username or password">
nit CANNOT_BE

http://codereview.chromium.org/195082/diff/2001/3001#newcode4763
Line 4763: <message name="IDS_SYNC_LOGIN_COULDNOT_CONNECT" desc="The connect
error message">
nit COULD_NOT

http://codereview.chromium.org/195082/diff/2001/3001#newcode4774
Line 4774: <message name="IDS_SYNC_MERGE_DIALOG_HEADER" desc="The header at the
top of the merge and sync dialog.">
lets call this IDS_SYNC_MERGE_INTRODUCTION to be consistent with the login case

http://codereview.chromium.org/195082/diff/2001/3001#newcode4777
Line 4777: <message name="IDS_SYNC_MERGE_DIALOG_MERGE_SYNC" desc="The label of
the button to actually merge your profile">
IDS_SYNC_MERGE_AND_SYNC_LABEL or IDS_SYNC_MERGE_MERGE_AND_SYNC_LABEL if we're
looking for autocomplete-friendly consistency :)

http://codereview.chromium.org/195082/diff/2001/3001#newcode4780
Line 4780: <message name="IDS_SYNC_MERGE_DIALOG_FOOTER" desc="The footer at the
bottom of the merge and sync dialog.">
we've historically called this blurb the 'warning', so I'd prefer
IDS_SYNC_MERGE_WARNING

http://codereview.chromium.org/195082/diff/2001/3005
File chrome/browser/sync/resources/gaia_login.html (right):

http://codereview.chromium.org/195082/diff/2001/3005#newcode107
Line 107: var ltr = templateData['textdirection'] == 'ltr';
Does 'document.dir' work instead of templateData['textdirection']?

http://codereview.chromium.org/195082/diff/2001/3005#newcode109
Line 109: if (logoIsAtEndOfSentence == ltr) {
What cases other than textdirection=='rtl' do we want to move the logo to the
right of Account? (or are there rtl cases where you want to avoid doing so)?

http://codereview.chromium.org/195082/diff/2001/3005#newcode110
Line 110: // Move the google logo after the word 'Account'
nit - period

http://codereview.chromium.org/195082/diff/2001/3002
File chrome/browser/views/toolbar_view.cc (right):

http://codereview.chromium.org/195082/diff/2001/3002#newcode1079
Line 1079: IDS_SYNC_LOGIN_DIALOG_TITLE) + L"...");
these are similar the same at the moment but they don't have to be, I would
prefer leaving MENU_LABEL.  Or renaming LOGIN_DIALOG_TITLE to
SYNC_MY_BOOKMARKS_LABEL, but that doesn't sound as permanent.

[tim (not reviewing), 2009-09-15 21:58:56]

http://codereview.chromium.org/195082/diff/2001/3005
File chrome/browser/sync/resources/gaia_login.html (right):

http://codereview.chromium.org/195082/diff/2001/3005#newcode109
Line 109: if (logoIsAtEndOfSentence == ltr) {
On 2009/09/15 20:52:47, timsteele wrote:
> What cases other than textdirection=='rtl' do we want to move the logo to the
> right of Account? (or are there rtl cases where you want to avoid doing so)?

pff. shame on me, turns out French is one of them, I understand now.  I hear
indexOf isn't the proper way to handle bidi text, but aside from having separate
HTML files for different languages I'm not sure how to do anything better than
what you have here, so I think it is okay for now.

[jungshik at Google, 2009-09-15 23:20:31]

http://codereview.chromium.org/195082/diff/2001/3005
File chrome/browser/sync/resources/gaia_login.html (right):

http://codereview.chromium.org/195082/diff/2001/3005#newcode17
Line 17: }
I'm afraid this is not necessary with html getting 'dir: rtl'

http://codereview.chromium.org/195082/diff/2001/3005#newcode43
Line 43: style="margin-bottom: 0" onload="initForm();">
I think 'body' needs the following as well:

i18n-values=".style.fontFamily:fontfamily;.style.fontSize:fontsize"

http://codereview.chromium.org/195082/diff/2001/3004
File chrome/browser/sync/resources/merge_and_sync.html (right):

http://codereview.chromium.org/195082/diff/2001/3004#newcode8
Line 8: dir: rtl;
Do we need this with html getting 'dir='rtl'?

[tim (not reviewing), 2009-09-21 05:58:28]

Any word on the CLA stuff yet?  This LGTM, so pending Jungshik's sign-off, as
soon as the CLA is taken care of I will commit this on your behalf!

http://codereview.chromium.org/195082/diff/4003/6005
File chrome/app/generated_resources.grd (right):

http://codereview.chromium.org/195082/diff/4003/6005#newcode4725
Line 4725: <message name="SYNC_MY_BOOKMARKS_LABEL" desc="Text to show for the
sync login dialog.">
It does need the IDS_ prefix though :)

http://codereview.chromium.org/195082/diff/4003/6006
File chrome/browser/views/toolbar_view.cc (right):

http://codereview.chromium.org/195082/diff/4003/6006#newcode1071
Line 1071: SYNC_MY_BOOKMARKS_LABEL) + L"...");
nit- please put the "l10n_util::..." indented 4 spaces from the start of 1069
("app_menu_..."), and then bring the rest of this function call up to the same
line.

[tim (not reviewing), 2009-09-27 20:27:53]

On 2009/09/21 05:58:28, timsteele wrote:
> Any word on the CLA stuff yet?  This LGTM, so pending Jungshik's sign-off, as
> soon as the CLA is taken care of I will commit this on your behalf!
> 
> http://codereview.chromium.org/195082/diff/4003/6005
> File chrome/app/generated_resources.grd (right):
> 
> http://codereview.chromium.org/195082/diff/4003/6005#newcode4725
> Line 4725: <message name="SYNC_MY_BOOKMARKS_LABEL" desc="Text to show for the
> sync login dialog.">
> It does need the IDS_ prefix though :)
> 
> http://codereview.chromium.org/195082/diff/4003/6006
> File chrome/browser/views/toolbar_view.cc (right):
> 
> http://codereview.chromium.org/195082/diff/4003/6006#newcode1071
> Line 1071: SYNC_MY_BOOKMARKS_LABEL) + L"...");
> nit- please put the "l10n_util::..." indented 4 spaces from the start of 1069
> ("app_menu_..."), and then bring the rest of this function call up to the same
> line.

I think we're good-to-go now, Bruno, could you add AUTHORS to this change with
your name/email? Also, do you have a new patch to upload?

Jungshik, any final comments?

[Bruno, 2009-09-28 17:37:52]

On 2009/09/27 20:27:53, timsteele wrote:
> On 2009/09/21 05:58:28, timsteele wrote:
> > Any word on the CLA stuff yet?  This LGTM, so pending Jungshik's sign-off,
as
> > soon as the CLA is taken care of I will commit this on your behalf!
> > 
> > http://codereview.chromium.org/195082/diff/4003/6005
> > File chrome/app/generated_resources.grd (right):
> > 
> > http://codereview.chromium.org/195082/diff/4003/6005#newcode4725
> > Line 4725: <message name="SYNC_MY_BOOKMARKS_LABEL" desc="Text to show for
the
> > sync login dialog.">
> > It does need the IDS_ prefix though :)
> > 
> > http://codereview.chromium.org/195082/diff/4003/6006
> > File chrome/browser/views/toolbar_view.cc (right):
> > 
> > http://codereview.chromium.org/195082/diff/4003/6006#newcode1071
> > Line 1071: SYNC_MY_BOOKMARKS_LABEL) + L"...");
> > nit- please put the "l10n_util::..." indented 4 spaces from the start of
1069
> > ("app_menu_..."), and then bring the rest of this function call up to the
same
> > line.
> 
> I think we're good-to-go now, Bruno, could you add AUTHORS to this change with
> your name/email? Also, do you have a new patch to upload?
> 
> Jungshik, any final comments?

Patch uploaded, ready to go

[jungshik at Google, 2009-09-28 18:38:55]

LGTM !

Thanks again and sorry for the delay.

[tim (not reviewing), 2009-09-28 22:44:23]

On 2009/09/28 18:38:55, Jungshik Shin wrote:
> LGTM !
> 
> Thanks again and sorry for the delay.

Landed in http://src.chromium.org/viewvc/chrome?view=rev&revision=27426.
Congrats on the first patch!