|
|
Created:
3 years, 7 months ago by Xi Han Modified:
3 years, 7 months ago CC:
chromium-reviews, dominickn+watch_chromium.org, pkotwicz+watch_chromium.org, zpeng+watch_chromium.org, asvitkine+watch_chromium.org, agrieve+watch_chromium.org Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
DescriptionCorrectly record WebAPK install source for Launch.HomescreenSource
In this CL:
1) Introduces source WEBAPK_UNKNONW for WebAPKs whose source info are lost.
2) Stores the WebAPK's source info after the install is either succeeded or timed
out.
3) Retrives the source info from WebappDataStorage when the WebAPK is launched,
and records the launch metrics after the source is avaiable.
BUG=711147
Review-Url: https://codereview.chromium.org/2860193002
Cr-Commit-Position: refs/heads/master@{#474285}
Committed: https://chromium.googlesource.com/chromium/src/+/80906b06689bd05c3b727c115242bedd7d6910f2
Patch Set 1 #
Total comments: 12
Patch Set 2 #
Total comments: 10
Patch Set 3 : Change the default value of source to WEBAPK_UNKNOWN for Webapp. #
Total comments: 8
Patch Set 4 : Revert the default source of Webapp to UNKNOWN, and warm up sharedPreference in WebappLauncherActivity. #
Total comments: 4
Patch Set 5 : Nits. #Patch Set 6 : Rever the source change in WebAPK. #
Total comments: 4
Patch Set 7 : Nits. #
Total comments: 2
Patch Set 8 : Nits. #
Total comments: 2
Patch Set 9 : Nits. #Patch Set 10 : Rebase. #Messages
Total messages: 54 (29 generated)
Description was changed from ========== Correctly record WebAPK install source for Launch.HomescreenSource In this CL: 1) Introduce source WEBAPK_UNKNONW for WebAPKs whose source info are lost. 2) Store the WebAPK's source info after the install is either succeeded or timed out. 3) Retrives the source info from WebappDataStorage when the WebAPK is launched, and records the launch metrics after the source is avaiable. BUG=711147 ========== to ========== Correctly record WebAPK install source for Launch.HomescreenSource In this CL: 1) Introduces source WEBAPK_UNKNONW for WebAPKs whose source info are lost. 2) Stores the WebAPK's source info after the install is either succeeded or timed out. 3) Retrives the source info from WebappDataStorage when the WebAPK is launched, and records the launch metrics after the source is avaiable. BUG=711147 ==========
Patchset #1 (id:1) has been deleted
hanxi@chromium.org changed reviewers: + pkotwicz@chromium.org
Hi Peter, could you please take a look? Thanks!
https://codereview.chromium.org/2860193002/diff/20001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/webapps/WebApkActivity.java (right): https://codereview.chromium.org/2860193002/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/webapps/WebApkActivity.java:186: storage.updateTimeOfLastCheckForUpdatedWebManifest(); You need https://codereview.chromium.org/2866773003/ :) https://codereview.chromium.org/2860193002/diff/20001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappDataStorage.java (right): https://codereview.chromium.org/2860193002/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappDataStorage.java:282: ShortcutSource.UNKNOWN)); If a WebAPK is launched the first time from a deep link won't this code cause us to report that the WebAPK was launched from a deep link every subsequent time? I suggest deleting lines 280 - 282 https://codereview.chromium.org/2860193002/diff/20001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappLauncherActivity.java (right): https://codereview.chromium.org/2860193002/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappLauncherActivity.java:86: } Can we query the source from WebappDataStorage here? Also your code in WebApkActivity#onResumeWithNative() seems to ignore the source when it is passed via the intent (e.g. when the WebAPK is launched from a notification) https://codereview.chromium.org/2860193002/diff/20001/chrome/android/webapk/s... File chrome/android/webapk/shell_apk/shell_apk_version.gni (right): https://codereview.chromium.org/2860193002/diff/20001/chrome/android/webapk/s... chrome/android/webapk/shell_apk/shell_apk_version.gni:9: template_shell_apk_version = 5 6 because of https://codereview.chromium.org/2849873002/ :) https://codereview.chromium.org/2860193002/diff/20001/chrome/browser/android/... File chrome/browser/android/shortcut_info.h (right): https://codereview.chromium.org/2860193002/diff/20001/chrome/browser/android/... chrome/browser/android/shortcut_info.h:52: // Used for WebAPK PWAs whose install source info are lost. Nit: "are lost" -> "was lost" (because "source information" is singular)
Patchset #2 (id:40001) has been deleted
Patchset #2 (id:60001) has been deleted
Hi Peter, PTAL, thanks! https://codereview.chromium.org/2860193002/diff/20001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/webapps/WebApkActivity.java (right): https://codereview.chromium.org/2860193002/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/webapps/WebApkActivity.java:186: storage.updateTimeOfLastCheckForUpdatedWebManifest(); On 2017/05/05 21:05:43, pkotwicz wrote: > You need https://codereview.chromium.org/2866773003/ :) Acknowledged. https://codereview.chromium.org/2860193002/diff/20001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappDataStorage.java (right): https://codereview.chromium.org/2860193002/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappDataStorage.java:282: ShortcutSource.UNKNOWN)); On 2017/05/05 21:05:43, pkotwicz wrote: > If a WebAPK is launched the first time from a deep link won't this code cause us > to report that the WebAPK was launched from a deep link every subsequent time? > > I suggest deleting lines 280 - 282 > Oh, I thought we wouldn't pass the check in line 254, but obviously we did. I change it to not set source for WebAPKs. https://codereview.chromium.org/2860193002/diff/20001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappLauncherActivity.java (right): https://codereview.chromium.org/2860193002/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappLauncherActivity.java:86: } On 2017/05/05 21:05:43, pkotwicz wrote: > Can we query the source from WebappDataStorage here? If we query the source here, we could potentially slow down the launch of WebAPKs, right? In fact, I don't think recording launch metrics for webapk in here is right. The WebappLauncherActivity is always created no matter whether the associated WebApkActivity has been created or not. We ends up recording more than the real launch times. > Also your code in WebApkActivity#onResumeWithNative() seems to ignore the source > when it is passed via the intent (e.g. when the WebAPK is launched from a > notification) Good catch, updated. Now we only records the source once, and we record the source of Notification when a WebAPK is launched from a notification, but not if the notification just brings a existing WebAPK to foreground. https://codereview.chromium.org/2860193002/diff/20001/chrome/android/webapk/s... File chrome/android/webapk/shell_apk/shell_apk_version.gni (right): https://codereview.chromium.org/2860193002/diff/20001/chrome/android/webapk/s... chrome/android/webapk/shell_apk/shell_apk_version.gni:9: template_shell_apk_version = 5 On 2017/05/05 21:05:43, pkotwicz wrote: > 6 because of https://codereview.chromium.org/2849873002/ :) I will rebase after this CL lands:) https://codereview.chromium.org/2860193002/diff/20001/chrome/browser/android/... File chrome/browser/android/shortcut_info.h (right): https://codereview.chromium.org/2860193002/diff/20001/chrome/browser/android/... chrome/browser/android/shortcut_info.h:52: // Used for WebAPK PWAs whose install source info are lost. On 2017/05/05 21:05:43, pkotwicz wrote: > Nit: "are lost" -> "was lost" (because "source information" is singular) Thanks!
pkotwicz@chromium.org changed reviewers: + dominickn@chromium.org
CCing Dominick as he likely has wisdom to share (and might find flaws in my suggestions) https://codereview.chromium.org/2860193002/diff/20001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappLauncherActivity.java (right): https://codereview.chromium.org/2860193002/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappLauncherActivity.java:86: } > If we query the source here, we could potentially slow down the launch of > WebAPKs, right? In fact, I don't think recording launch metrics for webapk in > here is right. The WebappLauncherActivity is always created no matter whether > the associated WebApkActivity has been created or not. We ends up recording more > than the real launch times. - I think that the launch metrics are recorded each time WebappLauncherActivity is created instead of when a Webapp/WebAPK is created on purpose. I don't have a strong opinion as to whether we should record metrics when WebappLauncherActivity is created or when WebappActivity/WebApkActivity is created but it would be nice to be consistent for WebAPKs and Webapps - Querying WebappDataStorage is cheap compared to isValidWebApk() - In the future, we might check WebappDataStorage to determine whether a WebAPK is valid prior to running isValidWebApk() to speed up startup time (because verifying unbound WebAPKs is so slow) so we might end up querying WebappDataStorage in WebappLauncherActivity anyways https://codereview.chromium.org/2860193002/diff/80001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/webapps/WebApkActivity.java (right): https://codereview.chromium.org/2860193002/diff/80001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/webapps/WebApkActivity.java:147: } I think (Dominick might disagree) that we should determine whether the source is ShortcutSource.UNKNOWN or ShortcutSource.WEBAPK_UNKNOWN prior to calling LaunchMetrics#recordHomeScreenLaunchIntoStandaloneActivity() In particular, we would not store WEBAPK_UNKNOWN into WebappDataStorage https://codereview.chromium.org/2860193002/diff/80001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/webapps/WebApkInstaller.java (right): https://codereview.chromium.org/2860193002/diff/80001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/webapps/WebApkInstaller.java:79: WebappRegistry.getInstance().register( Nit: It might be worth adding a new method WebappRegistry#registerWebApk() I would be tempted to call WebappDataStorage#updateFromShortcutIntent() for the sake of clarity here (via WebApkInfo#create() and WebApkInfo#setWebappIntentExtras()). I think that the PackageManager will be able to find the WebApk immediately after installation
I'll need to think about this for a bit - it's really unfortunate that we have to special case WebAPKs out of WebappLauncherActivity's recordMetrics call. https://codereview.chromium.org/2860193002/diff/80001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/webapps/WebApkActivity.java (right): https://codereview.chromium.org/2860193002/diff/80001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/webapps/WebApkActivity.java:138: if (mWebappInfo.isLaunchedFromHomescreen()) { I'm not sure this works. isLaunchedFromHomescreen() just checks mWebappInfo.source(), which is taken from the intent used to launch the WebAPK. That source is unknown right - we have to get it from WebappDataStorage? https://codereview.chromium.org/2860193002/diff/80001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/webapps/WebApkActivity.java:147: } On 2017/05/09 18:37:36, pkotwicz wrote: > I think (Dominick might disagree) that we should determine whether the source is > ShortcutSource.UNKNOWN or ShortcutSource.WEBAPK_UNKNOWN prior to calling > LaunchMetrics#recordHomeScreenLaunchIntoStandaloneActivity() > In particular, we would not store WEBAPK_UNKNOWN into WebappDataStorage > I agree that if possible, we shouldn't explicitly store UNKNOWN - we should just default to WEBAPK_UNKNOWN if we don't get back anything from WebappDataStorage (or just return WEBAPK_UNKNOWN as the default value out of WebappDataStorage). If it's not possible I'm fine with explicitly storing it though. https://codereview.chromium.org/2860193002/diff/80001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/webapps/WebApkActivity.java:191: storage.updateSource(ShortcutSource.WEBAPK_UNKNOWN); As noted above, I agree with Peter that we shouldn't explicitly store UNKNOWN, but default to it in the WebAPK path if possible.
Patchset #3 (id:100001) has been deleted
Patchset #3 (id:120001) has been deleted
Patchset #3 (id:140001) has been deleted
Patchset #3 (id:160001) has been deleted
Patchset #4 (id:200001) has been deleted
Thank you both. I have remove the WebAPK's special way to record launch metrics. PTAL, thanks! https://codereview.chromium.org/2860193002/diff/20001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappLauncherActivity.java (right): https://codereview.chromium.org/2860193002/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappLauncherActivity.java:86: } On 2017/05/09 18:37:36, pkotwicz wrote: > > If we query the source here, we could potentially slow down the launch of > > WebAPKs, right? In fact, I don't think recording launch metrics for webapk in > > here is right. The WebappLauncherActivity is always created no matter whether > > the associated WebApkActivity has been created or not. We ends up recording > more > > than the real launch times. > > - I think that the launch metrics are recorded each time WebappLauncherActivity > is created instead of when a Webapp/WebAPK is created on purpose. I don't have a > strong opinion as to whether we should record metrics when > WebappLauncherActivity is created or when WebappActivity/WebApkActivity is > created but it would be nice to be consistent for WebAPKs and Webapps > > - Querying WebappDataStorage is cheap compared to isValidWebApk() > > - In the future, we might check WebappDataStorage to determine whether a WebAPK > is valid prior to running isValidWebApk() to speed up startup time (because > verifying unbound WebAPKs is so slow) so we might end up querying > WebappDataStorage in WebappLauncherActivity anyways Yes, we would check WebappDataStorage here. Updated my code. Now we don't need to specialize WebAPK case. I guess I understand why the LaunchMetrics.recordHomeScreenLaunchIntoStandaloneActivity() is called here. This is because it doesn't matter where it is called, the LaunchMetrics.commitMetrics() is called only once in ChromeActvity#onResumeWithNative(). https://codereview.chromium.org/2860193002/diff/80001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/webapps/WebApkActivity.java (right): https://codereview.chromium.org/2860193002/diff/80001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/webapps/WebApkActivity.java:138: if (mWebappInfo.isLaunchedFromHomescreen()) { On 2017/05/10 06:04:46, dominickn wrote: > I'm not sure this works. isLaunchedFromHomescreen() just checks > mWebappInfo.source(), which is taken from the intent used to launch the WebAPK. > That source is unknown right - we have to get it from WebappDataStorage? If the WebAPK is resumed by external Intent or notification, this check could make sure we don't get the source from the WebappDataStorage, right? https://codereview.chromium.org/2860193002/diff/80001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/webapps/WebApkActivity.java:147: } On 2017/05/10 06:04:46, dominickn wrote: > On 2017/05/09 18:37:36, pkotwicz wrote: > > I think (Dominick might disagree) that we should determine whether the source > is > > ShortcutSource.UNKNOWN or ShortcutSource.WEBAPK_UNKNOWN prior to calling > > LaunchMetrics#recordHomeScreenLaunchIntoStandaloneActivity() > > In particular, we would not store WEBAPK_UNKNOWN into WebappDataStorage > > > > I agree that if possible, we shouldn't explicitly store UNKNOWN - we should just > default to WEBAPK_UNKNOWN if we don't get back anything from WebappDataStorage > (or just return WEBAPK_UNKNOWN as the default value out of WebappDataStorage). > If it's not possible I'm fine with explicitly storing it though. Agree, it is neat to have WEBAPK_UNKNOWN as the default source for both Webapp and WebAPKs. https://codereview.chromium.org/2860193002/diff/80001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/webapps/WebApkActivity.java:191: storage.updateSource(ShortcutSource.WEBAPK_UNKNOWN); On 2017/05/10 06:04:46, dominickn wrote: > As noted above, I agree with Peter that we shouldn't explicitly store UNKNOWN, > but default to it in the WebAPK path if possible. Removed.
https://codereview.chromium.org/2860193002/diff/80001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/webapps/WebApkActivity.java (right): https://codereview.chromium.org/2860193002/diff/80001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/webapps/WebApkActivity.java:147: } I wasn't suggesting changing the default source in WebappDataStorage. Not sure if Dominick was suggesting it. I was thinking of something like this in WebappLauncherActivity.java if (validWebApk && webappSource == ShortcutSource.UNKNOWN) { WebappDataStorage storage = WebappRegistry.getInstance().getWebappDataStorage(webappInfo.id()); if (storage != null) { webappSource = storage.getSource(); if (webappSource == ShortcutSource.UNKNOWN) { webappSource = ShortcutSource.WEBAPK_UNKNOWN; } } } LaunchMetrics.recordHomeScreenLaunchIntoStandaloneActivity(webappUrl, webappSource); https://codereview.chromium.org/2860193002/diff/180001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappLauncherActivity.java (right): https://codereview.chromium.org/2860193002/diff/180001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappLauncherActivity.java:86: storage != null ? storage.getSource() : ShortcutSource.WEBAPK_UNKNOWN; Nit: I like putting () around conditionals when writing statements like this because it makes it clearer in my opinion. e.g. webAppSource = (storage != null) ? storage.getSource() : ShortcutSource.WEBAPK_UNKNOWN;
We should only use WEBAPK_UNKNOWN as the source for WebAPKs, not as the universal default. https://codereview.chromium.org/2860193002/diff/180001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappInfo.java (right): https://codereview.chromium.org/2860193002/diff/180001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappInfo.java:80: intent, ShortcutHelper.EXTRA_SOURCE, ShortcutSource.WEBAPK_UNKNOWN); The default should remain as ShortcutSource.UNKNOWN. https://codereview.chromium.org/2860193002/diff/180001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappLauncherActivity.java (right): https://codereview.chromium.org/2860193002/diff/180001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappLauncherActivity.java:82: if (validWebApk && webappInfo.isLaunchedFromHomescreen()) { Using isLaunchedFromHomescreen works, but only because MainActivity#launchHostBrowserInWebApkMode injects the EXTERNAL_INTENT source if this is launched from an external intent. Similarly, ServiceTabLauncher#launchTab injects the NOTIFICATION source. A homescreen launch for a WebAPK will always have an empty/default source at this stage. I think it's safer to use: webappSource == (expected default value here) Since we might have other places in the future where we need to explicitly inject a source on the incoming intent. https://codereview.chromium.org/2860193002/diff/180001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappLauncherActivity.java:84: WebappRegistry.getInstance().getWebappDataStorage(webappInfo.id()); When WebappLauncherActivity is run, have we called through to WebappRegistry#warmUpSharedPrefs yet? If not, this won't return anything. WebappLauncherActivity only inherits from Activity so I don't think it will have loaded the DataStorage objects on startup.
Patchset #4 (id:220001) has been deleted
Patchset #4 (id:240001) has been deleted
Patchset #4 (id:260001) 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...
I reverted the default source of Webapp to ShortcutSouce.UNKNOWN. PTAL, thanks! https://codereview.chromium.org/2860193002/diff/80001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/webapps/WebApkActivity.java (right): https://codereview.chromium.org/2860193002/diff/80001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/webapps/WebApkActivity.java:147: } On 2017/05/10 21:08:59, pkotwicz wrote: > I wasn't suggesting changing the default source in WebappDataStorage. Not sure > if Dominick was suggesting it. > > I was thinking of something like this in WebappLauncherActivity.java > > if (validWebApk && webappSource == ShortcutSource.UNKNOWN) { > WebappDataStorage storage = > WebappRegistry.getInstance().getWebappDataStorage(webappInfo.id()); > if (storage != null) { > webappSource = storage.getSource(); > if (webappSource == ShortcutSource.UNKNOWN) { > webappSource = ShortcutSource.WEBAPK_UNKNOWN; > } > } > } > LaunchMetrics.recordHomeScreenLaunchIntoStandaloneActivity(webappUrl, > webappSource); Ok, I must misunderstood. Revert the default source for Webapps to ShortcutSource.UNKNOWN. https://codereview.chromium.org/2860193002/diff/180001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappInfo.java (right): https://codereview.chromium.org/2860193002/diff/180001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappInfo.java:80: intent, ShortcutHelper.EXTRA_SOURCE, ShortcutSource.WEBAPK_UNKNOWN); On 2017/05/11 03:35:49, dominickn (OOO May 12-19th) wrote: > The default should remain as ShortcutSource.UNKNOWN. Done. https://codereview.chromium.org/2860193002/diff/180001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappLauncherActivity.java (right): https://codereview.chromium.org/2860193002/diff/180001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappLauncherActivity.java:82: if (validWebApk && webappInfo.isLaunchedFromHomescreen()) { On 2017/05/11 03:35:49, dominickn (OOO May 12-19th) wrote: > Using isLaunchedFromHomescreen works, but only because > MainActivity#launchHostBrowserInWebApkMode injects the EXTERNAL_INTENT source if > this is launched from an external intent. Similarly, > ServiceTabLauncher#launchTab injects the NOTIFICATION source. A homescreen > launch for a WebAPK will always have an empty/default source at this stage. > > I think it's safer to use: > > webappSource == (expected default value here) > > Since we might have other places in the future where we need to explicitly > inject a source on the incoming intent. Done. https://codereview.chromium.org/2860193002/diff/180001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappLauncherActivity.java:84: WebappRegistry.getInstance().getWebappDataStorage(webappInfo.id()); On 2017/05/11 03:35:49, dominickn (OOO May 12-19th) wrote: > When WebappLauncherActivity is run, have we called through to > WebappRegistry#warmUpSharedPrefs yet? If not, this won't return anything. > WebappLauncherActivity only inherits from Activity so I don't think it will have > loaded the DataStorage objects on startup. Good catch! Add a call of WebappRegistry#warmUpSharedPrefs() here. https://codereview.chromium.org/2860193002/diff/180001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappLauncherActivity.java:86: storage != null ? storage.getSource() : ShortcutSource.WEBAPK_UNKNOWN; On 2017/05/10 21:08:59, pkotwicz wrote: > Nit: I like putting () around conditionals when writing statements like this > because it makes it clearer in my opinion. > > e.g. > > webAppSource = (storage != null) ? storage.getSource() : > ShortcutSource.WEBAPK_UNKNOWN; Acknowledged.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Just nits! Shouldn't you also need to update ShortcutSource.UNKNOWN in ShortcutHelper#retrieveWebApks() https://codereview.chromium.org/2860193002/diff/280001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappLauncherActivity.java (right): https://codereview.chromium.org/2860193002/diff/280001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappLauncherActivity.java:83: // Retrieves the source of the WebAPK from WebappDataStorage if it is unknown. Can this block be part of a new function? Maybe WebappLauncherActivity#storeWebApkSourceInStorage() Overwriting webappSource is slightly confusing because webappSource is used later in this function on line 110. I am aware of the return on line 103 https://codereview.chromium.org/2860193002/diff/280001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappLauncherActivity.java:89: WebappRegistry.getInstance().getWebappDataStorage(webappInfo.id()); Can we move the remainder of this block outside of the try like this: WebappDataStorage storage = null; try { ... } finally { } if (storage != null) { ... }
Patchset #5 (id:300001) has been deleted
PTAL, thanks! https://codereview.chromium.org/2860193002/diff/280001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappLauncherActivity.java (right): https://codereview.chromium.org/2860193002/diff/280001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappLauncherActivity.java:83: // Retrieves the source of the WebAPK from WebappDataStorage if it is unknown. On 2017/05/11 18:10:33, pkotwicz wrote: > Can this block be part of a new function? Maybe > WebappLauncherActivity#storeWebApkSourceInStorage() How about |getWebApkSource|? The function reads the source from the WebappDataStorage, rather than stores. > Overwriting webappSource is slightly confusing because webappSource is used > later in this function on line 110. I am aware of the return on line 103 Hmm, I will add another local variable. https://codereview.chromium.org/2860193002/diff/280001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappLauncherActivity.java:89: WebappRegistry.getInstance().getWebappDataStorage(webappInfo.id()); On 2017/05/11 18:10:33, pkotwicz wrote: > Can we move the remainder of this block outside of the try like this: > > WebappDataStorage storage = null; > try { > ... > } finally { > } > if (storage != null) { > ... > } Done.
PTAL, thanks!
pkotwicz@chromium.org changed reviewers: + yfriedman@chromium.org
Yaron for OWNERS LGTM. Sorry for the delay.
https://codereview.chromium.org/2860193002/diff/340001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/webapps/WebApkInstaller.java (right): https://codereview.chromium.org/2860193002/diff/340001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/webapps/WebApkInstaller.java:79: WebappRegistry.getInstance().register( I honestly can't remember the implications for doing this now - I had thought we considered doing this at one point but didn't want to register with the registry too early... Do you remember that concern and/or why it's not valid? https://codereview.chromium.org/2860193002/diff/340001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappLauncherActivity.java (right): https://codereview.chromium.org/2860193002/diff/340001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappLauncherActivity.java:103: WebappRegistry.warmUpSharedPrefsForId(webappInfo.id()); i don't think this is buying you anything. you're fetching 2 lines later.
PTAL, thanks! https://codereview.chromium.org/2860193002/diff/340001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/webapps/WebApkInstaller.java (right): https://codereview.chromium.org/2860193002/diff/340001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/webapps/WebApkInstaller.java:79: WebappRegistry.getInstance().register( On 2017/05/18 01:35:14, Yaron wrote: > I honestly can't remember the implications for doing this now - I had thought we > considered doing this at one point but didn't want to register with the registry > too early... Do you remember that concern and/or why it's not valid? For "unknown source", we don't have a good way to know whether the WebAPK is installed unless Chrome listens to all of the package install broadcast. However, it has been changed in Google Play install. Chrome only listens to the broadcast Play sends to itself. So we will know exactly when the WebApk is installed, and thus store the source in the SharedPreference. https://codereview.chromium.org/2860193002/diff/340001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappLauncherActivity.java (right): https://codereview.chromium.org/2860193002/diff/340001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappLauncherActivity.java:103: WebappRegistry.warmUpSharedPrefsForId(webappInfo.id()); On 2017/05/18 01:35:14, Yaron wrote: > i don't think this is buying you anything. you're fetching 2 lines later. WebappRegistry#warmUpSharedPrefsForId() needs to be called first to initialize the storage (read data from the sharedPreference), otherwise WebappRegistery#getWebappDataStorage() will return null. But you remind me that it should be called within try block in line 107 before getWebappDataStorage() is called. Updated.
lgtm https://codereview.chromium.org/2860193002/diff/360001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappLauncherActivity.java (right): https://codereview.chromium.org/2860193002/diff/360001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappLauncherActivity.java:84: // Retrieves the source of the WebAPK from WebappDataStorage if it is unknown. |webappSource| will *not* be unknown in the case of an external intent to a webapk. Otherwise, it's not trustworthy and we must read shared pref to get installation source.
Hi Dom: PTAL, thanks! https://codereview.chromium.org/2860193002/diff/360001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappLauncherActivity.java (right): https://codereview.chromium.org/2860193002/diff/360001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappLauncherActivity.java:84: // Retrieves the source of the WebAPK from WebappDataStorage if it is unknown. On 2017/05/18 17:44:47, Yaron wrote: > |webappSource| will *not* be unknown in the case of an external intent to a > webapk. Otherwise, it's not trustworthy and we must read shared pref to get > installation source. Thanks!
lgtm % nit, thanks for waiting for me. :) https://codereview.chromium.org/2860193002/diff/380001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappLauncherActivity.java (right): https://codereview.chromium.org/2860193002/diff/380001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappLauncherActivity.java:85: // {@link webappSource} will not be unknown in the case of an external intent to a Nit: "will not be unknown in the case of an external intent or a notification that launches a WebAPK". Notifications also explicitly set the source in ServiceTabLauncher.java.
Please hold off for reviewing until I updated. Thanks!
https://codereview.chromium.org/2860193002/diff/380001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappLauncherActivity.java (right): https://codereview.chromium.org/2860193002/diff/380001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappLauncherActivity.java:85: // {@link webappSource} will not be unknown in the case of an external intent to a On 2017/05/22 00:23:13, dominickn wrote: > Nit: "will not be unknown in the case of an external intent or a notification > that launches a WebAPK". > > Notifications also explicitly set the source in ServiceTabLauncher.java. Thanks for catching this. Updated the comments.
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, yfriedman@chromium.org, dominickn@chromium.org Link to the patchset: https://codereview.chromium.org/2860193002/#ps400001 (title: "Nits.")
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
Try jobs failed on following builders: cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...) chromeos_daisy_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_daisy_...) chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...) linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by hanxi@chromium.org
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 hanxi@chromium.org
The CQ bit was checked by hanxi@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from dominickn@chromium.org, pkotwicz@chromium.org, yfriedman@chromium.org Link to the patchset: https://codereview.chromium.org/2860193002/#ps420001 (title: "Rebase.")
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": 420001, "attempt_start_ts": 1495633361256580, "parent_rev": "40f676517d2d8b840c41eb67a7f874a154e7f54a", "commit_rev": "80906b06689bd05c3b727c115242bedd7d6910f2"}
Message was sent while issue was closed.
Description was changed from ========== Correctly record WebAPK install source for Launch.HomescreenSource In this CL: 1) Introduces source WEBAPK_UNKNONW for WebAPKs whose source info are lost. 2) Stores the WebAPK's source info after the install is either succeeded or timed out. 3) Retrives the source info from WebappDataStorage when the WebAPK is launched, and records the launch metrics after the source is avaiable. BUG=711147 ========== to ========== Correctly record WebAPK install source for Launch.HomescreenSource In this CL: 1) Introduces source WEBAPK_UNKNONW for WebAPKs whose source info are lost. 2) Stores the WebAPK's source info after the install is either succeeded or timed out. 3) Retrives the source info from WebappDataStorage when the WebAPK is launched, and records the launch metrics after the source is avaiable. BUG=711147 Review-Url: https://codereview.chromium.org/2860193002 Cr-Commit-Position: refs/heads/master@{#474285} Committed: https://chromium.googlesource.com/chromium/src/+/80906b06689bd05c3b727c115242... ==========
Message was sent while issue was closed.
Committed patchset #10 (id:420001) as https://chromium.googlesource.com/chromium/src/+/80906b06689bd05c3b727c115242... |