There seem to be three copies of some strings in .grd files |
||||||||||||
Issue description
E.g. IDS_INFOBAR_MISSING_LOCATION_PERMISSION_TEXT is in three files. Two of these are identical ("Chrome needs") and the other is the same but with chromium instead of chrome.
See https://cs.chromium.org/search/?q=IDS_INFOBAR_MISSING_LOCATION_PERMISSION_TEXT&type=cs for all three.
I can think of two improvements:
1. Remove one of the two identical strings. If there is a need sometimes to have different strings for different products, have one of the files take priority and only put in duplicate definitions when the strings are different
2. Remove the chromium specific one and have a placeholder for the product name.
Maybe both of these things already exist (I thought 2 did) but this string and others like it are just being used improperly?
Not sure who looks after this stuff so adding two TPMs who might know...
,
May 10 2017
dimu@, can you PTAL?
,
May 10 2017
Ugh, thanks a lot Monorail outage. Sorry for comment spam. dimu@, can you PTAL?
,
May 10 2017
I not very familiar about how the .grd are being used by different products. Re-assign to tedchoc@ who added these IDs. Would you please take a look? Thanks!
,
May 10 2017
If I recall (and I know very little of the capabilities of grd, so I'm unassigning this from myself as I'm not going to work towards any resolution), we use this string in both java and C++ code. Again to the best of my recollection, formatter_data="android_java" is the normal way of sharing strings across java and C++, but at the time chromium_strings.grd and google_chrome_strings.grd did not support it and would not be able to generate the java one we needed. I would love to know if the product name replacement works, but I haven't seen it anywhere either. You need to find someone familiar with strings at this point, and I don't know who that is (maybe agable@?)
,
May 10 2017
Nope, not me :( I only know things about the process of copying strings into prod, and copying translations out. I know nothing else.
,
May 11 2017
+oshima who i've heard knows about grd files
,
May 11 2017
And +tapted who knows about everything
,
May 11 2017
+thakis since he's in tools/grit/OWNERS. I suspect many of these questions are more about the tools than the grd format. I fear in two more steps we will have all chromium engineers on this bug though :-P
,
May 11 2017
chrome/app/chromium_strings.grd and chrome/app/google_chrome_strings.grd are supposed to have identical strings except for product name. Using placeholders tends to not work well in non-english languages due to inflections and what have you.
I don't know what chrome/android/java/strings/android_chrome_strings.grd is for. If comment 5 is still true, we should probably find a way to make chrome/app/{chromium,google_chrome}_strings.grd usable from java.
,
May 16 2017
Ted - is there any point in having chromium versions of the android only strings? Currently it seems like the java version will be chrome not chromium, so if it is useful we'll get a weird mix.
,
May 16 2017
@#11, I don't know the historical reasoning between the split on desktop. I certainly have no intention of adding that complexity to the Android strings if we can avoid it, which we have for the last 6ish years. I'm actually surprised placeholders aren't ok for the product name as I thought we intentionally do not translate it. Granted, I don't know if there are occurrence of longer Product-y names like "Google Chrome" where you could imagine some language that would convert to "Chrome by Google".
,
May 26 2017
I just had a bit of a lookie and see a few options to improve things:
1. set up the formatter_data=android_java stuff for the branded strings (chrome/app/{chromium,google_chrome}_strings.grid).
This doesn't look too hard but it might get fiddly. If we do this there will just be two versions of these strings - one for Chrome and one for Chromium. This might not be needed for Android strings where we don't ship a chromium, but it means that branded strings where the same string is used on desktop and also in java code for Android would be supported.
2. just move the branded Android only strings that are needed in java into generated_resources.grd.
This would be super simple to do. It would not support branded strings used in java and on destkop, and would introduce branded strings into generated_resources which is maybe bad (there seem to be quite a few there already but that maybe should be fixed).
3. Make the branded strings be .grdp files conditionally included into generated_resources.grd. I think this would have the same effect as 1. but would probably be a lot simpler.
I'm going to have an attempt at 3 as it seems like the proper way to do things and will make things simpler. If it is a stupid idea for some reason please let me know!
,
May 26 2017
Feel free to re-CC me but I'm not sure any of the active discussion needs my blessing. I do appreciate you reviewing this though, thanks!
,
Jun 29 2017
Another duplication just appeared (in https://codereview.chromium.org/2912133002): chrome/app/generated_resources.grd: IDS_INTENT_PICKER_BUBBLE_VIEW_INCOGNITO_* chrome/android/java/strings/android_chrome_strings.grd: IDS_EXTERNAL_APP_LEAVE_INCOGNITO_* No apparent way around it right now but if a solution comes up, those should be part of it.
,
Jul 7 2017
,
Jul 10 2017
+digit
From an email by digit@:
I'm currently working on localized strings support for Chrome on Android. Yes, the topic can be really confusing :)
Looking into your issue, it looks like the string you're trying to use in generated_resources.grd is inside an <if expr="chromeos"> element, as such it is never placed into the generated R.java when grit is invoked with the "-t android" option. In other words, while these strings appear in two different files, they are not duplicated in the final binaries.
I've just tried this locally and it worked. On the other hand, I don't see any use of IDS_INTENT_PICKER_BUBBLE_VIEW_INCOGNITO_WARNING et al in the source tree, and I wonder if these definitions are obsolete now? If so you might want to remove them from generated_resources.grd instead? If not, the ID name doesn't make much sense for Android and might probably be changed too (e.g. IDS_NAVIGATION_INCOGNITO_WARNING).
…
Is there an existing mechanism for accessing strings defined in generated_resources.grd from Java?
The mechanism seems to be:
add formatter_data="android_java" to every <message> element. Better place these elements inside an <if expr="is_android"> element if they are specific to Android (and of course ensure they are not filtered out, like in your case).
If you add a new .grd file, ensure that there is a java_strings_grd("my_new_grd_strings") rule in your BUILD.gn to properly process it with grit and generate the corresponding .xml files.
Add the corresponding target (e.g. my_new_grd_strings) is listed in the deps section of the android_resources("chrome_java_resources") definition in chrome/app/BUILD.gn to ensure that said resources will be part of the final org/chromium/chrome/R.java file.
For the record, the R.java source file is located inside of $OUT/gen/chrome/android/chrome_java_resources.srcjar, in case you need to take a look at it to check everything :)
,
Jul 10 2017
digit: thanks for all the info, it sounds like you know what you're talking about :) The case I've been looking at is for the branded resource files, which don't have the android_java stuff set up yet. Could you take a look at #13 and let me know what you think? I had another look today at this as I should have time this week to spend on it. I think taking option 1) will result in lots of stuff being added to the BUILD file for the various languages that need to be supported.
,
Jul 10 2017
Ignore #15, it turns out there's a simple solution spotted by digit@ (I was trying to use an <if expr="chromeos"> string from Android).
,
Jul 14 2017
digit - ping re #18. Could you provide some advice? After looking into it a bit I've decided this really needs whoever the expert(s) in resources is / are to fix.
,
Jul 19 2017
I just took a quick look at the .grd files:
- chrome/android/java/strings/android_chrome_strings.grd contains strings
that are exclusively used by Java code from chrome/android/java/.
There are many messages defined here that contain Chrome or Google, but
the vast majority of them do not.
- chrome/android/webapk/strings/android_webapk_strings.grd is exactly in
the same situation, btw.
I agree that option 1. seems the best, i.e.: get rid of these two files by moving the non-branded messages to generated_resources.grd, and the branded ones to ${BRAND}_strings.grd files (if they don't already exist there).
For the latter, this means duplicating the messages by replacing Chrome with Chromium, even though we don't plan to release Chromium builds for Android ourselves (but someone else may want to do just that, and shouldn't have to suffer from our historical inconsistencies).
This should not require any line of Java source files, since all strings are exported by the same auto-generated org.chromium.chrome.R class in the end.
Of course, this assumes that adding formatter_data="android_java" as appropriate works as expected. I'm really curious why this would not be the case.
Keep in mind that you will also have to move the java_strings_grd() calls from chrome/android/BUILD.gn to chrome/app/BUILD.gn in order to ensure that the Android .xml string resource files are generated properly for the branded messages.
Option 2. would be definitely bad and confusing for everyone :-/
Option 3. might work and actually simplify the BUILD.gn files, but I'm not sure how to properly set the proper conditions for GRIT to include either the google_chrome or chromium .grdp files.
Let me know if this isn't very clear :)
,
Jan 21
(2 days ago)
|
||||||||||||
►
Sign in to add a comment |
||||||||||||
Comment 1 by amineer@chromium.org
, May 10 2017