Store URLs in WebappDataStorage, and purge them when history is cleared.

Store URLs in WebappDataStorage, and purge them when history is cleared.

Sites which have been added to homescreen on Android should be
allowed to open in standalone mode in response to push notifications. As
Chromium cannot detect what sites are on the homescreen after they
have been added, the heuristic is sites which have been launched from
homescreen within the last ten days will be opened in standalone from
a tap on a notification.

This CL lays the groundwork for this feature by storing webapp URLs
in WebappDataStorage. This is needed to be able to look up the last used
time, which is currently persisted in WebappDataStorage.

The last used time and webapp URL combined are history, so this
CL also resets both pieces of data when the user clears their history.
This requires further changes to allow null values in these fields. The
last used time is set appropriately when the app is next launched from
homescreen; the URL is reset if it is empty when the app is launched.
Additional tests for the behaviour introduced in this CL are added.

BUG=541711, 570579
Committed: https://crrev.com/6e400421b116644b4e8b23c29fd2e0c3886af3af
Cr-Commit-Position: refs/heads/master@{#383223}

[dominickn, 2016-02-29 07:49:14]

dominickn@chromium.org changed reviewers:
+ dfalcantara@chromium.org

[dominickn, 2016-02-29 07:49:15]

PTAL. This ended up being a pretty large patch.

1. Sites which have launched from homescreen in the last 10 days are permitted
to launch into standalone mode from a notification

2. This means we need to persist the origin in WebappDataStorage, where the last
launch time is also stored

3. The origin and last launch time are privacy sensitive, so they need to be
cleared when the user clears their history

4. Tests for things

[dominickn, 2016-02-29 10:55:42]

Description was changed from

==========
Store origins in WebappDataStorage, and purge the data when history is cleared.

Sites which have been added to homescreen should be allowed to open in
standalone mode in response to push notifications. As Chromium cannot
detect what sites are on the homescreen after they have been added, the
heuristic to allow this is sites which have been launched from
homescreen within the last ten days will be opened in standalone from
a notification.

This CL lays the groundwork for this feature by storing webapp origins
in WebappDataStorage. This is needed to be able to look up the last used
time, which is currently persisted in WebappDataStorage.

The last used time and webapp origin are both forms of history, so this
CL also resets both pieces of data when the user clears their history.
This requires further changes to allow null values in these fields. The
last used time is set appropriately when the app is next launched from
homescreen; the origin URL is set *every* time the app is launched, as
a lookup of the value is already an asynchronous operation. Additional
tests for the behaviour introduced in this CL are added.

BUG=541711,570579
==========

to

==========
Store origins in WebappDataStorage, and purge them when history is cleared.

Sites which have been added to homescreen on Android should be
allowed to open in standalone mode in response to push notifications. As
Chromium cannot detect what sites are on the homescreen after they
have been added, the heuristic is sites which have been launched from
homescreen within the last ten days will be opened in standalone from
a tap on a notification.

This CL lays the groundwork for this feature by storing webapp origins
in WebappDataStorage. This is needed to be able to look up the last used
time, which is currently persisted in WebappDataStorage.

The last used time and webapp origin are both forms of history, so this
CL also resets both pieces of data when the user clears their history.
This requires further changes to allow null values in these fields. The
last used time is set appropriately when the app is next launched from
homescreen; the origin URL is set *every* time the app is launched, as
a lookup of the value is already an asynchronous operation. Additional
tests for the behaviour introduced in this CL are added.

BUG=541711,570579
==========

[dominickn, 2016-03-01 01:08:27]

On 2016/02/29 07:49:15, dominickn wrote:
> PTAL. This ended up being a pretty large patch.
> 
> 1. Sites which have launched from homescreen in the last 10 days are permitted
> to launch into standalone mode from a notification
> 
> 2. This means we need to persist the origin in WebappDataStorage, where the
last
> launch time is also stored
> 
> 3. The origin and last launch time are privacy sensitive, so they need to be
> cleared when the user clears their history
> 
> 4. Tests for things

Follow-up: I'm not sure why the unit tests and instrumentation tests are dying
on
linx_android_rel_ng. Everything so far passes locally for me; one of the bot
failures is a NoSuchMethodError, which I think indicates some sort of compile
time versus runtime classpath issue?

[dominickn, 2016-03-02 03:11:39]

dominickn@chromium.org changed reviewers:
+ mlamouri@chromium.org

[dominickn, 2016-03-02 03:12:49]

+mlamouri: FYI, PTAL.

Update on test failures: I can reproduce the failure by building on GYP.
My local build is GN, which happily passes the tests without the
NoSuchMethodError. This is beyond me now, so I'll ask on clank-dev.

[dominickn, 2016-03-08 04:48:57]

Update: Fixed the NoSuchMethodError. Now the browsing data remover unit tests
are timing out.
The timeout happens at the JNI call introduced in this CL to clear the webapp
history
(URL/last launched date) when clearing browsing history.

Are C++ unit tests allowed to make JNI calls?

[gone, 2016-03-08 19:25:56]

On 2016/03/08 04:48:57, dominickn wrote:
> Update: Fixed the NoSuchMethodError. Now the browsing data remover unit tests
> are timing out.
> The timeout happens at the JNI call introduced in this CL to clear the webapp
> history
> (URL/last launched date) when clearing browsing history.
> 
> Are C++ unit tests allowed to make JNI calls?

I don't think that Java-side code is hooked in when C++ tests are run; that'd
require Android Instrumentation.

[gone, 2016-03-08 22:40:39]

Still going through the tests, but here's an initial set.

https://chromiumcodereview.appspot.com/1749603002/diff/1/chrome/android/java/...
File
chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappDataStorage.java
(right):

https://chromiumcodereview.appspot.com/1749603002/diff/1/chrome/android/java/...
chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappDataStorage.java:38:
static final long NULL_LAST_USED = 0;
It's kind of weird to have both an INVALID and a NULL.  Maybe these could be
renamed like this?

LAST_USED_UNSET = 0;
LAST_USED_INVALID = -1

https://chromiumcodereview.appspot.com/1749603002/diff/1/chrome/android/java/...
chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappDataStorage.java:154:
.edit().putLong(KEY_LAST_USED, NULL_LAST_USED).remove(KEY_ORIGIN_URL).apply();
nit: indent by 8

https://chromiumcodereview.appspot.com/1749603002/diff/1/chrome/browser/andro...
File chrome/browser/android/webapps/webapp_registry.cc (right):

https://chromiumcodereview.appspot.com/1749603002/diff/1/chrome/browser/andro...
chrome/browser/android/webapps/webapp_registry.cc:52: const
JavaParamRef<jclass>& klass,
nit: indentation; common convention is to use clazz

https://chromiumcodereview.appspot.com/1749603002/diff/20001/chrome/android/j...
File
chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappDataStorage.java
(right):

https://chromiumcodereview.appspot.com/1749603002/diff/20001/chrome/android/j...
chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappDataStorage.java:56:
// The last used time may be invalid if the user has cleared their history.
Given that you're using two different values for INVALID and NULL, can you put
the assert back?

https://chromiumcodereview.appspot.com/1749603002/diff/20001/chrome/android/j...
chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappDataStorage.java:97:
@VisibleForTesting
Venting, no action items here:
For the record, this Class makes me weep whenever I see it.  It's an
over-engineered mess for the one (now two) things it does.

See my comments on
https://chromiumcodereview.appspot.com/1286973003/diff/100001/chrome/android/...

https://chromiumcodereview.appspot.com/1749603002/diff/20001/chrome/android/j...
chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappDataStorage.java:156:
.edit().putLong(KEY_LAST_USED, NULL_LAST_USED).remove(KEY_ORIGIN_URL).apply();
nit: indent by 8

https://chromiumcodereview.appspot.com/1749603002/diff/20001/chrome/android/j...
chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappDataStorage.java:194:
void updateOriginUrl(String originUrl) {
Add javadocs to these

https://chromiumcodereview.appspot.com/1749603002/diff/20001/chrome/android/j...
File
chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappRegistry.java
(right):

https://chromiumcodereview.appspot.com/1749603002/diff/20001/chrome/android/j...
chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappRegistry.java:64:
// WebappDataStorage.INVALID_LAST_USED) if a web app appears in the registry.
nit: Rework this comment so that i.e. != WebappDataStorage.INVALID_LAST_USED
fits on the same line.  It's hard to parse.

https://chromiumcodereview.appspot.com/1749603002/diff/20001/chrome/android/j...
chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappRegistry.java:97:
* used time set to 0 by the user clearing their history. Cleanup is run, at
most, once a month.
nit: newline between this and @param

https://chromiumcodereview.appspot.com/1749603002/diff/20001/chrome/android/j...
chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappRegistry.java:180:
if (callback == null) return;
Is this callback == null check here only for tests?  Seems like it's the kind of
thing you should assert never happens, then change the test to pass in a
runnable that checks if it gets called.

https://chromiumcodereview.appspot.com/1749603002/diff/20001/chrome/android/j...
chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappRegistry.java:187:
static void clearWebappHistory(Context context, final long callbackPointer) {
If this is only called by native code, use private static void.

https://chromiumcodereview.appspot.com/1749603002/diff/20001/chrome/browser/b...
File chrome/browser/browsing_data/browsing_data_remover.cc (right):

https://chromiumcodereview.appspot.com/1749603002/diff/20001/chrome/browser/b...
chrome/browser/browsing_data/browsing_data_remover.cc:562:
waiting_for_clear_webapp_history_ = true;
The other clearing functions generally seem to PostTaskAndReply; is that not
required in this case because of the AsyncTask in Java land?  If so, it'd be
worth commenting on that here.

[gone, 2016-03-08 23:03:52]

https://chromiumcodereview.appspot.com/1749603002/diff/20001/chrome/android/j...
File
chrome/android/javatests/src/org/chromium/chrome/browser/preferences/privacy/ClearBrowsingDataPreferencesTest.java
(right):

https://chromiumcodereview.appspot.com/1749603002/diff/20001/chrome/android/j...
chrome/android/javatests/src/org/chromium/chrome/browser/preferences/privacy/ClearBrowsingDataPreferencesTest.java:148:
// The webapp should still exist in the registry.
web app?

https://chromiumcodereview.appspot.com/1749603002/diff/20001/chrome/android/j...
chrome/android/javatests/src/org/chromium/chrome/browser/preferences/privacy/ClearBrowsingDataPreferencesTest.java:165:
WebappDataStorage.getOriginUrl(getActivity(), "first",
Indenting here seems like it's kind of all over the place

https://chromiumcodereview.appspot.com/1749603002/diff/20001/chrome/android/j...
File
chrome/android/junit/src/org/chromium/chrome/browser/webapps/WebappDataStorageTest.java
(right):

https://chromiumcodereview.appspot.com/1749603002/diff/20001/chrome/android/j...
chrome/android/junit/src/org/chromium/chrome/browser/webapps/WebappDataStorageTest.java:156:
WebappDataStorage.getOriginUrl(Robolectric.application, "test",
Maybe it'd make sense to create and declare the FetchCallback outside of the
function invocation.  It'd help with readability.

https://chromiumcodereview.appspot.com/1749603002/diff/20001/chrome/android/j...
File
chrome/android/junit/src/org/chromium/chrome/browser/webapps/WebappRegistryTest.java
(right):

https://chromiumcodereview.appspot.com/1749603002/diff/20001/chrome/android/j...
chrome/android/junit/src/org/chromium/chrome/browser/webapps/WebappRegistryTest.java:327:
assertTrue(actual.contains("webapp1"));
Should this check for equality with TextUtils.equals() instead of checking for
contains()?

[AlvarezD.AA11, 2016-03-08 23:07:35]

alvarezd.aa11@gmail.com changed reviewers:
+ AlvarezD.AA11@gmail.com

[dominickn, 2016-03-09 08:18:31]

dominickn@chromium.org changed reviewers:
- AlvarezD.AA11@gmail.com

[dominickn, 2016-03-09 08:18:33]

PTAL, thanks!

This CL now depends on http://crrev.com/1776903002, which refactors
WebappRegistry so it's mockable in unit tests.

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

https://codereview.chromium.org/1749603002/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappDataStorage.java:97:
@VisibleForTesting
On 2016/03/08 22:40:39, dfalcantara wrote:
> Venting, no action items here:
> For the record, this Class makes me weep whenever I see it.  It's an
> over-engineered mess for the one (now two) things it does.
> 
> See my comments on
>
https://chromiumcodereview.appspot.com/1286973003/diff/100001/chrome/android/...

Definitely, definitely agreed.

https://codereview.chromium.org/1749603002/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappDataStorage.java:156:
.edit().putLong(KEY_LAST_USED, NULL_LAST_USED).remove(KEY_ORIGIN_URL).apply();
On 2016/03/08 22:40:39, dfalcantara wrote:
> nit: indent by 8

Done.

https://codereview.chromium.org/1749603002/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappDataStorage.java:194:
void updateOriginUrl(String originUrl) {
On 2016/03/08 22:40:39, dfalcantara wrote:
> Add javadocs to these

Done.

https://codereview.chromium.org/1749603002/diff/20001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappRegistry.java
(right):

https://codereview.chromium.org/1749603002/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappRegistry.java:64:
// WebappDataStorage.INVALID_LAST_USED) if a web app appears in the registry.
On 2016/03/08 22:40:39, dfalcantara wrote:
> nit: Rework this comment so that i.e. != WebappDataStorage.INVALID_LAST_USED
> fits on the same line.  It's hard to parse.

Done.

https://codereview.chromium.org/1749603002/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappRegistry.java:97:
* used time set to 0 by the user clearing their history. Cleanup is run, at
most, once a month.
On 2016/03/08 22:40:39, dfalcantara wrote:
> nit: newline between this and @param

Done.

https://codereview.chromium.org/1749603002/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappRegistry.java:180:
if (callback == null) return;
On 2016/03/08 22:40:39, dfalcantara wrote:
> Is this callback == null check here only for tests?  Seems like it's the kind
of
> thing you should assert never happens, then change the test to pass in a
> runnable that checks if it gets called.

I just copied from the block above this one. I'll refactor both.

https://codereview.chromium.org/1749603002/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappRegistry.java:187:
static void clearWebappHistory(Context context, final long callbackPointer) {
On 2016/03/08 22:40:39, dfalcantara wrote:
> If this is only called by native code, use private static void.

I did this in patchset #3, but it fails to compile on the
android_clang_dbg_recipe bot with an "uncalled private method" error from
findbugs. I think I'll need to remove the private?

https://codereview.chromium.org/1749603002/diff/20001/chrome/android/javatest...
File
chrome/android/javatests/src/org/chromium/chrome/browser/preferences/privacy/ClearBrowsingDataPreferencesTest.java
(right):

https://codereview.chromium.org/1749603002/diff/20001/chrome/android/javatest...
chrome/android/javatests/src/org/chromium/chrome/browser/preferences/privacy/ClearBrowsingDataPreferencesTest.java:148:
// The webapp should still exist in the registry.
On 2016/03/08 23:03:52, dfalcantara wrote:
> web app?

Done.

https://codereview.chromium.org/1749603002/diff/20001/chrome/android/javatest...
chrome/android/javatests/src/org/chromium/chrome/browser/preferences/privacy/ClearBrowsingDataPreferencesTest.java:165:
WebappDataStorage.getOriginUrl(getActivity(), "first",
On 2016/03/08 23:03:52, dfalcantara wrote:
> Indenting here seems like it's kind of all over the place

Indenting Java I don't know how

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

https://codereview.chromium.org/1749603002/diff/20001/chrome/android/junit/sr...
chrome/android/junit/src/org/chromium/chrome/browser/webapps/WebappDataStorageTest.java:156:
WebappDataStorage.getOriginUrl(Robolectric.application, "test",
On 2016/03/08 23:03:52, dfalcantara wrote:
> Maybe it'd make sense to create and declare the FetchCallback outside of the
> function invocation.  It'd help with readability.

Done.

https://codereview.chromium.org/1749603002/diff/20001/chrome/android/junit/sr...
File
chrome/android/junit/src/org/chromium/chrome/browser/webapps/WebappRegistryTest.java
(right):

https://codereview.chromium.org/1749603002/diff/20001/chrome/android/junit/sr...
chrome/android/junit/src/org/chromium/chrome/browser/webapps/WebappRegistryTest.java:327:
assertTrue(actual.contains("webapp1"));
On 2016/03/08 23:03:52, dfalcantara wrote:
> Should this check for equality with TextUtils.equals() instead of checking for
> contains()?

I just followed what was already in this test - happy to change it to
TextUtils.equals() if you think that's better.

https://codereview.chromium.org/1749603002/diff/20001/chrome/browser/browsing...
File chrome/browser/browsing_data/browsing_data_remover.cc (right):

https://codereview.chromium.org/1749603002/diff/20001/chrome/browser/browsing...
chrome/browser/browsing_data/browsing_data_remover.cc:562:
waiting_for_clear_webapp_history_ = true;
On 2016/03/08 22:40:39, dfalcantara wrote:
> The other clearing functions generally seem to PostTaskAndReply; is that not
> required in this case because of the AsyncTask in Java land?  If so, it'd be
> worth commenting on that here.

I actually have no idea, I was just doing this the same way that the other JNI
call does it. :)

[gone, 2016-03-10 23:27:22]

I don't think I saw anything egregious here, but I'll take another pass.

https://chromiumcodereview.appspot.com/1749603002/diff/20001/chrome/android/j...
File
chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappDataStorage.java
(right):

https://chromiumcodereview.appspot.com/1749603002/diff/20001/chrome/android/j...
chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappDataStorage.java:240:
private final class BitmapFetchTask extends AsyncTask<Void, Void, Bitmap> {
So much \o/.

https://chromiumcodereview.appspot.com/1749603002/diff/20001/chrome/android/j...
File
chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappRegistry.java
(right):

https://chromiumcodereview.appspot.com/1749603002/diff/20001/chrome/android/j...
chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappRegistry.java:187:
static void clearWebappHistory(Context context, final long callbackPointer) {
On 2016/03/09 08:18:32, dominickn wrote:
> On 2016/03/08 22:40:39, dfalcantara wrote:
> > If this is only called by native code, use private static void.
> 
> I did this in patchset #3, but it fails to compile on the
> android_clang_dbg_recipe bot with an "uncalled private method" error from
> findbugs. I think I'll need to remove the private?

Strange... it's definitely called, isn't it?  I mean, it even compiled and
presumably ran when you did it locally.  Maybe findbugs is having trouble
unraveling it.  I'm fine with removing private.

https://chromiumcodereview.appspot.com/1749603002/diff/20001/chrome/android/j...
File
chrome/android/javatests/src/org/chromium/chrome/browser/preferences/privacy/ClearBrowsingDataPreferencesTest.java
(right):

https://chromiumcodereview.appspot.com/1749603002/diff/20001/chrome/android/j...
chrome/android/javatests/src/org/chromium/chrome/browser/preferences/privacy/ClearBrowsingDataPreferencesTest.java:165:
WebappDataStorage.getOriginUrl(getActivity(), "first",
On 2016/03/09 08:18:32, dominickn wrote:
> On 2016/03/08 23:03:52, dfalcantara wrote:
> > Indenting here seems like it's kind of all over the place
> 
> Indenting Java I don't know how

I want to say it's kind of subjective in these cases, but I guess this new
vresion seems alright.

https://chromiumcodereview.appspot.com/1749603002/diff/20001/chrome/android/j...
File
chrome/android/junit/src/org/chromium/chrome/browser/webapps/WebappRegistryTest.java
(right):

https://chromiumcodereview.appspot.com/1749603002/diff/20001/chrome/android/j...
chrome/android/junit/src/org/chromium/chrome/browser/webapps/WebappRegistryTest.java:327:
assertTrue(actual.contains("webapp1"));
On 2016/03/09 08:18:33, dominickn wrote:
> On 2016/03/08 23:03:52, dfalcantara wrote:
> > Should this check for equality with TextUtils.equals() instead of checking
for
> > contains()?
> 
> I just followed what was already in this test - happy to change it to
> TextUtils.equals() if you think that's better.

Acknowledged.

https://chromiumcodereview.appspot.com/1749603002/diff/20001/chrome/browser/b...
File chrome/browser/browsing_data/browsing_data_remover.cc (right):

https://chromiumcodereview.appspot.com/1749603002/diff/20001/chrome/browser/b...
chrome/browser/browsing_data/browsing_data_remover.cc:562:
waiting_for_clear_webapp_history_ = true;
On 2016/03/09 08:18:33, dominickn wrote:
> On 2016/03/08 22:40:39, dfalcantara wrote:
> > The other clearing functions generally seem to PostTaskAndReply; is that not
> > required in this case because of the AsyncTask in Java land?  If so, it'd be
> > worth commenting on that here.
> 
> I actually have no idea, I was just doing this the same way that the other JNI
> call does it. :)

Ah, in that case could you leave a comment to make it clearer?

https://chromiumcodereview.appspot.com/1749603002/diff/40001/chrome/android/j...
File
chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappDataStorage.java
(right):

https://chromiumcodereview.appspot.com/1749603002/diff/40001/chrome/android/j...
chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappDataStorage.java:60:
} else {
Does this need to run in the case when it's invalid?

https://chromiumcodereview.appspot.com/1749603002/diff/40001/chrome/android/j...
File
chrome/android/junit/src/org/chromium/chrome/browser/webapps/WebappDataStorageTest.java
(right):

https://chromiumcodereview.appspot.com/1749603002/diff/40001/chrome/android/j...
chrome/android/junit/src/org/chromium/chrome/browser/webapps/WebappDataStorageTest.java:46:
T mExpected;
Put this field up at the top.

https://chromiumcodereview.appspot.com/1749603002/diff/40001/chrome/android/j...
File
chrome/android/junit/src/org/chromium/chrome/browser/webapps/WebappRegistryTest.java
(right):

https://chromiumcodereview.appspot.com/1749603002/diff/40001/chrome/android/j...
chrome/android/junit/src/org/chromium/chrome/browser/webapps/WebappRegistryTest.java:57:
Set<String> mExpected;
members go at the top

[gone, 2016-03-10 23:30:23]

Er, lgtm % my comments and Mounir's review

[dominickn, 2016-03-11 05:14:43]

Thanks!

ping @mlamouri - PTAL. I'll need to follow up with dfalcantara about how much
more plumbing/refactoring will be required to actually allow notifications to
open in the webapp frame.

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

https://codereview.chromium.org/1749603002/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappDataStorage.java:240:
private final class BitmapFetchTask extends AsyncTask<Void, Void, Bitmap> {
On 2016/03/10 23:27:21, dfalcantara wrote:
> So much \o/.

\o/ \o/

https://codereview.chromium.org/1749603002/diff/20001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappRegistry.java
(right):

https://codereview.chromium.org/1749603002/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappRegistry.java:187:
static void clearWebappHistory(Context context, final long callbackPointer) {
On 2016/03/10 23:27:21, dfalcantara wrote:
> On 2016/03/09 08:18:32, dominickn wrote:
> > On 2016/03/08 22:40:39, dfalcantara wrote:
> > > If this is only called by native code, use private static void.
> > 
> > I did this in patchset #3, but it fails to compile on the
> > android_clang_dbg_recipe bot with an "uncalled private method" error from
> > findbugs. I think I'll need to remove the private?
> 
> Strange... it's definitely called, isn't it?  I mean, it even compiled and
> presumably ran when you did it locally.  Maybe findbugs is having trouble
> unraveling it.  I'm fine with removing private.

Done.

https://codereview.chromium.org/1749603002/diff/20001/chrome/browser/browsing...
File chrome/browser/browsing_data/browsing_data_remover.cc (right):

https://codereview.chromium.org/1749603002/diff/20001/chrome/browser/browsing...
chrome/browser/browsing_data/browsing_data_remover.cc:562:
waiting_for_clear_webapp_history_ = true;
On 2016/03/10 23:27:21, dfalcantara wrote:
> On 2016/03/09 08:18:33, dominickn wrote:
> > On 2016/03/08 22:40:39, dfalcantara wrote:
> > > The other clearing functions generally seem to PostTaskAndReply; is that
not
> > > required in this case because of the AsyncTask in Java land?  If so, it'd
be
> > > worth commenting on that here.
> > 
> > I actually have no idea, I was just doing this the same way that the other
JNI
> > call does it. :)
> 
> Ah, in that case could you leave a comment to make it clearer?

Done. The threading model across JNI calls is still evil black magic to me.

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

https://codereview.chromium.org/1749603002/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappDataStorage.java:60:
} else {
On 2016/03/10 23:27:22, dfalcantara wrote:
> Does this need to run in the case when it's invalid?

No. WebappDataStorage.getLastUsedTime() should only return INVALID when
WebappDataStorage.open() is called before WebappRegistry.registerWebapp(),
which is an error case. When history is cleared, the last used time is UNSET
rather than INVALID.

When last used time is INVALID, there shouldn't be any stored data, so there's
no need to update it, and so we assert that it's empty.

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

https://codereview.chromium.org/1749603002/diff/40001/chrome/android/junit/sr...
chrome/android/junit/src/org/chromium/chrome/browser/webapps/WebappDataStorageTest.java:46:
T mExpected;
On 2016/03/10 23:27:22, dfalcantara wrote:
> Put this field up at the top.

Done.

https://codereview.chromium.org/1749603002/diff/40001/chrome/android/junit/sr...
File
chrome/android/junit/src/org/chromium/chrome/browser/webapps/WebappRegistryTest.java
(right):

https://codereview.chromium.org/1749603002/diff/40001/chrome/android/junit/sr...
chrome/android/junit/src/org/chromium/chrome/browser/webapps/WebappRegistryTest.java:57:
Set<String> mExpected;
On 2016/03/10 23:27:22, dfalcantara wrote:
> members go at the top

Done.

[dominickn, 2016-03-11 05:15:02]

dominickn@chromium.org changed reviewers:
+ mkwst@chromium.org

[dominickn, 2016-03-11 05:17:38]

+mkwst - PTAL at the BDR. Thanks!

[Mike West, 2016-03-11 15:45:29]

The browsing data remover change LGTM.

That said, have you talked with the privacy team about the data storage and
removal? For instance, if the data is only useful for ~10 days, is there a
mechanism going through and purging expired data, is this data intended to be
synced along with the rest of your history, etc.

[dominickn, 2016-03-11 22:41:23]

On 2016/03/11 15:45:29, Mike West wrote:
> The browsing data remover change LGTM.
> 
> That said, have you talked with the privacy team about the data storage and
> removal? For instance, if the data is only useful for ~10 days, is there a
> mechanism going through and purging expired data, is this data intended to be
> synced along with the rest of your history, etc.

The history clearing added in this CL was at the request of the privacy team.

The WebappDataStorage already has a purging mechanism - data that hasn't been
accessed in 3 months is automatically evicted,
with the cleanup process running once a month. The data added in this CL will be
purged by that mechanism as well. I believe
homescreen shortcuts in general aren't synced between devices.

[Mike West, 2016-03-14 10:39:39]

On 2016/03/11 at 22:41:23, dominickn wrote:
> On 2016/03/11 15:45:29, Mike West wrote:
> > The browsing data remover change LGTM.
> > 
> > That said, have you talked with the privacy team about the data storage and
> > removal? For instance, if the data is only useful for ~10 days, is there a
> > mechanism going through and purging expired data, is this data intended to
be
> > synced along with the rest of your history, etc.
> 
> The history clearing added in this CL was at the request of the privacy team.
> 
> The WebappDataStorage already has a purging mechanism - data that hasn't been
accessed in 3 months is automatically evicted,
> with the cleanup process running once a month. The data added in this CL will
be purged by that mechanism as well. I believe
> homescreen shortcuts in general aren't synced between devices.

Good, thanks.

[mlamouri (slow - plz ping), 2016-03-14 17:09:20]

FYI, I will have a look at this tomorrow.

[mlamouri (slow - plz ping), 2016-03-16 13:34:28]

Looks good in general but I would like to see if you could do better with
regards to the origin definition (ie. top frame origin vs target origin).

Also, it seems that ideally, you want to use the Manifest's scope property here.
Would you be willing to implement this?

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

https://codereview.chromium.org/1749603002/diff/60001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappDataStorage.java:76:
@VisibleForTesting
Can you remove this? I'm pretty sure it is not visible for testing.

https://codereview.chromium.org/1749603002/diff/60001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappDataStorage.java:150:
* This does not remove the stored splash screen image (if any) for the app.
Deleting the origin sounds odd as in not really related to history and might
give a false impression of privacy (ie. the splashscreen image might still
reveals the website). Though I guess it's a requirement from privacy.

https://codereview.chromium.org/1749603002/diff/60001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappDataStorage.java:239:
void updateLastUsedTime(long lastUsedTime) {
Did you consider updateLastUsedTime() and resetLastUsedTime()? Actually, I can't
see a usage with updateLastUsedTime() that takes something else than current
time.

https://codereview.chromium.org/1749603002/diff/60001/chrome/browser/android/...
File chrome/browser/android/shortcut_helper.cc (right):

https://codereview.chromium.org/1749603002/diff/60001/chrome/browser/android/...
chrome/browser/android/shortcut_helper.cc:138:
web_contents->GetLastCommittedURL().GetOrigin().spec()));
I don't think that's a good idea to use the top frame url's origin. Ideally, we
should probably use ShortcutInfo.url because this is where the shortcut will
point to and that will guarantee a consistency.

[dominickn, 2016-03-17 04:11:11]

Description was changed from

==========
Store origins in WebappDataStorage, and purge them when history is cleared.

Sites which have been added to homescreen on Android should be
allowed to open in standalone mode in response to push notifications. As
Chromium cannot detect what sites are on the homescreen after they
have been added, the heuristic is sites which have been launched from
homescreen within the last ten days will be opened in standalone from
a tap on a notification.

This CL lays the groundwork for this feature by storing webapp origins
in WebappDataStorage. This is needed to be able to look up the last used
time, which is currently persisted in WebappDataStorage.

The last used time and webapp origin are both forms of history, so this
CL also resets both pieces of data when the user clears their history.
This requires further changes to allow null values in these fields. The
last used time is set appropriately when the app is next launched from
homescreen; the origin URL is set *every* time the app is launched, as
a lookup of the value is already an asynchronous operation. Additional
tests for the behaviour introduced in this CL are added.

BUG=541711,570579
==========

to

==========
Store URLs in WebappDataStorage, and purge them when history is cleared.

Sites which have been added to homescreen on Android should be
allowed to open in standalone mode in response to push notifications. As
Chromium cannot detect what sites are on the homescreen after they
have been added, the heuristic is sites which have been launched from
homescreen within the last ten days will be opened in standalone from
a tap on a notification.

This CL lays the groundwork for this feature by storing webapp URLs
in WebappDataStorage. This is needed to be able to look up the last used
time, which is currently persisted in WebappDataStorage.

The last used time and webapp URL combined are history, so this
CL also resets both pieces of data when the user clears their history.
This requires further changes to allow null values in these fields. The
last used time is set appropriately when the app is next launched from
homescreen; the URL is reset if it is empty when the app is launched.
Additional tests for the behaviour introduced in this CL are added.

BUG=541711,570579
==========

[dominickn, 2016-03-17 04:31:27]

@mlamouri: Thanks!

@mlamouri, @dfalcantara: PTAnL. I've renamed "origin" to "url", and also made it
so that the URL is only set in WebappActivity if it doesn't exist.

Also note that there will be a follow up patch which will copy much of the other
data stored in the homescreen intent into the WebappDataStorage, as launching
from a notification to a full screen webapp won't work otherwise (see
crbug.com/594872). Since there's no way to query the launcher about homescreen
shortcuts, it means we have to maintain the data twice (it also needs to stay in
the homescreen intent so it can be restored/etc.).


On 2016/03/16 13:34:28, Mounir Lamouri wrote:
> Also, it seems that ideally, you want to use the Manifest's scope property
here.
> Would you be willing to implement this?

I'm happy to take a look (is there a bug?). However, I'd like to implement all
of this first before adding on the scope, since we need something that's working
for the apps out there that don't have scope in their manifests.

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

https://codereview.chromium.org/1749603002/diff/60001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappDataStorage.java:76:
@VisibleForTesting
On 2016/03/16 13:34:28, Mounir Lamouri wrote:
> Can you remove this? I'm pretty sure it is not visible for testing.

It's used in the ClearBrowsingDataPreferencesTest, so without this
annotation ProGuard strips out the reference and the test crashes
with NoSuchMethodError.

https://codereview.chromium.org/1749603002/diff/60001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappDataStorage.java:150:
* This does not remove the stored splash screen image (if any) for the app.
On 2016/03/16 13:34:28, Mounir Lamouri wrote:
> Deleting the origin sounds odd as in not really related to history and might
> give a false impression of privacy (ie. the splashscreen image might still
> reveals the website). Though I guess it's a requirement from privacy.

I guess the combination of having a last used time and an origin gives you
history information. :)

https://codereview.chromium.org/1749603002/diff/60001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappDataStorage.java:239:
void updateLastUsedTime(long lastUsedTime) {
On 2016/03/16 13:34:28, Mounir Lamouri wrote:
> Did you consider updateLastUsedTime() and resetLastUsedTime()? Actually, I
can't
> see a usage with updateLastUsedTime() that takes something else than current
> time.

Originally, the reset method used this mechanism, but now it doesn't.
Refactored.

https://codereview.chromium.org/1749603002/diff/60001/chrome/browser/android/...
File chrome/browser/android/shortcut_helper.cc (right):

https://codereview.chromium.org/1749603002/diff/60001/chrome/browser/android/...
chrome/browser/android/shortcut_helper.cc:138:
web_contents->GetLastCommittedURL().GetOrigin().spec()));
On 2016/03/16 13:34:28, Mounir Lamouri wrote:
> I don't think that's a good idea to use the top frame url's origin. Ideally,
we
> should probably use ShortcutInfo.url because this is where the shortcut will
> point to and that will guarantee a consistency.

Done.

[gone, 2016-03-18 17:52:21]

still lgtm on my end.

https://chromiumcodereview.appspot.com/1749603002/diff/80001/chrome/android/j...
File
chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappDataStorage.java
(right):

https://chromiumcodereview.appspot.com/1749603002/diff/80001/chrome/android/j...
chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappDataStorage.java:120:
* already a URL stored; since webapps added to homescreen cannot change the URL
which they
nit: home screen

https://chromiumcodereview.appspot.com/1749603002/diff/80001/chrome/android/j...
chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappDataStorage.java:125:
* @param url The URL to set for the web app.
nit: Indent "The URL ..."

[mlamouri (slow - plz ping), 2016-03-21 15:01:52]

Now I'm confused. If you save `url`, aren't you basically starting implementing
bug 594872 because start_url is saved as mUri in WebAppInfo. What's the general
plan with bug 594872?

[dominickn, 2016-03-21 21:27:23]

On 2016/03/21 15:01:52, Mounir Lamouri wrote:
> Now I'm confused. If you save `url`, aren't you basically starting
implementing
> bug 594872 because start_url is saved as mUri in WebAppInfo. What's the
general
> plan with bug 594872?

Yes, the idea now is to store the exact information from the homescreen intent
in
WebappDataStorage. So rather than strip down to an origin in this CL, we store
the precise URL (and then convert the stored URL to an origin or whatever
other form we think is best when it's used). When we have `scope` available,
we'll switch to using that.

The follow up to this patch will fix crbug.com/594872 by storing every other
important
piece of data from the homescreen intent in WebappDataStorage as well. This CL
is
now consistent with that bug. If I had stored the origin, it would have been a
redundant
piece of information since the origin is retrievable from the URL.

[dominickn, 2016-03-24 04:23:04]

@mlamouri: ping?

[mlamouri (slow - plz ping), 2016-03-24 10:36:42]

Sorry about the delay. Because I wrote a comment, it was no longer in my review
queue (but in my CL with drafts... this is silly!).

Could you maybe call it scope then? Maybe you could pass the origin as a default
value (because today we don't use the Manifest value) so when we implement scope
in the Manifest, it would be just about plumbing the value up to this CL. I'm
afraid that making this the URL will be very confusing.

lgtm with that.

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

https://codereview.chromium.org/1749603002/diff/60001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappDataStorage.java:150:
* This does not remove the stored splash screen image (if any) for the app.
On 2016/03/17 at 04:31:26, dominickn wrote:
> On 2016/03/16 13:34:28, Mounir Lamouri wrote:
> > Deleting the origin sounds odd as in not really related to history and might
> > give a false impression of privacy (ie. the splashscreen image might still
> > reveals the website). Though I guess it's a requirement from privacy.
> 
> I guess the combination of having a last used time and an origin gives you
history information. :)

Sure. What I meant is that the splashscreen image wil likely leak the origin.

[dominickn, 2016-03-25 01:56:13]

The CQ bit was checked by dominickn@chromium.org

[dominickn, 2016-03-25 01:56:14]

The patchset sent to the CQ was uploaded after l-g-t-m from mkwst@chromium.org,
dfalcantara@chromium.org, mlamouri@chromium.org
Link to the patchset: https://codereview.chromium.org/1749603002/#ps120001
(title: "Rebase")

[commit-bot: I haz the power, 2016-03-25 01:56:36]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1749603002/120001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1749603002/120001

[commit-bot: I haz the power, 2016-03-25 02:03:01]

Description was changed from

==========
Store URLs in WebappDataStorage, and purge them when history is cleared.

Sites which have been added to homescreen on Android should be
allowed to open in standalone mode in response to push notifications. As
Chromium cannot detect what sites are on the homescreen after they
have been added, the heuristic is sites which have been launched from
homescreen within the last ten days will be opened in standalone from
a tap on a notification.

This CL lays the groundwork for this feature by storing webapp URLs
in WebappDataStorage. This is needed to be able to look up the last used
time, which is currently persisted in WebappDataStorage.

The last used time and webapp URL combined are history, so this
CL also resets both pieces of data when the user clears their history.
This requires further changes to allow null values in these fields. The
last used time is set appropriately when the app is next launched from
homescreen; the URL is reset if it is empty when the app is launched.
Additional tests for the behaviour introduced in this CL are added.

BUG=541711,570579
==========

to

==========
Store URLs in WebappDataStorage, and purge them when history is cleared.

Sites which have been added to homescreen on Android should be
allowed to open in standalone mode in response to push notifications. As
Chromium cannot detect what sites are on the homescreen after they
have been added, the heuristic is sites which have been launched from
homescreen within the last ten days will be opened in standalone from
a tap on a notification.

This CL lays the groundwork for this feature by storing webapp URLs
in WebappDataStorage. This is needed to be able to look up the last used
time, which is currently persisted in WebappDataStorage.

The last used time and webapp URL combined are history, so this
CL also resets both pieces of data when the user clears their history.
This requires further changes to allow null values in these fields. The
last used time is set appropriately when the app is next launched from
homescreen; the URL is reset if it is empty when the app is launched.
Additional tests for the behaviour introduced in this CL are added.

BUG=541711,570579
==========

[commit-bot: I haz the power, 2016-03-25 02:03:04]

Committed patchset #7 (id:120001)

[commit-bot: I haz the power, 2016-03-25 02:04:00]

Description was changed from

==========
Store URLs in WebappDataStorage, and purge them when history is cleared.

Sites which have been added to homescreen on Android should be
allowed to open in standalone mode in response to push notifications. As
Chromium cannot detect what sites are on the homescreen after they
have been added, the heuristic is sites which have been launched from
homescreen within the last ten days will be opened in standalone from
a tap on a notification.

This CL lays the groundwork for this feature by storing webapp URLs
in WebappDataStorage. This is needed to be able to look up the last used
time, which is currently persisted in WebappDataStorage.

The last used time and webapp URL combined are history, so this
CL also resets both pieces of data when the user clears their history.
This requires further changes to allow null values in these fields. The
last used time is set appropriately when the app is next launched from
homescreen; the URL is reset if it is empty when the app is launched.
Additional tests for the behaviour introduced in this CL are added.

BUG=541711,570579
==========

to

==========
Store URLs in WebappDataStorage, and purge them when history is cleared.

Sites which have been added to homescreen on Android should be
allowed to open in standalone mode in response to push notifications. As
Chromium cannot detect what sites are on the homescreen after they
have been added, the heuristic is sites which have been launched from
homescreen within the last ten days will be opened in standalone from
a tap on a notification.

This CL lays the groundwork for this feature by storing webapp URLs
in WebappDataStorage. This is needed to be able to look up the last used
time, which is currently persisted in WebappDataStorage.

The last used time and webapp URL combined are history, so this
CL also resets both pieces of data when the user clears their history.
This requires further changes to allow null values in these fields. The
last used time is set appropriately when the app is next launched from
homescreen; the URL is reset if it is empty when the app is launched.
Additional tests for the behaviour introduced in this CL are added.

BUG=541711,570579

Committed: https://crrev.com/6e400421b116644b4e8b23c29fd2e0c3886af3af
Cr-Commit-Position: refs/heads/master@{#383223}
==========

[commit-bot: I haz the power, 2016-03-25 02:04:01]

Patchset 7 (id:??) landed as
https://crrev.com/6e400421b116644b4e8b23c29fd2e0c3886af3af
Cr-Commit-Position: refs/heads/master@{#383223}