Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2948653002:
Allow localization of WebApk name. (Closed)
DescriptionAllow localization of WebApk name.
We want to support WebAPK with localized names. In this CL:
1. Removes "name" and "short_name" metadata from shell APK's
AndroidManifest.xml.
Instead, these two fields in WebApkInfo and the application label will
obtained from resources (string).
2. Adds non translateable strings.
3. Ensures WebApkInfo is compatible with existing WebAPKs that don't have these
two strings.
BUG=734212
Review-Url: https://codereview.chromium.org/2948653002
Cr-Commit-Position: refs/heads/master@{#482274}
Committed: https://chromium.googlesource.com/chromium/src/+/82c6369965606f7ce2098aac7c442ca7e1f1f28e
Patch Set 1 #
Total comments: 16
Patch Set 2 : pkotwicz@'s comments. #
Total comments: 13
Patch Set 3 : dominickn@'s comments. #Patch Set 4 : Fix the tests failing. #
Total comments: 2
Patch Set 5 : Fix another tests. #Messages
Total messages: 42 (29 generated)
Patchset #1 (id:1) has been deleted
Patchset #1 (id:20001) has been deleted
The CQ bit was checked by hanxi@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...) mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
Patchset #2 (id:60001) has been deleted
Patchset #1 (id:40001) has been deleted
hanxi@chromium.org changed reviewers: + pkotwicz@chromium.org
Hi Peter, could you please take a look? Thanks!
The CQ bit was checked by hanxi@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Just nits! https://codereview.chromium.org/2948653002/diff/80001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/webapps/WebApkInfo.java (right): https://codereview.chromium.org/2948653002/diff/80001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/webapps/WebApkInfo.java:103: } If res == null, we should just return null for WebApkInfo https://codereview.chromium.org/2948653002/diff/80001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/webapps/WebApkInfo.java:107: shortNameId = res.getIdentifier(webApkPackageName + STRING_SHORT_NAME, null, null); Can you pass the package name as the last parameter instead? This way STRING_NAME and STRING_SHORT_NAME are the real name https://codereview.chromium.org/2948653002/diff/80001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/webapps/WebApkInfo.java:134: Bitmap primaryIcon = decodeImageResource(webApkPackageName, primaryIconId); Can we pass |res| to decodeImageResource() ? https://codereview.chromium.org/2948653002/diff/80001/chrome/android/junit/sr... File chrome/android/junit/src/org/chromium/chrome/browser/webapps/WebApkInfoTest.java (right): https://codereview.chromium.org/2948653002/diff/80001/chrome/android/junit/sr... chrome/android/junit/src/org/chromium/chrome/browser/webapps/WebApkInfoTest.java:61: super(); Is this super call necessary? https://codereview.chromium.org/2948653002/diff/80001/chrome/android/junit/sr... chrome/android/junit/src/org/chromium/chrome/browser/webapps/WebApkInfoTest.java:96: if (name == null) throw new NullPointerException(); Nit: You don't need to check for null here since |mStringIdMap| supports null keys https://codereview.chromium.org/2948653002/diff/80001/chrome/android/junit/sr... chrome/android/junit/src/org/chromium/chrome/browser/webapps/WebApkInfoTest.java:110: public void addStringForTesting(String string, int identifier, String value) { Nit: 'string' -> 'identifierName' https://codereview.chromium.org/2948653002/diff/80001/chrome/android/junit/sr... chrome/android/junit/src/org/chromium/chrome/browser/webapps/WebApkInfoTest.java:329: String name = "@string/name"; Shouldn't |name| and |shortName| have the value of the name and shortName? https://codereview.chromium.org/2948653002/diff/80001/chrome/android/webapk/s... File chrome/android/webapk/strings/android_webapk_strings.grd (right): https://codereview.chromium.org/2948653002/diff/80001/chrome/android/webapk/s... chrome/android/webapk/strings/android_webapk_strings.grd:115: <message name="IDS_NAME" desc="The application name of the WebAPK." translateable="false"> Nit: Remove the word "application" Technically, the "application name" is the short_name
Patchset #2 (id:100001) has been deleted
Hi Peter, PTAL, thanks! https://codereview.chromium.org/2948653002/diff/80001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/webapps/WebApkInfo.java (right): https://codereview.chromium.org/2948653002/diff/80001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/webapps/WebApkInfo.java:103: } On 2017/06/20 19:39:48, pkotwicz wrote: > If res == null, we should just return null for WebApkInfo Done. https://codereview.chromium.org/2948653002/diff/80001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/webapps/WebApkInfo.java:107: shortNameId = res.getIdentifier(webApkPackageName + STRING_SHORT_NAME, null, null); On 2017/06/20 19:39:48, pkotwicz wrote: > Can you pass the package name as the last parameter instead? This way > STRING_NAME and STRING_SHORT_NAME are the real name Done. https://codereview.chromium.org/2948653002/diff/80001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/webapps/WebApkInfo.java:134: Bitmap primaryIcon = decodeImageResource(webApkPackageName, primaryIconId); On 2017/06/20 19:39:48, pkotwicz wrote: > Can we pass |res| to decodeImageResource() ? Good catch! https://codereview.chromium.org/2948653002/diff/80001/chrome/android/junit/sr... File chrome/android/junit/src/org/chromium/chrome/browser/webapps/WebApkInfoTest.java (right): https://codereview.chromium.org/2948653002/diff/80001/chrome/android/junit/sr... chrome/android/junit/src/org/chromium/chrome/browser/webapps/WebApkInfoTest.java:61: super(); On 2017/06/20 19:39:48, pkotwicz wrote: > Is this super call necessary? Removed. https://codereview.chromium.org/2948653002/diff/80001/chrome/android/junit/sr... chrome/android/junit/src/org/chromium/chrome/browser/webapps/WebApkInfoTest.java:96: if (name == null) throw new NullPointerException(); On 2017/06/20 19:39:48, pkotwicz wrote: > Nit: You don't need to check for null here since |mStringIdMap| supports null > keys Done. https://codereview.chromium.org/2948653002/diff/80001/chrome/android/junit/sr... chrome/android/junit/src/org/chromium/chrome/browser/webapps/WebApkInfoTest.java:110: public void addStringForTesting(String string, int identifier, String value) { On 2017/06/20 19:39:48, pkotwicz wrote: > Nit: 'string' -> 'identifierName' Done. https://codereview.chromium.org/2948653002/diff/80001/chrome/android/junit/sr... chrome/android/junit/src/org/chromium/chrome/browser/webapps/WebApkInfoTest.java:329: String name = "@string/name"; On 2017/06/20 19:39:48, pkotwicz wrote: > Shouldn't |name| and |shortName| have the value of the name and shortName? Yes, I will use some other values. https://codereview.chromium.org/2948653002/diff/80001/chrome/android/webapk/s... File chrome/android/webapk/strings/android_webapk_strings.grd (right): https://codereview.chromium.org/2948653002/diff/80001/chrome/android/webapk/s... chrome/android/webapk/strings/android_webapk_strings.grd:115: <message name="IDS_NAME" desc="The application name of the WebAPK." translateable="false"> On 2017/06/20 19:39:49, pkotwicz wrote: > Nit: Remove the word "application" > > Technically, the "application name" is the short_name Done.
Description was changed from ========== Allow localization of WebApk name. We want to support WebAPK with localized names. In this CL: 1. Removes "name" and "short_name" metadata from shell APK's AndroidManifest.xml. Instead, these two fields in WebApkInfo and the application label will obtained from resources (string). 2. Adds non translateable strings. 3. Make WebApkInfo is compatible with existing WebAPKs that don't have these two strings. BUG=734212 ========== to ========== Allow localization of WebApk name. We want to support WebAPK with localized names. In this CL: 1. Removes "name" and "short_name" metadata from shell APK's AndroidManifest.xml. Instead, these two fields in WebApkInfo and the application label will obtained from resources (string). 2. Adds non translateable strings. 3. Ensures WebApkInfo is compatible with existing WebAPKs that don't have these two strings. BUG=734212 ==========
LGTM!
hanxi@chromium.org changed reviewers: + dominickn@chromium.org
Hi Dom, could you please take a look? Thanks!
Looks pretty good. https://codereview.chromium.org/2948653002/diff/120001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/webapps/WebApkInfo.java (right): https://codereview.chromium.org/2948653002/diff/120001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/webapps/WebApkInfo.java:32: public static final String STRING_NAME = "name"; Nit: STRING_NAME is a bit ambiguous. Can you call this RESOURCE_NAME, RESOURCE_SHORT_NAME, and RESOURCE_STRING_TYPE to emphasise that they're associated with fetching resources? https://codereview.chromium.org/2948653002/diff/120001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/webapps/WebApkInfo.java:102: return null; I'm not convinced about this change in behaviour - pkotwicz, I'm curious about your reasons for bailing out entirely rather than maintaining the old behaviour? https://codereview.chromium.org/2948653002/diff/120001/chrome/android/junit/s... File chrome/android/junit/src/org/chromium/chrome/browser/webapps/WebApkInfoTest.java (right): https://codereview.chromium.org/2948653002/diff/120001/chrome/android/junit/s... chrome/android/junit/src/org/chromium/chrome/browser/webapps/WebApkInfoTest.java:61: mResourceMap = new HashMap<String, Resources>(); Nit: you should be able to do = new HashMap<>(); https://codereview.chromium.org/2948653002/diff/120001/chrome/android/junit/s... chrome/android/junit/src/org/chromium/chrome/browser/webapps/WebApkInfoTest.java:115: private String getkey(String name, String defType, String defPackage) { Nit: getKey() https://codereview.chromium.org/2948653002/diff/120001/chrome/android/junit/s... chrome/android/junit/src/org/chromium/chrome/browser/webapps/WebApkInfoTest.java:334: Can you also test the fallback behaviour: that if there's nothing in resources, the data in the bundle is returned?
The CQ bit was checked by hanxi@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Hi Dom, PTAL, thanks! https://codereview.chromium.org/2948653002/diff/120001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/webapps/WebApkInfo.java (right): https://codereview.chromium.org/2948653002/diff/120001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/webapps/WebApkInfo.java:32: public static final String STRING_NAME = "name"; On 2017/06/22 03:26:01, dominickn wrote: > Nit: STRING_NAME is a bit ambiguous. Can you call this RESOURCE_NAME, > RESOURCE_SHORT_NAME, and RESOURCE_STRING_TYPE to emphasise that they're > associated with fetching resources? Done. https://codereview.chromium.org/2948653002/diff/120001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/webapps/WebApkInfo.java:102: return null; On 2017/06/22 03:26:01, dominickn wrote: > I'm not convinced about this change in behaviour - pkotwicz, I'm curious about > your reasons for bailing out entirely rather than maintaining the old behaviour? WebAPKs have "drawables" in resources since version 1. If the resources is null, something must be wrong. I feel it is ok to return null here, but need to update WebApkInfoTest to make sure the packageManager has resource for that WebAPK (as I did in this patch). https://codereview.chromium.org/2948653002/diff/120001/chrome/android/junit/s... File chrome/android/junit/src/org/chromium/chrome/browser/webapps/WebApkInfoTest.java (right): https://codereview.chromium.org/2948653002/diff/120001/chrome/android/junit/s... chrome/android/junit/src/org/chromium/chrome/browser/webapps/WebApkInfoTest.java:61: mResourceMap = new HashMap<String, Resources>(); On 2017/06/22 03:26:01, dominickn wrote: > Nit: you should be able to do = new HashMap<>(); Done. https://codereview.chromium.org/2948653002/diff/120001/chrome/android/junit/s... chrome/android/junit/src/org/chromium/chrome/browser/webapps/WebApkInfoTest.java:115: private String getkey(String name, String defType, String defPackage) { On 2017/06/22 03:26:01, dominickn wrote: > Nit: getKey() Done. https://codereview.chromium.org/2948653002/diff/120001/chrome/android/junit/s... chrome/android/junit/src/org/chromium/chrome/browser/webapps/WebApkInfoTest.java:334: On 2017/06/22 03:26:01, dominickn wrote: > Can you also test the fallback behaviour: that if there's nothing in resources, > the data in the bundle is returned? I have thought about this, and the |testSanity| would work just as your suggestion. In the setUp(), you will see each tests in this test suite has used the FackPackageManager with an empty resource. This isn't intentional, but currently in WebApkInfo, we returns null when there isn't any resource found for the WebAPK. This behaviour will cause existing tests failed if not setting resource in the packageManager.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
Patchset #4 (id:160001) has been deleted
Just one nit! https://codereview.chromium.org/2948653002/diff/180001/chrome/android/webapk/... File chrome/android/webapk/test/src/org/chromium/webapk/test/WebApkTestHelper.java (right): https://codereview.chromium.org/2948653002/diff/180001/chrome/android/webapk/... chrome/android/webapk/test/src/org/chromium/webapk/test/WebApkTestHelper.java:25: public static class FakeResources extends Resources { Nit: I would keep FakeResources in WebApkInfoTest because the implementation of FakeResources is specific to that test. I think that you use Mockito.mock() to create the Resource object for line 96
I'll lg once you've finished moving things around to pass the bot. https://codereview.chromium.org/2948653002/diff/120001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/webapps/WebApkInfo.java (right): https://codereview.chromium.org/2948653002/diff/120001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/webapps/WebApkInfo.java:102: return null; On 2017/06/22 14:17:35, Xi Han wrote: > On 2017/06/22 03:26:01, dominickn wrote: > > I'm not convinced about this change in behaviour - pkotwicz, I'm curious about > > your reasons for bailing out entirely rather than maintaining the old > behaviour? > > WebAPKs have "drawables" in resources since version 1. If the resources is null, > something must be wrong. I feel it is ok to return null here, but need to update > WebApkInfoTest to make sure the packageManager has resource for that WebAPK (as > I did in this patch). Acknowledged. https://codereview.chromium.org/2948653002/diff/120001/chrome/android/junit/s... File chrome/android/junit/src/org/chromium/chrome/browser/webapps/WebApkInfoTest.java (right): https://codereview.chromium.org/2948653002/diff/120001/chrome/android/junit/s... chrome/android/junit/src/org/chromium/chrome/browser/webapps/WebApkInfoTest.java:334: On 2017/06/22 14:17:35, Xi Han wrote: > On 2017/06/22 03:26:01, dominickn wrote: > > Can you also test the fallback behaviour: that if there's nothing in > resources, > > the data in the bundle is returned? > > I have thought about this, and the |testSanity| would work just as your > suggestion. In the setUp(), you will see each tests in this test suite has used > the FackPackageManager with an empty resource. This isn't intentional, but > currently in WebApkInfo, we returns null when there isn't any resource found for > the WebAPK. This behaviour will cause existing tests failed if not setting > resource in the packageManager. Acknowledged, but that does seem to be more of a side effect of the setup rather than an explicit test.
Hi Dom, PTAL, thanks! https://codereview.chromium.org/2948653002/diff/120001/chrome/android/junit/s... File chrome/android/junit/src/org/chromium/chrome/browser/webapps/WebApkInfoTest.java (right): https://codereview.chromium.org/2948653002/diff/120001/chrome/android/junit/s... chrome/android/junit/src/org/chromium/chrome/browser/webapps/WebApkInfoTest.java:334: On 2017/06/23 03:09:19, dominickn wrote: > On 2017/06/22 14:17:35, Xi Han wrote: > > On 2017/06/22 03:26:01, dominickn wrote: > > > Can you also test the fallback behaviour: that if there's nothing in > > resources, > > > the data in the bundle is returned? > > > > I have thought about this, and the |testSanity| would work just as your > > suggestion. In the setUp(), you will see each tests in this test suite has > used > > the FackPackageManager with an empty resource. This isn't intentional, but > > currently in WebApkInfo, we returns null when there isn't any resource found > for > > the WebAPK. This behaviour will cause existing tests failed if not setting > > resource in the packageManager. > > Acknowledged, but that does seem to be more of a side effect of the setup rather > than an explicit test. Ok, added a test for this case:) https://codereview.chromium.org/2948653002/diff/180001/chrome/android/webapk/... File chrome/android/webapk/test/src/org/chromium/webapk/test/WebApkTestHelper.java (right): https://codereview.chromium.org/2948653002/diff/180001/chrome/android/webapk/... chrome/android/webapk/test/src/org/chromium/webapk/test/WebApkTestHelper.java:25: public static class FakeResources extends Resources { On 2017/06/22 20:47:33, pkotwicz wrote: > Nit: I would keep FakeResources in WebApkInfoTest because the implementation of > FakeResources is specific to that test. > > I think that you use Mockito.mock() to create the Resource object for line 96 Sounds good. Thanks for the suggestion!
The CQ bit was checked by hanxi@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
lgtm, thanks
The CQ bit was checked by hanxi@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from pkotwicz@chromium.org Link to the patchset: https://codereview.chromium.org/2948653002/#ps200001 (title: "Fix another tests.")
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 200001, "attempt_start_ts": 1498484636467760,
"parent_rev": "860dd9ed86a08dbdd234f0d89f6e53a079a683b3", "commit_rev":
"82c6369965606f7ce2098aac7c442ca7e1f1f28e"}
Message was sent while issue was closed.
Description was changed from ========== Allow localization of WebApk name. We want to support WebAPK with localized names. In this CL: 1. Removes "name" and "short_name" metadata from shell APK's AndroidManifest.xml. Instead, these two fields in WebApkInfo and the application label will obtained from resources (string). 2. Adds non translateable strings. 3. Ensures WebApkInfo is compatible with existing WebAPKs that don't have these two strings. BUG=734212 ========== to ========== Allow localization of WebApk name. We want to support WebAPK with localized names. In this CL: 1. Removes "name" and "short_name" metadata from shell APK's AndroidManifest.xml. Instead, these two fields in WebApkInfo and the application label will obtained from resources (string). 2. Adds non translateable strings. 3. Ensures WebApkInfo is compatible with existing WebAPKs that don't have these two strings. BUG=734212 Review-Url: https://codereview.chromium.org/2948653002 Cr-Commit-Position: refs/heads/master@{#482274} Committed: https://chromium.googlesource.com/chromium/src/+/82c6369965606f7ce2098aac7c44... ==========
Message was sent while issue was closed.
Committed patchset #5 (id:200001) as https://chromium.googlesource.com/chromium/src/+/82c6369965606f7ce2098aac7c44... |
