Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2094903003:
Pass all intent extras needed to render splash screen when launching WebAPK (Closed)
DescriptionPass all intent extras needed to render splash screen when launching WebAPK
This CL adds to the WebAPK's Android Manifest attributes for rendering the
splash screen. These attributes are passed to WebappActivity.
The "string value" for the display and orientation attributes is used in the
WebAPK's Android Manifest instead of the WebDisplayMode and
ScreenOrientationValues enum values. This is done to enable WebAPKs to work with
non-Chrome browsers in the future.
BUG=623216
TEST=WebappInfoTest
R=hanxi, dominickn
TBR=dfalcantara
Committed: https://crrev.com/f8678dfec2891e16e637db2459125edd32fe3c91
Cr-Commit-Position: refs/heads/master@{#402965}
Patch Set 1 #Patch Set 2 : Merge branch 'master' into webapk_more_meta #
Total comments: 13
Patch Set 3 : Merge branch 'master' into webapk_more_meta #Patch Set 4 : Merge branch 'master' into webapk_more_meta #
Total comments: 12
Patch Set 5 : Merge branch 'master' into webapk_more_meta #Patch Set 6 : Merge branch 'master' into webapk_more_meta #Patch Set 7 : Merge branch 'master' into webapk_more_meta #
Messages
Total messages: 32 (9 generated)
Description was changed from ========== Pass all intent extras needed to render splash screen when launching WebAPK This CL adds to the WebAPK's Android Manifest attributes for rendering the splash screen. These attributes are passed by WebappActivity. The "string value" for the display and orientation attributes is used in the WebAPK's Android Manifest instead of the WebDisplayMode and ScreenOrientationValues enum values. This is done to enable WebAPKs to work with non-Chrome browsers in the future. BUG=623216 TEST=WebappInfoTest ========== to ========== Pass all intent extras needed to render splash screen when launching WebAPK This CL adds to the WebAPK's Android Manifest attributes for rendering the splash screen. These attributes are passed to WebappActivity. The "string value" for the display and orientation attributes is used in the WebAPK's Android Manifest instead of the WebDisplayMode and ScreenOrientationValues enum values. This is done to enable WebAPKs to work with non-Chrome browsers in the future. BUG=623216 TEST=WebappInfoTest ==========
pkotwicz@chromium.org changed reviewers: + hanxi@chromium.org
Xi can you please take a look?
https://codereview.chromium.org/2094903003/diff/20001/chrome/android/webapk/s... File chrome/android/webapk/shell_apk/src/org/chromium/webapk/shell_apk/MainActivity.java (right): https://codereview.chromium.org/2094903003/diff/20001/chrome/android/webapk/s... chrome/android/webapk/shell_apk/src/org/chromium/webapk/shell_apk/MainActivity.java:83: boolean isIconGenerated = TextUtils.isEmpty(bundle.getString(META_DATA_ICON_URL)); What is the value of the icon_url? I thought it was the url for the WebAPK icon, or it was only non-null when the icon was a Favicon? https://codereview.chromium.org/2094903003/diff/20001/chrome/android/webapk/s... chrome/android/webapk/shell_apk/src/org/chromium/webapk/shell_apk/MainActivity.java:87: Log.w(TAG, "Package name of the WebAPK:" + packageName); Since you are working on this class, could you please add more log, and change Log.w() to Log.v() or Log.d(). Thanks!
https://codereview.chromium.org/2094903003/diff/20001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappInfo.java (right): https://codereview.chromium.org/2094903003/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappInfo.java:61: String displayModeString = displayModeString -> displayMode. https://codereview.chromium.org/2094903003/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappInfo.java:62: IntentUtils.safeGetStringExtra(intent, WebApkConstants.EXTRA_DISPLAY_MODE); I don't like putting WebAPKs' specific logic here, but I understand why you have to introduce new constants for these two fields. https://codereview.chromium.org/2094903003/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappInfo.java:82: String orientationString = orientationString to orientation. https://codereview.chromium.org/2094903003/diff/20001/chrome/android/webapk/s... File chrome/android/webapk/shell_apk/src/org/chromium/webapk/shell_apk/MainActivity.java (right): https://codereview.chromium.org/2094903003/diff/20001/chrome/android/webapk/s... chrome/android/webapk/shell_apk/src/org/chromium/webapk/shell_apk/MainActivity.java:101: .putExtra(WebApkConstants.EXTRA_DISPLAY_MODE, displayMode) Maybe put a note here to remind that a conversion needs to be done when parsing extras of the displayMode and the orientation from the launch intent of a WebAPK.
Xi, can you please take another look? https://codereview.chromium.org/2094903003/diff/20001/chrome/android/webapk/s... File chrome/android/webapk/shell_apk/src/org/chromium/webapk/shell_apk/MainActivity.java (right): https://codereview.chromium.org/2094903003/diff/20001/chrome/android/webapk/s... chrome/android/webapk/shell_apk/src/org/chromium/webapk/shell_apk/MainActivity.java:83: boolean isIconGenerated = TextUtils.isEmpty(bundle.getString(META_DATA_ICON_URL)); It is the URL of the WebbAPK's app icon - the one which appears in the app list. It is non-empty unless the icon is generated (rounded rectangle with first letter of WebAPK title) https://codereview.chromium.org/2094903003/diff/20001/chrome/android/webapk/s... chrome/android/webapk/shell_apk/src/org/chromium/webapk/shell_apk/MainActivity.java:87: Log.w(TAG, "Package name of the WebAPK:" + packageName); Changed to Log.v() Which additional properties do you want logged? I do not think that we should log all of the properties that we pass via the intent to Chrome. We should only log the properties that we often use for debugging (If I want to log something for debugging I can temporarily add the log myself) https://codereview.chromium.org/2094903003/diff/20001/chrome/android/webapk/s... chrome/android/webapk/shell_apk/src/org/chromium/webapk/shell_apk/MainActivity.java:101: .putExtra(WebApkConstants.EXTRA_DISPLAY_MODE, displayMode) I think the additional conversion is an implementation detail of Chrome.
lgtm with nits. https://codereview.chromium.org/2094903003/diff/20001/chrome/android/webapk/s... File chrome/android/webapk/shell_apk/src/org/chromium/webapk/shell_apk/MainActivity.java (right): https://codereview.chromium.org/2094903003/diff/20001/chrome/android/webapk/s... chrome/android/webapk/shell_apk/src/org/chromium/webapk/shell_apk/MainActivity.java:83: boolean isIconGenerated = TextUtils.isEmpty(bundle.getString(META_DATA_ICON_URL)); On 2016/06/27 19:55:37, pkotwicz wrote: > It is the URL of the WebbAPK's app icon - the one which appears in the app list. > It is non-empty unless the icon is generated (rounded rectangle with first > letter of WebAPK title) I see, thanks for the explanation. https://codereview.chromium.org/2094903003/diff/20001/chrome/android/webapk/s... chrome/android/webapk/shell_apk/src/org/chromium/webapk/shell_apk/MainActivity.java:87: Log.w(TAG, "Package name of the WebAPK:" + packageName); On 2016/06/27 19:55:37, pkotwicz wrote: > Changed to Log.v() Which additional properties do you want logged? I do not > think that we should log all of the properties that we pass via the intent to > Chrome. We should only log the properties that we often use for debugging (If I > want to log something for debugging I can temporarily add the log myself) Fair enough. https://codereview.chromium.org/2094903003/diff/20001/chrome/android/webapk/s... chrome/android/webapk/shell_apk/src/org/chromium/webapk/shell_apk/MainActivity.java:101: .putExtra(WebApkConstants.EXTRA_DISPLAY_MODE, displayMode) On 2016/06/27 19:55:37, pkotwicz wrote: > I think the additional conversion is an implementation detail of Chrome. We'd better mention that WebAPK doesn't use the same constant ShortcutHelper.EXTRA_DISPLAY_MODE as what webapp does.An conversion is handled within WebapInfo.
pkotwicz@chromium.org changed reviewers: + dfalcantara@chromium.org
dfalcantara@ can you please take a look? https://codereview.chromium.org/2094903003/diff/20001/chrome/android/webapk/s... File chrome/android/webapk/shell_apk/src/org/chromium/webapk/shell_apk/MainActivity.java (right): https://codereview.chromium.org/2094903003/diff/20001/chrome/android/webapk/s... chrome/android/webapk/shell_apk/src/org/chromium/webapk/shell_apk/MainActivity.java:101: .putExtra(WebApkConstants.EXTRA_DISPLAY_MODE, displayMode) I added a comment. Please let me know if it can be improved.
dfalcantara@chromium.org changed reviewers: + dominickn@chromium.org
Swapping out Dominick for me.
Can you explain more about why these string constants need to be in the Android manifest at this point? The linked bug isn't quite clear (e.g. are the "awesome splashscreens" referenced different to the existing splash screen used for standalone web apps? As far as I'm aware, all of the data for the existing splash screens is held within WebappDataStorage, which webapks are reusing).
The reason for having the string constants in the Android manifest is so that when a user (1) clears Chrome's cache (2) launches a WebAPK that we can use the values from the AndroidManifest instead of showing a blank splash screen. WebAPKs are different from non-WebAPK web apps because WebAPKs still launch in WebAPK mode after the user clears Chrome's cached data. Whenever the data for the splash screen is available from WebappDataStorage WebAPKs will use the data from WebappDataStorage. WebAPKs currently get most of their splash screen data from their launch intent. hanxi@ is working on making them get most of their launch data from WebappDataStorage (https://codereview.chromium.org/2071213005/) Yes the "awesome splashscreen" that I am referring to in the bug is the current splash screen implementation.
On 2016/06/28 01:11:45, pkotwicz wrote: > The reason for having the string constants in the Android manifest is so that > when a user > (1) clears Chrome's cache > (2) launches a WebAPK > that we can use the values from the AndroidManifest instead of showing a blank > splash screen. > > WebAPKs are different from non-WebAPK web apps because WebAPKs still launch in > WebAPK mode after the user clears Chrome's cached data. Whenever the data for > the splash screen is available from WebappDataStorage WebAPKs will use the data > from WebappDataStorage. > > WebAPKs currently get most of their splash screen data from their launch intent. > hanxi@ is working on making them get most of their launch data from > WebappDataStorage (https://codereview.chromium.org/2071213005/) > > Yes the "awesome splashscreen" that I am referring to in the bug is the current > splash screen implementation. This makes sense. However, *all* WebappDataStorages are currently wiped if the user clears site data (called through from BrowsingDataRemover -> WebappRegistry::UnregisterWebapps). I think that means the splash screen image and all the other WebAPK related data disappears anyway when the user clears. Since you need to do it anyway, would it be cleaner to simply persist WebAPK-related WebappDataStorages when the user clears site data? It would involve skipping WebAPKs in WebappRegistry.unregisterAllWebapps. That way, you won't need the extra string constants in the manifest, right, since you'll still have the WebappDataStorage? Or have I missed something here? :) Before you skip WebAPKs in the clean up step, you'll need to verify with the privacy team (if you haven't already done so) that it is okay to keep WebAPK data if the user has wiped their browsing history. I assume that if we know the WebAPK is still installed, it should be okay.
Sorry for the confusion. I meant when a user clears Chrome's data via Settings->Apps->Chrome->Storage->Clear Data
On 2016/06/28 03:05:07, pkotwicz wrote: > Sorry for the confusion. I meant when a user clears Chrome's data via > Settings->Apps->Chrome->Storage->Clear Data OK, that makes more sense. But how will the splashscreen image otherwise be persisted and accessed if the WebappDataStorage disappears when Chrome's data is cleared? (Sorry for all the questions, just making sure I understand how the whole pipeline will work).
If Chrome's data is cleared we will use the app icon instead. The splash screen code in WebappActivity already has a way to use the app icon (provided that the app icon is big enough) if the splash screen icon cannot be fetched from WebappDataStorage. In the future we may decide to bundle the splash screen icon with the WebAPK too.
Thanks for all the explanations! This mostly looks fine. https://codereview.chromium.org/2094903003/diff/60001/chrome/android/javatest... File chrome/android/javatests/src/org/chromium/chrome/browser/webapps/WebappInfoTest.java (right): https://codereview.chromium.org/2094903003/diff/60001/chrome/android/javatest... chrome/android/javatests/src/org/chromium/chrome/browser/webapps/WebappInfoTest.java:274: } Can you please add a testIntentDisplayModeAndOrientation, since parsing these out of the intent now has two possible paths? Also, you should test in the testIntentWebApk* tests that enum values in the intent are overridden by the existence of the string. https://codereview.chromium.org/2094903003/diff/60001/chrome/android/webapk/l... File chrome/android/webapk/libs/common/src/org/chromium/webapk/lib/common/WebApkConstants.java (right): https://codereview.chromium.org/2094903003/diff/60001/chrome/android/webapk/l... chrome/android/webapk/libs/common/src/org/chromium/webapk/lib/common/WebApkConstants.java:17: public static final String EXTRA_DISPLAY_MODE = "org.chromium.webapk.lib.common.display_mode"; Can we replace the EXTRA_ prefix? I can see potential confusion because EXTRA_ collides with the constant in ShortcutHelper, even though they store different (but related) types of data. Maybe WEBAPK_? https://codereview.chromium.org/2094903003/diff/60001/chrome/android/webapk/s... File chrome/android/webapk/shell_apk/BUILD.gn (right): https://codereview.chromium.org/2094903003/diff/60001/chrome/android/webapk/s... chrome/android/webapk/shell_apk/BUILD.gn:21: webapk_orientation = "landscape" Nit: perhaps use "portrait" instead (more common case?) https://codereview.chromium.org/2094903003/diff/60001/chrome/android/webapk/s... File chrome/android/webapk/shell_apk/src/org/chromium/webapk/shell_apk/MainActivity.java (right): https://codereview.chromium.org/2094903003/diff/60001/chrome/android/webapk/s... chrome/android/webapk/shell_apk/src/org/chromium/webapk/shell_apk/MainActivity.java:30: private static final String EXTRA_ID = "org.chromium.chrome.browser.webapp_id"; Nit: is it possible to just use ShortcutHelper here directly? If it isn't, we should definitely look at putting these constants in a single location accessible from both.
dominickn@ can you please take another look? https://codereview.chromium.org/2094903003/diff/60001/chrome/android/javatest... File chrome/android/javatests/src/org/chromium/chrome/browser/webapps/WebappInfoTest.java (right): https://codereview.chromium.org/2094903003/diff/60001/chrome/android/javatest... chrome/android/javatests/src/org/chromium/chrome/browser/webapps/WebappInfoTest.java:274: } I have added tests for extracting the non-WebAPK display mode and orientation. I have not added tests that the enum values in the intent are overridden by the existence of the string. I chose the priority order randomly so I do not think that it is worth testing the priority order. https://codereview.chromium.org/2094903003/diff/60001/chrome/android/webapk/l... File chrome/android/webapk/libs/common/src/org/chromium/webapk/lib/common/WebApkConstants.java (right): https://codereview.chromium.org/2094903003/diff/60001/chrome/android/webapk/l... chrome/android/webapk/libs/common/src/org/chromium/webapk/lib/common/WebApkConstants.java:17: public static final String EXTRA_DISPLAY_MODE = "org.chromium.webapk.lib.common.display_mode"; Ok renamed to EXTRA_WEBAPK_DISPLAY_MODE https://codereview.chromium.org/2094903003/diff/60001/chrome/android/webapk/s... File chrome/android/webapk/shell_apk/BUILD.gn (right): https://codereview.chromium.org/2094903003/diff/60001/chrome/android/webapk/s... chrome/android/webapk/shell_apk/BUILD.gn:21: webapk_orientation = "landscape" Ok changed to portrait. The defaults are for testing only. These are not the defaults for WebAPK generation so their values do not matter a whole lot. https://codereview.chromium.org/2094903003/diff/60001/chrome/android/webapk/s... File chrome/android/webapk/shell_apk/src/org/chromium/webapk/shell_apk/MainActivity.java (right): https://codereview.chromium.org/2094903003/diff/60001/chrome/android/webapk/s... chrome/android/webapk/shell_apk/src/org/chromium/webapk/shell_apk/MainActivity.java:30: private static final String EXTRA_ID = "org.chromium.chrome.browser.webapp_id"; No it is not possible to use ShortcutHelper directly Code in //chrome/android/webapk/shell_apk is bundled into an APK which is downloaded and installed when a user adds a web page to the home screen via the app menu. Code in webapk/shell_apk cannot depend on anything in Chrome (e.g. it cannot depend on //base) The only code which is shared between webapk/shell_apk and Chrome is the code in webapk/libs/common. I do not think that it makes sense to move the ShortcutHelper constants into org.chromium.webapk.lib.common.WebApkConstants
dominickn@ can you please take another look? https://codereview.chromium.org/2094903003/diff/60001/chrome/android/javatest... File chrome/android/javatests/src/org/chromium/chrome/browser/webapps/WebappInfoTest.java (right): https://codereview.chromium.org/2094903003/diff/60001/chrome/android/javatest... chrome/android/javatests/src/org/chromium/chrome/browser/webapps/WebappInfoTest.java:274: } I have added tests for extracting the non-WebAPK display mode and orientation. I have not added tests that the enum values in the intent are overridden by the existence of the string. I chose the priority order randomly so I do not think that it is worth testing the priority order. https://codereview.chromium.org/2094903003/diff/60001/chrome/android/webapk/l... File chrome/android/webapk/libs/common/src/org/chromium/webapk/lib/common/WebApkConstants.java (right): https://codereview.chromium.org/2094903003/diff/60001/chrome/android/webapk/l... chrome/android/webapk/libs/common/src/org/chromium/webapk/lib/common/WebApkConstants.java:17: public static final String EXTRA_DISPLAY_MODE = "org.chromium.webapk.lib.common.display_mode"; Ok renamed to EXTRA_WEBAPK_DISPLAY_MODE https://codereview.chromium.org/2094903003/diff/60001/chrome/android/webapk/s... File chrome/android/webapk/shell_apk/BUILD.gn (right): https://codereview.chromium.org/2094903003/diff/60001/chrome/android/webapk/s... chrome/android/webapk/shell_apk/BUILD.gn:21: webapk_orientation = "landscape" Ok changed to portrait. The defaults are for testing only. These are not the defaults for WebAPK generation so their values do not matter a whole lot. https://codereview.chromium.org/2094903003/diff/60001/chrome/android/webapk/s... File chrome/android/webapk/shell_apk/src/org/chromium/webapk/shell_apk/MainActivity.java (right): https://codereview.chromium.org/2094903003/diff/60001/chrome/android/webapk/s... chrome/android/webapk/shell_apk/src/org/chromium/webapk/shell_apk/MainActivity.java:30: private static final String EXTRA_ID = "org.chromium.chrome.browser.webapp_id"; No it is not possible to use ShortcutHelper directly Code in //chrome/android/webapk/shell_apk is bundled into an APK which is downloaded and installed when a user adds a web page to the home screen via the app menu. Code in webapk/shell_apk cannot depend on anything in Chrome (e.g. it cannot depend on //base) The only code which is shared between webapk/shell_apk and Chrome is the code in webapk/libs/common. I do not think that it makes sense to move the ShortcutHelper constants into org.chromium.webapk.lib.common.WebApkConstants
https://codereview.chromium.org/2094903003/diff/60001/chrome/android/javatest... File chrome/android/javatests/src/org/chromium/chrome/browser/webapps/WebappInfoTest.java (right): https://codereview.chromium.org/2094903003/diff/60001/chrome/android/javatest... chrome/android/javatests/src/org/chromium/chrome/browser/webapps/WebappInfoTest.java:274: } On 2016/06/28 17:56:21, pkotwicz wrote: > I have added tests for extracting the non-WebAPK display mode and orientation. > > I have not added tests that the enum values in the intent are overridden by the > existence of the string. I chose the priority order randomly so I do not think > that it is worth testing the priority order. Even though you chose the order randomly, it's still important to unit test it, particularly since it's destructive. You're setting a canonical value in AndroidManifest.xml, so testing the overriding behaviour is necessary to ensure that the canonical value stays canonical. https://codereview.chromium.org/2094903003/diff/60001/chrome/android/webapk/s... File chrome/android/webapk/shell_apk/src/org/chromium/webapk/shell_apk/MainActivity.java (right): https://codereview.chromium.org/2094903003/diff/60001/chrome/android/webapk/s... chrome/android/webapk/shell_apk/src/org/chromium/webapk/shell_apk/MainActivity.java:30: private static final String EXTRA_ID = "org.chromium.chrome.browser.webapp_id"; On 2016/06/28 17:56:22, pkotwicz wrote: > No it is not possible to use ShortcutHelper directly > > Code in //chrome/android/webapk/shell_apk is bundled into an APK which is > downloaded and installed when a user adds a web page to the home screen via the > app menu. Code in webapk/shell_apk cannot depend on anything in Chrome (e.g. it > cannot depend on //base) > > The only code which is shared between webapk/shell_apk and Chrome is the code in > webapk/libs/common. I do not think that it makes sense to move the > ShortcutHelper constants into org.chromium.webapk.lib.common.WebApkConstants Acknowledged. Out of curiosity, why don't you think moving the ShortcutHelper constants into WebApkConstants makes sense? Alternatively, why is duplicating the constants a better solution?
dominickn@ can you please take another look? https://codereview.chromium.org/2094903003/diff/60001/chrome/android/javatest... File chrome/android/javatests/src/org/chromium/chrome/browser/webapps/WebappInfoTest.java (right): https://codereview.chromium.org/2094903003/diff/60001/chrome/android/javatest... chrome/android/javatests/src/org/chromium/chrome/browser/webapps/WebappInfoTest.java:274: } I added unit tests :) https://codereview.chromium.org/2094903003/diff/60001/chrome/android/webapk/s... File chrome/android/webapk/shell_apk/src/org/chromium/webapk/shell_apk/MainActivity.java (right): https://codereview.chromium.org/2094903003/diff/60001/chrome/android/webapk/s... chrome/android/webapk/shell_apk/src/org/chromium/webapk/shell_apk/MainActivity.java:30: private static final String EXTRA_ID = "org.chromium.chrome.browser.webapp_id"; I don't have a very good answer for why I don't think it makes sense. I will discuss with hanxi@ and yfriedman@ to get their opinion. If I move the ShortcutHelper constants to WebApkConstants I will do it in a separate CL Some context: I am trying to have as little code as possible in webapk/libs/common because: - It is expensive to change WebApkConstants. Any changes require any WebAPKs currently installed on the user's device to be re-downloaded from the WebAPK server. Thus, WebApkConstants should not have non-WebAPK-applicable ShortcutHelper constants. (Currently there are no non-WebAPK-applicable ShortcutHelper constants) - It is hard to think about files in webapk/libs/common. It is possible for the WebAPK apk to use an older version of WebApkConstants than chrome_apk
On 2016/06/29 01:54:00, pkotwicz wrote: > dominickn@ can you please take another look? > > https://codereview.chromium.org/2094903003/diff/60001/chrome/android/javatest... > File > chrome/android/javatests/src/org/chromium/chrome/browser/webapps/WebappInfoTest.java > (right): > > https://codereview.chromium.org/2094903003/diff/60001/chrome/android/javatest... > chrome/android/javatests/src/org/chromium/chrome/browser/webapps/WebappInfoTest.java:274: > } > I added unit tests :) > > https://codereview.chromium.org/2094903003/diff/60001/chrome/android/webapk/s... > File > chrome/android/webapk/shell_apk/src/org/chromium/webapk/shell_apk/MainActivity.java > (right): > > https://codereview.chromium.org/2094903003/diff/60001/chrome/android/webapk/s... > chrome/android/webapk/shell_apk/src/org/chromium/webapk/shell_apk/MainActivity.java:30: > private static final String EXTRA_ID = "org.chromium.chrome.browser.webapp_id"; > I don't have a very good answer for why I don't think it makes sense. > I will discuss with hanxi@ and yfriedman@ to get their opinion. If I move the > ShortcutHelper constants to WebApkConstants I will do it in a separate CL > > Some context: I am trying to have as little code as possible in > webapk/libs/common because: > - It is expensive to change WebApkConstants. Any changes require any WebAPKs > currently installed on the user's device to be re-downloaded from the WebAPK > server. Thus, WebApkConstants should not have non-WebAPK-applicable > ShortcutHelper constants. (Currently there are no non-WebAPK-applicable > ShortcutHelper constants) > - It is hard to think about files in webapk/libs/common. It is possible for the > WebAPK apk to use an older version of WebApkConstants than chrome_apk Your reasoning makes sense - sounds like WebApkConstants might not be what we want to use. Sometime in the future we should try and come up with a way to remove the duplication without adding to WebApkConstants. :) Thanks for adding the tests. lgtm.
On 2016/06/29 02:11:18, dominickn wrote: > On 2016/06/29 01:54:00, pkotwicz wrote: > > dominickn@ can you please take another look? > > > > > https://codereview.chromium.org/2094903003/diff/60001/chrome/android/javatest... > > File > > > chrome/android/javatests/src/org/chromium/chrome/browser/webapps/WebappInfoTest.java > > (right): > > > > > https://codereview.chromium.org/2094903003/diff/60001/chrome/android/javatest... > > > chrome/android/javatests/src/org/chromium/chrome/browser/webapps/WebappInfoTest.java:274: > > } > > I added unit tests :) > > > > > https://codereview.chromium.org/2094903003/diff/60001/chrome/android/webapk/s... > > File > > > chrome/android/webapk/shell_apk/src/org/chromium/webapk/shell_apk/MainActivity.java > > (right): > > > > > https://codereview.chromium.org/2094903003/diff/60001/chrome/android/webapk/s... > > > chrome/android/webapk/shell_apk/src/org/chromium/webapk/shell_apk/MainActivity.java:30: > > private static final String EXTRA_ID = > "org.chromium.chrome.browser.webapp_id"; > > I don't have a very good answer for why I don't think it makes sense. > > I will discuss with hanxi@ and yfriedman@ to get their opinion. If I move the > > ShortcutHelper constants to WebApkConstants I will do it in a separate CL > > > > Some context: I am trying to have as little code as possible in > > webapk/libs/common because: > > - It is expensive to change WebApkConstants. Any changes require any WebAPKs > > currently installed on the user's device to be re-downloaded from the WebAPK > > server. Thus, WebApkConstants should not have non-WebAPK-applicable > > ShortcutHelper constants. (Currently there are no non-WebAPK-applicable > > ShortcutHelper constants) > > - It is hard to think about files in webapk/libs/common. It is possible for > the > > WebAPK apk to use an older version of WebApkConstants than chrome_apk > > Your reasoning makes sense - sounds like WebApkConstants might not be what > we want to use. Sometime in the future we should try and come up with a way to > remove the duplication without adding to WebApkConstants. :) > > Thanks for adding the tests. lgtm. FYI you can TBR dfalcantara for WebappInfo.java (I should really own that too).
Description was changed from ========== Pass all intent extras needed to render splash screen when launching WebAPK This CL adds to the WebAPK's Android Manifest attributes for rendering the splash screen. These attributes are passed to WebappActivity. The "string value" for the display and orientation attributes is used in the WebAPK's Android Manifest instead of the WebDisplayMode and ScreenOrientationValues enum values. This is done to enable WebAPKs to work with non-Chrome browsers in the future. BUG=623216 TEST=WebappInfoTest ========== to ========== Pass all intent extras needed to render splash screen when launching WebAPK This CL adds to the WebAPK's Android Manifest attributes for rendering the splash screen. These attributes are passed to WebappActivity. The "string value" for the display and orientation attributes is used in the WebAPK's Android Manifest instead of the WebDisplayMode and ScreenOrientationValues enum values. This is done to enable WebAPKs to work with non-Chrome browsers in the future. BUG=623216 TEST=WebappInfoTest R=hanxi, dominickn TBR=dfalcantara ==========
The CQ bit was checked by pkotwicz@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from hanxi@chromium.org, dominickn@chromium.org Link to the patchset: https://codereview.chromium.org/2094903003/#ps120001 (title: "Merge branch 'master' into webapk_more_meta")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Description was changed from ========== Pass all intent extras needed to render splash screen when launching WebAPK This CL adds to the WebAPK's Android Manifest attributes for rendering the splash screen. These attributes are passed to WebappActivity. The "string value" for the display and orientation attributes is used in the WebAPK's Android Manifest instead of the WebDisplayMode and ScreenOrientationValues enum values. This is done to enable WebAPKs to work with non-Chrome browsers in the future. BUG=623216 TEST=WebappInfoTest R=hanxi, dominickn TBR=dfalcantara ========== to ========== Pass all intent extras needed to render splash screen when launching WebAPK This CL adds to the WebAPK's Android Manifest attributes for rendering the splash screen. These attributes are passed to WebappActivity. The "string value" for the display and orientation attributes is used in the WebAPK's Android Manifest instead of the WebDisplayMode and ScreenOrientationValues enum values. This is done to enable WebAPKs to work with non-Chrome browsers in the future. BUG=623216 TEST=WebappInfoTest R=hanxi, dominickn TBR=dfalcantara ==========
Message was sent while issue was closed.
Committed patchset #7 (id:120001)
Message was sent while issue was closed.
Description was changed from ========== Pass all intent extras needed to render splash screen when launching WebAPK This CL adds to the WebAPK's Android Manifest attributes for rendering the splash screen. These attributes are passed to WebappActivity. The "string value" for the display and orientation attributes is used in the WebAPK's Android Manifest instead of the WebDisplayMode and ScreenOrientationValues enum values. This is done to enable WebAPKs to work with non-Chrome browsers in the future. BUG=623216 TEST=WebappInfoTest R=hanxi, dominickn TBR=dfalcantara ========== to ========== Pass all intent extras needed to render splash screen when launching WebAPK This CL adds to the WebAPK's Android Manifest attributes for rendering the splash screen. These attributes are passed to WebappActivity. The "string value" for the display and orientation attributes is used in the WebAPK's Android Manifest instead of the WebDisplayMode and ScreenOrientationValues enum values. This is done to enable WebAPKs to work with non-Chrome browsers in the future. BUG=623216 TEST=WebappInfoTest R=hanxi, dominickn TBR=dfalcantara Committed: https://crrev.com/f8678dfec2891e16e637db2459125edd32fe3c91 Cr-Commit-Position: refs/heads/master@{#402965} ==========
Message was sent while issue was closed.
Patchset 7 (id:??) landed as https://crrev.com/f8678dfec2891e16e637db2459125edd32fe3c91 Cr-Commit-Position: refs/heads/master@{#402965} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
