Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(303)

Issue 2860193002: Correctly record WebAPK install source for Launch.HomescreenSource (Closed)

Created:
3 years, 7 months ago by Xi Han
Modified:
3 years, 7 months ago
Reviewers:
dominickn, pkotwicz, Yaron
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.

Description

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/+/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. #

Unified diffs Side-by-side diffs Delta from patch set Stats (+76 lines, -14 lines) Patch
M chrome/android/java/src/org/chromium/chrome/browser/webapps/WebApkActivity.java View 1 2 3 4 5 1 chunk +2 lines, -3 lines 0 comments Download
M chrome/android/java/src/org/chromium/chrome/browser/webapps/WebApkInstaller.java View 1 3 chunks +15 lines, -2 lines 0 comments Download
M chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappDataStorage.java View 1 2 3 4 5 3 chunks +21 lines, -6 lines 0 comments Download
M chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappLauncherActivity.java View 1 2 3 4 5 6 7 8 9 3 chunks +31 lines, -1 line 0 comments Download
M chrome/browser/android/shortcut_info.h View 1 3 1 chunk +5 lines, -1 line 0 comments Download
M chrome/browser/android/webapk/webapk_installer.cc View 1 2 3 4 5 6 7 8 9 1 chunk +1 line, -1 line 0 comments Download
M tools/metrics/histograms/enums.xml View 1 2 3 4 5 6 7 8 9 1 chunk +1 line, -0 lines 0 comments Download

Messages

