Add UMA metric to track the time elapsed since a WebAPK was last launched

Add UMA metric to track the time elapsed since a WebAPK was last launched

This CL also:
Adds a new SharedPreferences property "has_been_launched" to track whether the
WebAPK/Webapp has been launched from the home screen.

BUG=730201
Test=WebApkIntegrationTest.*, WebappModeTest.*

Review-Url: https://codereview.chromium.org/2930553002
Cr-Commit-Position: refs/heads/master@{#483485}
Committed: https://chromium.googlesource.com/chromium/src/+/5c9311ed7789a0591420b9b4b46cab5519651caa

[pkotwicz, 2017-06-06 21:56:25]

pkotwicz@chromium.org changed reviewers:
+ dominickn@chromium.org, hanxi@chromium.org

[pkotwicz, 2017-06-06 21:56:26]

Xi and Dominick can you please take a look?

I added a new timestamp to WebappDataStorage because in the future we might want
to record "last launched" differently than "last used". For instance, we might
want to consider WebAPK launches from a deep link as "launches"

[pkotwicz, 2017-06-06 21:59:52]

Patchset #1 (id:1) has been deleted

[pkotwicz, 2017-06-06 22:11:09]

Patchset #1 (id:20001) has been deleted

[pkotwicz, 2017-06-06 22:11:15]

Patchset #1 (id:40001) has been deleted

[dominickn, 2017-06-07 05:00:54]

https://codereview.chromium.org/2930553002/diff/60001/chrome/android/java/src...
File chrome/android/java/src/org/chromium/chrome/browser/ServiceTabLauncher.java
(right):

https://codereview.chromium.org/2930553002/diff/60001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/ServiceTabLauncher.java:83:
if (storage == null || !storage.wasUsedRecently()
This must use wasLaunchedRecently, not wasUsedRecently.

Otherwise, you can have a situation where:

1. a legacy PWA is opened by the user
2. the user goes back to the homescreen and then deletes the legacy PWA
3. before we do a cleanup, the PWA site sends the user a notification, which the
user opens
4. the notification deep-link opens in a full screen frame because the user
recently used the PWA
5. that opening then updates the used-recently date, which allows the site to
continue opening in a full screen frame from notifications

The heuristic relies on counting only home screen launches so that we know the
legacy PWA shortcut is still there. The way you've changed it means that it will
count all launches as homescreen launches for this heuristic, opening up these
holes.

The "used" in the name of the key currently storing this data is an unfortunate
historical artefact from before the launch heuristic existed.

https://codereview.chromium.org/2930553002/diff/60001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappDataStorage.java
(right):

https://codereview.chromium.org/2930553002/diff/60001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappDataStorage.java:298:
public boolean wasUsedRecently() {
More generally, the only reason this exists is to facilitate:

1) the heuristic for whether we should deep-link from notifications (obviated by
WebAPKs)

2) cleaning up webapp data that hasn't been launched recently (also obviated by
WebAPKs).

We need to make a decision once WebAPKs are launched whether or not legacy PWA
shortcuts will continue to work. All of these existing launch heuristics are
hacks to get around the fact that we don't know if they're still installed or
not.

https://codereview.chromium.org/2930553002/diff/60001/chrome/android/junit/sr...
File
chrome/android/junit/src/org/chromium/chrome/browser/webapps/WebappDataStorageTest.java
(right):

https://codereview.chromium.org/2930553002/diff/60001/chrome/android/junit/sr...
chrome/android/junit/src/org/chromium/chrome/browser/webapps/WebappDataStorageTest.java:180:
}
You should add tests here for the new updateLastUsed(true) case to make sure the
right values are recorded.

[pkotwicz, 2017-06-08 01:14:00]

Dominick, can you please take another look?

https://codereview.chromium.org/2930553002/diff/60001/chrome/android/java/src...
File chrome/android/java/src/org/chromium/chrome/browser/ServiceTabLauncher.java
(right):