Total messages: 54 (29 generated)
Xi Han
Hi Peter, could you please take a look? Thanks!
3 years, 7 months ago (2017-05-04 20:04:08 UTC) #4
pkotwicz
https://codereview.chromium.org/2860193002/diff/20001/chrome/android/java/src/org/chromium/chrome/browser/webapps/WebApkActivity.java File chrome/android/java/src/org/chromium/chrome/browser/webapps/WebApkActivity.java (right): https://codereview.chromium.org/2860193002/diff/20001/chrome/android/java/src/org/chromium/chrome/browser/webapps/WebApkActivity.java#newcode186 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/org/chromium/chrome/browser/webapps/WebappDataStorage.java File chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappDataStorage.java (right): ...
3 years, 7 months ago (2017-05-05 21:05:43 UTC) #5
Xi Han
Hi Peter, PTAL, thanks! https://codereview.chromium.org/2860193002/diff/20001/chrome/android/java/src/org/chromium/chrome/browser/webapps/WebApkActivity.java File chrome/android/java/src/org/chromium/chrome/browser/webapps/WebApkActivity.java (right): https://codereview.chromium.org/2860193002/diff/20001/chrome/android/java/src/org/chromium/chrome/browser/webapps/WebApkActivity.java#newcode186 chrome/android/java/src/org/chromium/chrome/browser/webapps/WebApkActivity.java:186: storage.updateTimeOfLastCheckForUpdatedWebManifest(); On 2017/05/05 21:05:43, pkotwicz ...
3 years, 7 months ago (2017-05-08 20:55:28 UTC) #8
pkotwicz
CCing Dominick as he likely has wisdom to share (and might find flaws in my ...
3 years, 7 months ago (2017-05-09 18:37:36 UTC) #10
dominickn
I'll need to think about this for a bit - it's really unfortunate that we ...
3 years, 7 months ago (2017-05-10 06:04:46 UTC) #11
Xi Han
Thank you both. I have remove the WebAPK's special way to record launch metrics. PTAL, ...
3 years, 7 months ago (2017-05-10 20:49:58 UTC) #17
pkotwicz
https://codereview.chromium.org/2860193002/diff/80001/chrome/android/java/src/org/chromium/chrome/browser/webapps/WebApkActivity.java File chrome/android/java/src/org/chromium/chrome/browser/webapps/WebApkActivity.java (right): https://codereview.chromium.org/2860193002/diff/80001/chrome/android/java/src/org/chromium/chrome/browser/webapps/WebApkActivity.java#newcode147 chrome/android/java/src/org/chromium/chrome/browser/webapps/WebApkActivity.java:147: } I wasn't suggesting changing the default source in ...
3 years, 7 months ago (2017-05-10 21:08:59 UTC) #18
dominickn
We should only use WEBAPK_UNKNOWN as the source for WebAPKs, not as the universal default. ...
3 years, 7 months ago (2017-05-11 03:35:49 UTC) #19
Xi Han
I reverted the default source of Webapp to ShortcutSouce.UNKNOWN. PTAL, thanks! https://codereview.chromium.org/2860193002/diff/80001/chrome/android/java/src/org/chromium/chrome/browser/webapps/WebApkActivity.java File chrome/android/java/src/org/chromium/chrome/browser/webapps/WebApkActivity.java (right): ...
3 years, 7 months ago (2017-05-11 17:14:28 UTC) #25
pkotwicz
Just nits! Shouldn't you also need to update ShortcutSource.UNKNOWN in ShortcutHelper#retrieveWebApks() https://codereview.chromium.org/2860193002/diff/280001/chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappLauncherActivity.java File chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappLauncherActivity.java (right): ...
3 years, 7 months ago (2017-05-11 18:10:34 UTC) #28
Xi Han
PTAL, thanks! https://codereview.chromium.org/2860193002/diff/280001/chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappLauncherActivity.java File chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappLauncherActivity.java (right): https://codereview.chromium.org/2860193002/diff/280001/chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappLauncherActivity.java#newcode83 chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappLauncherActivity.java:83: // Retrieves the source of the WebAPK ...
3 years, 7 months ago (2017-05-15 14:50:32 UTC) #30
Xi Han
PTAL, thanks!
3 years, 7 months ago (2017-05-15 18:16:54 UTC) #31
pkotwicz
Yaron for OWNERS LGTM. Sorry for the delay.
3 years, 7 months ago (2017-05-17 21:24:59 UTC) #33
Yaron
https://codereview.chromium.org/2860193002/diff/340001/chrome/android/java/src/org/chromium/chrome/browser/webapps/WebApkInstaller.java File chrome/android/java/src/org/chromium/chrome/browser/webapps/WebApkInstaller.java (right): https://codereview.chromium.org/2860193002/diff/340001/chrome/android/java/src/org/chromium/chrome/browser/webapps/WebApkInstaller.java#newcode79 chrome/android/java/src/org/chromium/chrome/browser/webapps/WebApkInstaller.java:79: WebappRegistry.getInstance().register( I honestly can't remember the implications for doing ...
3 years, 7 months ago (2017-05-18 01:35:14 UTC) #34
Xi Han
PTAL, thanks! https://codereview.chromium.org/2860193002/diff/340001/chrome/android/java/src/org/chromium/chrome/browser/webapps/WebApkInstaller.java File chrome/android/java/src/org/chromium/chrome/browser/webapps/WebApkInstaller.java (right): https://codereview.chromium.org/2860193002/diff/340001/chrome/android/java/src/org/chromium/chrome/browser/webapps/WebApkInstaller.java#newcode79 chrome/android/java/src/org/chromium/chrome/browser/webapps/WebApkInstaller.java:79: WebappRegistry.getInstance().register( On 2017/05/18 01:35:14, Yaron wrote: > ...
3 years, 7 months ago (2017-05-18 14:47:44 UTC) #35
Yaron
lgtm https://codereview.chromium.org/2860193002/diff/360001/chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappLauncherActivity.java File chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappLauncherActivity.java (right): https://codereview.chromium.org/2860193002/diff/360001/chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappLauncherActivity.java#newcode84 chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappLauncherActivity.java:84: // Retrieves the source of the WebAPK from ...
3 years, 7 months ago (2017-05-18 17:44:47 UTC) #36
Xi Han
Hi Dom: PTAL, thanks! https://codereview.chromium.org/2860193002/diff/360001/chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappLauncherActivity.java File chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappLauncherActivity.java (right): https://codereview.chromium.org/2860193002/diff/360001/chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappLauncherActivity.java#newcode84 chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappLauncherActivity.java:84: // Retrieves the source of ...
3 years, 7 months ago (2017-05-18 18:27:59 UTC) #37
dominickn
lgtm % nit, thanks for waiting for me. :) https://codereview.chromium.org/2860193002/diff/380001/chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappLauncherActivity.java File chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappLauncherActivity.java (right): https://codereview.chromium.org/2860193002/diff/380001/chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappLauncherActivity.java#newcode85 chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappLauncherActivity.java:85: ...
3 years, 7 months ago (2017-05-22 00:23:13 UTC) #38
Xi Han
Please hold off for reviewing until I updated. Thanks!
3 years, 7 months ago (2017-05-23 19:13:58 UTC) #39
Xi Han
https://codereview.chromium.org/2860193002/diff/380001/chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappLauncherActivity.java File chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappLauncherActivity.java (right): https://codereview.chromium.org/2860193002/diff/380001/chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappLauncherActivity.java#newcode85 chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappLauncherActivity.java:85: // {@link webappSource} will not be unknown in the ...
3 years, 7 months ago (2017-05-23 19:18:08 UTC) #40
commit-bot: I haz the power
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2860193002/400001
3 years, 7 months ago (2017-05-23 19:19:06 UTC) #43
commit-bot: I haz the power
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_linux/builds/374221) chromeos_daisy_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_FAILED, ...
3 years, 7 months ago (2017-05-23 19:26:44 UTC) #45
commit-bot: I haz the power
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2860193002/400001
3 years, 7 months ago (2017-05-24 13:40:20 UTC) #47
commit-bot: I haz the power
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2860193002/420001
3 years, 7 months ago (2017-05-24 13:42:50 UTC) #51
commit-bot: I haz the power
3 years, 7 months ago (2017-05-24 14:38:37 UTC) #54
Message was sent while issue was closed.
Committed patchset #10 (id:420001) as
https://chromium.googlesource.com/chromium/src/+/80906b06689bd05c3b727c115242...

Powered by Google App Engine
This is Rietveld 408576698