https://codereview.chromium.org/2930553002/diff/60001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/ServiceTabLauncher.java:83:
if (storage == null || !storage.wasUsedRecently()
Both wasLaunchedRecently() and wasUsedRecently() are only updated for homescreen
launches. (With the exception of was-used-recently being also updated when the
webapp is registered)

wasUsedRecently() should match exactly with the old behavior of
wasLaunchedRecently()

https://codereview.chromium.org/2930553002/diff/60001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappDataStorage.java
(right):

https://codereview.chromium.org/2930553002/diff/60001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappDataStorage.java:298:
public boolean wasUsedRecently() {
On 2017/06/07 05:00:54, dominickn wrote:
> More generally, the only reason this exists is to facilitate:
> 
> 1) the heuristic for whether we should deep-link from notifications (obviated
by
> WebAPKs)
> 
> 2) cleaning up webapp data that hasn't been launched recently (also obviated
by
> WebAPKs).
> 
> We need to make a decision once WebAPKs are launched whether or not legacy PWA
> shortcuts will continue to work. All of these existing launch heuristics are
> hacks to get around the fact that we don't know if they're still installed or
> not.

Acknowledged.

https://codereview.chromium.org/2930553002/diff/60001/chrome/android/junit/sr...
File
chrome/android/junit/src/org/chromium/chrome/browser/webapps/WebappDataStorageTest.java
(right):

https://codereview.chromium.org/2930553002/diff/60001/chrome/android/junit/sr...
chrome/android/junit/src/org/chromium/chrome/browser/webapps/WebappDataStorageTest.java:180:
}
Added testUpdateLastUsedTime()

[dominickn, 2017-06-08 01:45:48]

https://codereview.chromium.org/2930553002/diff/60001/chrome/android/java/src...
File chrome/android/java/src/org/chromium/chrome/browser/ServiceTabLauncher.java
(right):

https://codereview.chromium.org/2930553002/diff/60001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/ServiceTabLauncher.java:83:
if (storage == null || !storage.wasUsedRecently()
On 2017/06/08 01:14:00, pkotwicz wrote:
> Both wasLaunchedRecently() and wasUsedRecently() are only updated for
homescreen
> launches. (With the exception of was-used-recently being also updated when the
> webapp is registered)
> 
> wasUsedRecently() should match exactly with the old behavior of
> wasLaunchedRecently()

What I mean is that you've changed the semantics of the LAST_USED and
LAST_LAUNCHED keys in a way that could potentially introduce a future bug. As
written, you're correct and the behaviour matches. However, updateLastUsedTime()
now updates the new LAST_LAUNCHED key iff |wasLaunchedFromHomescreen| is true.
That means that the correct value to be using here is now the one in
LAST_LAUNCHED, not in LAST_USED. To my mind, now that we have these two
timestamps, LAST_USED should be updated *any time* the WebAPK is opened (deep
link, notification, etc.), while LAST_LAUNCHED is purely for launches from
homescreen. That means LAST_LAUNCHED is the expendable one because its existence
is only for proving that something is still on the homescreen.

If we add any future calls to updateLastUsedTime() where
|wasLaunchedFromHomescreen| != true, this method automatically becomes incorrect
because LAST_USED will be updated at a time that's not a launch from the
homescreen. So we should preserve its semantics now so that we don't need to fix
it later.

https://codereview.chromium.org/2930553002/diff/80001/chrome/android/junit/sr...
File
chrome/android/junit/src/org/chromium/chrome/browser/webapps/WebappDataStorageTest.java
(right):

https://codereview.chromium.org/2930553002/diff/80001/chrome/android/junit/sr...
chrome/android/junit/src/org/chromium/chrome/browser/webapps/WebappDataStorageTest.java:193:
storage.updateLastUsedTime(false /* wasLaunchedFromHomescreen */);
As your CL is currently written, wasUsedRecently will now return true at this
point. But that's the incorrect semantics for the ServiceTabLauncher, which
should still regard the webapp as not having been launched from homescreen. You
should test this.

[pkotwicz, 2017-06-08 04:34:28]

Dominick, can you please take another look?

I have added a boolean SharedPreference "was_launched" in WebappDataStorage to
indicate whether the webapp / WebAPK has ever been launched from the home
screen. I think that this is one of the approaches that you mentioned.

I will update the CL description once you are happy with my approach.

[dominickn, 2017-06-08 05:15:27]

https://codereview.chromium.org/2930553002/diff/100001/chrome/android/java/sr...
File
chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappActivity.java
(right):

https://codereview.chromium.org/2930553002/diff/100001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappActivity.java:408:
onWillUpdateLastUsedTime(storage);
It would be good if you could add a quick test that after installing a webapp,
wasLaunched() is false, and then after launch, it's true.

https://codereview.chromium.org/2930553002/diff/100001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappActivity.java:409:
storage.setWasLaunched();
I think you need to setWasLaunched() before onWillUpdateLastUsedTime()

https://codereview.chromium.org/2930553002/diff/100001/chrome/android/java/sr...
File
chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappDataStorage.java
(right):

https://codereview.chromium.org/2930553002/diff/100001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappDataStorage.java:36:
static final String KEY_WAS_LAUNCHED = "was_launched";
I'd call this "HAS_BEEN_LAUNCHED"

https://codereview.chromium.org/2930553002/diff/100001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappDataStorage.java:311:
public boolean wasUsedRecently() {
I'd prefer that this method kept the same name. The additional comment really
isn't necessary because all of that information is already present (you could if
you wanted to move the comment within the method up to the Javadoc above).

https://codereview.chromium.org/2930553002/diff/100001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappDataStorage.java:396:
boolean wasLaunched() {
Call this "hasBeenLaunched()", and update the comment to say "Returns true if
the web app has been launched at least once from the home screen".

https://codereview.chromium.org/2930553002/diff/100001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappDataStorage.java:401:
void setWasLaunched() {
setHasBeenLaunched(). Ditto with the comment.

[Xi Han, 2017-06-09 14:55:35]

https://codereview.chromium.org/2930553002/diff/100001/chrome/android/java/sr...
File
chrome/android/java/src/org/chromium/chrome/browser/webapps/WebApkActivity.java
(right):

https://codereview.chromium.org/2930553002/diff/100001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/webapps/WebApkActivity.java:191:
WebApkUma.recordLaunchInterval(lastUsedMs - storage.getLastUsedTime());
I am confused here. Do you expect the |LastUsedTime| be changed between calling
of line 189 and line 191? Looks like it will record 0 to the launch interval
metrics.

https://codereview.chromium.org/2930553002/diff/100001/chrome/android/java/sr...
File
chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappDataStorage.java
(right):

https://codereview.chromium.org/2930553002/diff/100001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappDataStorage.java:305:
/**
Please merge the comments above with the ones below.

[pkotwicz, 2017-06-12 20:33:28]

Patchset #4 (id:120001) has been deleted

[pkotwicz, 2017-06-12 20:34:46]

Dominick and Xi can you please take another look?

https://codereview.chromium.org/2930553002/diff/100001/chrome/android/java/sr...
File
chrome/android/java/src/org/chromium/chrome/browser/webapps/WebApkActivity.java
(right):

https://codereview.chromium.org/2930553002/diff/100001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/webapps/WebApkActivity.java:191:
WebApkUma.recordLaunchInterval(lastUsedMs - storage.getLastUsedTime());
Thank you for catching this bug!

https://codereview.chromium.org/2930553002/diff/100001/chrome/android/java/sr...
File
chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappActivity.java
(right):

https://codereview.chromium.org/2930553002/diff/100001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappActivity.java:408:
onWillUpdateLastUsedTime(storage);
How do you suggest that I add such a test? I don't think that I can write the
test that you want as a JUnit test. I have already added two integration tests
as part of this CL

I could test that registering a webapp does not cause WebappDataStorage to think
that the webapp has been launched.

https://codereview.chromium.org/2930553002/diff/100001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappActivity.java:409:
storage.setWasLaunched();
Actually, it is important for the WebApkActivity method to be run before
WebappDataStorage#setHasBeenLaunched() is called because WebApkActivity needs to
know whether the lauch is the first launch.

I have changed around the code. Hopefully, it is clearer now

https://codereview.chromium.org/2930553002/diff/100001/chrome/android/java/sr...
File
chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappDataStorage.java
(right):

https://codereview.chromium.org/2930553002/diff/100001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappDataStorage.java:311:
public boolean wasUsedRecently() {
I think that having *used* in the name of the function makes sense given that
this function checks last_used and the timestamp is updated via
updateLastUsedTime()

I have merged the two comments together

[dominickn, 2017-06-14 00:19:39]

Looks pretty good

https://codereview.chromium.org/2930553002/diff/100001/chrome/android/java/sr...
File
chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappActivity.java
(right):

https://codereview.chromium.org/2930553002/diff/100001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappActivity.java:408:
onWillUpdateLastUsedTime(storage);
On 2017/06/12 20:34:46, pkotwicz wrote:
> How do you suggest that I add such a test? I don't think that I can write the
> test that you want as a JUnit test. I have already added two integration tests
> as part of this CL
> 
> I could test that registering a webapp does not cause WebappDataStorage to
think
> that the webapp has been launched.

Ah yes, the integration test is sufficient with another assert. Thanks

https://codereview.chromium.org/2930553002/diff/140001/chrome/android/javates...
File
chrome/android/javatests/src/org/chromium/chrome/browser/webapps/WebApkIntegrationTest.java
(right):

https://codereview.chromium.org/2930553002/diff/140001/chrome/android/javates...
chrome/android/javatests/src/org/chromium/chrome/browser/webapps/WebApkIntegrationTest.java:164:
Assert.assertNotEquals(WebappDataStorage.TIMESTAMP_INVALID,
storage.getLastLaunchedTime());
Can you add Assert.assertTrue(storage.hasBeenLaunched()); (and perhaps the
opposite before starting the activity?)

[pkotwicz, 2017-06-14 02:16:19]

Dominick, can you please take another look? I have added
testSetsHasBeenLaunchedOnFirstLaunch in WebappModeTest.java

[dominickn, 2017-06-14 02:58:24]

lgtm

In future, please upload rebases separately to make it easier to look at
patch-to-patch deltas.

[Xi Han, 2017-06-14 14:51:06]

lgtm

https://codereview.chromium.org/2930553002/diff/160001/base/android/record_hi...
File base/android/record_histogram.cc (right):

https://codereview.chromium.org/2930553002/diff/160001/base/android/record_hi...
base/android/record_histogram.cc:330: return samples->TotalCount();
Please remove |samples| and return histogram->SnapshotSamples()->TotalCount().

[pkotwicz, 2017-06-15 02:33:37]

pkotwicz@chromium.org changed reviewers:
+ isherman@chromium.org

[pkotwicz, 2017-06-15 02:33:38]

isherman@ for record_histogram.cc, RecordHistogram.java and histograms.xml

[pkotwicz, 2017-06-15 02:35:22]

Description was changed from

==========
Add UMA metric to track the time elapsed since a WebAPK was last launched

This CL also:
- Adds a new timestamp ("last_launched") to SharedPreferences to track when the
  WebAPK/Webapp was last launched. The timestamp is different from "last_used"
  because the "last_launched" timestamp, unlike the "last_used" timestamp, is
  not updated when the WebAPK/Webapp is installed.
- Merges WebappDataStorage#LAST_USED_UNSET and
  WebappDataStorage#LAST_USED_INVALID.
- Renames WebappDataStorage#LAST_USED_INVALID to
  WebappDataStorage#TIMESTAMP_INVALID.

BUG=730201
Test=WebApkIntegrationTest.*
==========

to

==========
Add UMA metric to track the time elapsed since a WebAPK was last launched

This CL also:
Adds a new SharedPreferences property "has_been_launched" to track whether the
WebAPK/Webapp has been launched from the home screen.

BUG=730201
Test=WebApkIntegrationTest.*, WebappModeTest.*
==========

[Ilya Sherman, 2017-06-15 03:16:52]

Metrics LGTM % a comment:

https://codereview.chromium.org/2930553002/diff/180001/tools/metrics/histogra...
File tools/metrics/histograms/histograms.xml (right):

https://codereview.chromium.org/2930553002/diff/180001/tools/metrics/histogra...
tools/metrics/histograms/histograms.xml:81727: +    homescreen.
Please document what is recorded on the first launch -- 0? some large value?
nothing (recording is skipped)?

[pkotwicz, 2017-06-23 20:16:09]

pkotwicz@chromium.org changed reviewers:
+ yfriedman@chromium.org

[pkotwicz, 2017-06-23 20:16:10]

Yaron for base/android changes

[Yaron, 2017-06-23 21:17:22]

lgtm

[pkotwicz, 2017-06-27 18:09:24]

The CQ bit was checked by pkotwicz@chromium.org

[pkotwicz, 2017-06-27 18:09:25]

The patchset sent to the CQ was uploaded after l-g-t-m from
dominickn@chromium.org, hanxi@chromium.org, isherman@chromium.org,
yfriedman@chromium.org
Link to the patchset: https://codereview.chromium.org/2930553002/#ps220001
(title: "Merge branch 'master' into last_launched2")

[commit-bot: I haz the power, 2017-06-27 18:09:45]

CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-06-27 19:29:31]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-06-27 19:29:33]

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...)

[pkotwicz, 2017-06-28 22:32:38]

The CQ bit was checked by pkotwicz@chromium.org

[pkotwicz, 2017-06-28 22:32:39]

The patchset sent to the CQ was uploaded after l-g-t-m from
dominickn@chromium.org, hanxi@chromium.org, yfriedman@chromium.org,
isherman@chromium.org
Link to the patchset: https://codereview.chromium.org/2930553002/#ps240001
(title: "Merge branch 'master' into last_launched2")

[commit-bot: I haz the power, 2017-06-28 22:33:05]

CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-06-29 01:00:47]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-06-29 01:00:49]

Try jobs failed on following builders:
  android_clang_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_clan...)

[pkotwicz, 2017-06-29 19:47:18]

The CQ bit was checked by pkotwicz@chromium.org

[pkotwicz, 2017-06-29 19:47:19]

The patchset sent to the CQ was uploaded after l-g-t-m from
dominickn@chromium.org, hanxi@chromium.org, yfriedman@chromium.org,
isherman@chromium.org
Link to the patchset: https://codereview.chromium.org/2930553002/#ps260001
(title: "Merge branch 'master' into last_launched2")

[commit-bot: I haz the power, 2017-06-29 19:47:39]

CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-06-29 21:08:57]

CQ is committing da patch.

Bot data: {"patchset_id": 260001, "attempt_start_ts": 1498765638338400,
"parent_rev": "fd94c00a57085279ce96fc861e41bee1eae2fcac", "commit_rev":
"5c9311ed7789a0591420b9b4b46cab5519651caa"}

[commit-bot: I haz the power, 2017-06-29 21:09:12]

Description was changed from

==========
Add UMA metric to track the time elapsed since a WebAPK was last launched

This CL also:
Adds a new SharedPreferences property "has_been_launched" to track whether the
WebAPK/Webapp has been launched from the home screen.

BUG=730201
Test=WebApkIntegrationTest.*, WebappModeTest.*
==========

to

==========
Add UMA metric to track the time elapsed since a WebAPK was last launched

This CL also:
Adds a new SharedPreferences property "has_been_launched" to track whether the
WebAPK/Webapp has been launched from the home screen.

BUG=730201
Test=WebApkIntegrationTest.*, WebappModeTest.*

Review-Url: https://codereview.chromium.org/2930553002
Cr-Commit-Position: refs/heads/master@{#483485}
Committed:
https://chromium.googlesource.com/chromium/src/+/5c9311ed7789a0591420b9b4b46c...
==========

[commit-bot: I haz the power, 2017-06-29 21:09:15]

Committed patchset #10 (id:260001) as
https://chromium.googlesource.com/chromium/src/+/5c9311ed7789a0591420b9b4b46c...