Store standalone web app data in WebappDataStorage as well as the homescreen intent.

Store standalone web app data in WebappDataStorage as well as the homescreen intent.

Homescreen launch intents for standalone web apps on Android contain
several properties which WebappActivity requires to launch the app in
the web frame (e.g. icon, names, theme/background color). This structure
means that launching webapps from sources other than the homescreen
(e.g. a notification) won't work. The root problem is that Android does
not have an API for querying shortcuts on the home screen after they
have been added.

This CL mirrors the homescreen intent data into Android Shared
Preferences via WebappDataStorage, and refactors how homescreen launch
intents for standalone apps are created. Standalone web apps added to
homescreen before this CL will have their data copied into a
WebappDataStorage when they are next launched. A new scope field is
computed from the URL to identify the URLs over which a web app may
control. This CL enables Chrome to launch a web app to standalone
mode without the homescreen intent.

The semantics of WebappDataStorage have also changed. Now, the only
correct way to access a WebappDataStorage is via the WebappRegistry.
New methods to access storage objects have been added to the registry,
and all clients should use these.

The existing WebappDataStorage expiry mechanism is maintained in this
CL, ensuring that web apps which have not been launched recently will
have their stored data removed. Additionally, URLs, scopes, and last
access times are purged when the user clears history.

Additional and updated tests for this functionality are also added.

BUG=594872
Committed: https://crrev.com/6509a4deca126b49eb9121fefedcfc092dee0631
Cr-Commit-Position: refs/heads/master@{#385402}

[dominickn, 2016-03-31 12:02:59]

Description was changed from

==========
Store standalone web app data in WebappDataStorage as well as the homescreen
intent.

Homescreen launch intents for standalone web apps on Android contain
several properties which WebappActivity requires to launch the app in
the web frame (e.g. icon, names, theme/background color). This structure
means that launching webapps from sources other than the homescreen
(e.g. a notification) won't work. The root problem is that Android does
not have an API for querying shortcuts on the home screen after they
have been added.

This CL mirrors the homescreen intent data into Android Shared
Preferences via WebappDataStorage, and refactors how homescreen launch
intents for standalone apps are created. Standalone web apps added to
homescreen before this CL will have their data copied into a
WebappDataStorage when they are next launched. By adding a new method to
load and create intents from a WebappDataStorage, this CL enables Chrome
to launch a web app to standalone mode without the homescreen intent.

The existing WebappDataStorage expiry mechanism is maintained in this
CL, ensuring that web apps which have not been launched recently will
have their stored data removed. Additionally, URLs, scopes, and last
access times are purged when the user clears history.

Additional and updated tests for this functionality are added.

BUG=594872
==========

to

==========
Store standalone web app data in WebappDataStorage as well as the homescreen
intent.

Homescreen launch intents for standalone web apps on Android contain
several properties which WebappActivity requires to launch the app in
the web frame (e.g. icon, names, theme/background color). This structure
means that launching webapps from sources other than the homescreen
(e.g. a notification) won't work. The root problem is that Android does
not have an API for querying shortcuts on the home screen after they
have been added.

This CL mirrors the homescreen intent data into Android Shared
Preferences via WebappDataStorage, and refactors how homescreen launch
intents for standalone apps are created. Standalone web apps added to
homescreen before this CL will have their data copied into a
WebappDataStorage when they are next launched. By adding a new method to
load and create intents from a WebappDataStorage, this CL enables Chrome
to launch a web app to standalone mode without the homescreen intent.

The semantics of WebappDataStorage have also been changed. Now, the
only correct way to access a WebappDataStorage is via the
WebappRegistry. New methods to access storage objects have been
added to the registry.

The existing WebappDataStorage expiry mechanism is maintained in this
CL, ensuring that web apps which have not been launched recently will
have their stored data removed. Additionally, URLs, scopes, and last
access times are purged when the user clears history.

Additional and updated tests for this functionality are also added.

BUG=594872
==========

[dominickn, 2016-03-31 12:10:57]

Description was changed from

==========
Store standalone web app data in WebappDataStorage as well as the homescreen
intent.

Homescreen launch intents for standalone web apps on Android contain
several properties which WebappActivity requires to launch the app in
the web frame (e.g. icon, names, theme/background color). This structure
means that launching webapps from sources other than the homescreen
(e.g. a notification) won't work. The root problem is that Android does
not have an API for querying shortcuts on the home screen after they
have been added.

This CL mirrors the homescreen intent data into Android Shared
Preferences via WebappDataStorage, and refactors how homescreen launch
intents for standalone apps are created. Standalone web apps added to
homescreen before this CL will have their data copied into a
WebappDataStorage when they are next launched. By adding a new method to
load and create intents from a WebappDataStorage, this CL enables Chrome
to launch a web app to standalone mode without the homescreen intent.

The semantics of WebappDataStorage have also been changed. Now, the
only correct way to access a WebappDataStorage is via the
WebappRegistry. New methods to access storage objects have been
added to the registry.

The existing WebappDataStorage expiry mechanism is maintained in this
CL, ensuring that web apps which have not been launched recently will
have their stored data removed. Additionally, URLs, scopes, and last
access times are purged when the user clears history.

Additional and updated tests for this functionality are also added.

BUG=594872
==========

to

==========
Store standalone web app data in WebappDataStorage as well as the homescreen
intent.

Homescreen launch intents for standalone web apps on Android contain
several properties which WebappActivity requires to launch the app in
the web frame (e.g. icon, names, theme/background color). This structure
means that launching webapps from sources other than the homescreen
(e.g. a notification) won't work. The root problem is that Android does
not have an API for querying shortcuts on the home screen after they
have been added.

This CL mirrors the homescreen intent data into Android Shared
Preferences via WebappDataStorage, and refactors how homescreen launch
intents for standalone apps are created. Standalone web apps added to
homescreen before this CL will have their data copied into a
WebappDataStorage when they are next launched. By adding a new method to
load and create intents from a WebappDataStorage, this CL enables Chrome
to launch a web app to standalone mode without the homescreen intent.

The semantics of WebappDataStorage have also been changed. Now, the
only correct way to access a WebappDataStorage is via the
WebappRegistry. New methods to access storage objects have been
added to the registry, and all clients should use these.

The existing WebappDataStorage expiry mechanism is maintained in this
CL, ensuring that web apps which have not been launched recently will
have their stored data removed. Additionally, URLs, scopes, and last
access times are purged when the user clears history.

Additional and updated tests for this functionality are also added.

BUG=594872
==========

[dominickn, 2016-03-31 12:59:35]

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

[dominickn, 2016-03-31 12:59:36]

PTAL. This patch brings all of the necessary bits of the 
homescreen intent into WebappDataStorage. Hopefully it 
refactors that structure for the better, forcing clients to access 
it from WebappRegistry only.

The changes to splashscreen fetching are necessary because
a WebappDataStorage must be created by adding a shortcut
*before* the splash screen image can be stored in it - otherwise
there is a potential race between the shortcut creation and
splash image download as to who gets to create the 
DataStorage first. I serialised this process to clearly enforce that
shortcut creation creates the DataStorage, which the splash
screen image is then added to once it's downloaded.

The next patch will implement notification in the webapp frame by
checking for a WebappDataStorage with the appropriate scope,
and creating an intent from it to send to WebappLauncherActivity.
It'll also need to implement a trusted intent check in
WebappActivity to skip the MAC check.

[gone, 2016-04-01 23:44:49]

Very cursory pass.

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

https://chromiumcodereview.appspot.com/1845233002/diff/20001/chrome/android/j...
chrome/android/java/src/org/chromium/chrome/browser/ShortcutHelper.java:231: *
@param id  Id of the webapp.
nit: Align these maybe?  It's kind of blobby atm.

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

https://chromiumcodereview.appspot.com/1845233002/diff/20001/chrome/android/j...
chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappActivity.java:256:
WebappRegistry.getWebappDataStorage(this, mWebappInfo.id(),
Does this extra callback significantly affect how quickly the WebappActivity
starts up, or how long it takes for the splash screen to be shown?

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

https://chromiumcodereview.appspot.com/1845233002/diff/20001/chrome/android/j...
chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappDataStorage.java:31:
* storage.getSplashScreenImage(callback);
Can you update this class comment so it's not so awkwardly just (1)?  Also so
that it's correct?

https://chromiumcodereview.appspot.com/1845233002/diff/20001/chrome/android/j...
chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappDataStorage.java:232:
public void createWebappLaunchIntent(final FetchCallback<Intent> callback) {
AIUI no value in reading these preferences on a background thread once we've
opened them up.  They're stored in memory as soon as we call
openSharedPreferences() the first time around.  I can't think of any other
situations where we explicitly call getString and whatever on background threads
unless we're already on that thread.

https://chromiumcodereview.appspot.com/1845233002/diff/20001/chrome/android/j...
chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappDataStorage.java:307:
* Returns the URL stored in this object, or "" if it is not stored.
Should be more explicit bot here and in the previous case that URL_INVALID is
returned instead of "".

https://chromiumcodereview.appspot.com/1845233002/diff/20001/chrome/android/j...
chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappDataStorage.java:315:
* Updates this object from the fields in the suppled intent. Does not check if
the intent is
nit: supplied

https://chromiumcodereview.appspot.com/1845233002/diff/20001/chrome/android/j...
chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappDataStorage.java:343:
// and URL are either set or cleared together.
Are we going to have to start worrying about versioning?  I'm guessing that the
next time a field is added here, you wouldn't want to key it on KEY_NAME.

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

https://chromiumcodereview.appspot.com/1845233002/diff/20001/chrome/android/j...
chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappRegistry.java:41:
* Called when a retrieval of the set of stored web app ids occurs.
nit: IDs

https://chromiumcodereview.appspot.com/1845233002/diff/20001/chrome/browser/a...
File chrome/browser/android/shortcut_helper.cc (right):

https://chromiumcodereview.appspot.com/1845233002/diff/20001/chrome/browser/a...
chrome/browser/android/shortcut_helper.cc:87: uintptr_t callback_pointer =
reinterpret_cast<uintptr_t>(
Don't we know whether the callback will be used at this point?  Does it make
sense to just not create it, then pass 0 in that case?

[gone, 2016-04-01 23:54:31]

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

https://chromiumcodereview.appspot.com/1845233002/diff/20001/chrome/android/j...
chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappDataStorage.java:191:
return context.getApplicationContext().getSharedPreferences(
Ironically, this is probably the only call that would trigger a StrictMode
violation in this entire class as the preferences are read from disk.  All the
other random preference setting on background threads is unnecessary (unless
additional work encoding or decoding is necessary).

https://chromiumcodereview.appspot.com/1845233002/diff/20001/chrome/android/j...
chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappDataStorage.java:404:
public static class Factory {
Is this thing still needed?

[dominickn, 2016-04-04 07:26:24]

Thanks!

https://codereview.chromium.org/1845233002/diff/20001/chrome/android/java/src...
File chrome/android/java/src/org/chromium/chrome/browser/ShortcutHelper.java
(right):

https://codereview.chromium.org/1845233002/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/ShortcutHelper.java:231: *
@param id  Id of the webapp.
On 2016/04/01 23:44:48, dfalcantara wrote:
> nit: Align these maybe?  It's kind of blobby atm.

Done.

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

https://codereview.chromium.org/1845233002/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappActivity.java:256:
WebappRegistry.getWebappDataStorage(this, mWebappInfo.id(),
On 2016/04/01 23:44:48, dfalcantara wrote:
> Does this extra callback significantly affect how quickly the WebappActivity
> starts up, or how long it takes for the splash screen to be shown?

The extra callback should only affect how long it takes to get the splash image
up, since it adds one more async call to get and check the registered webapp
IDs. As far as I can tell, the extra callback doesn't detrimentally affect this
(based on my highly unscientific testing of this patch against regular Chrome).

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

https://codereview.chromium.org/1845233002/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappDataStorage.java:31:
* storage.getSplashScreenImage(callback);
On 2016/04/01 23:44:48, dfalcantara wrote:
> Can you update this class comment so it's not so awkwardly just (1)?  Also so
> that it's correct?

I got rid of it. Not too many other instances of "how to use this class" around
Chrome, and it's very easy to grep for this.

https://codereview.chromium.org/1845233002/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappDataStorage.java:191:
return context.getApplicationContext().getSharedPreferences(
On 2016/04/01 23:54:31, dfalcantara wrote:
> Ironically, this is probably the only call that would trigger a StrictMode
> violation in this entire class as the preferences are read from disk.  All the
> other random preference setting on background threads is unnecessary (unless
> additional work encoding or decoding is necessary).

A lot of the time, the pattern is a static method that opens a WebappDataStorage
and then calls a method on it. So the thing needs to be wrapped in an AsyncTask
for the object creation, but then once you have a WebappDataStorage you're okay.

I've removed some of the thread asserts to make this more flexible for the
follow up patch. The methods which do bitmap encoding or decoding I've kept the
asserts in.

https://codereview.chromium.org/1845233002/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappDataStorage.java:232:
public void createWebappLaunchIntent(final FetchCallback<Intent> callback) {
On 2016/04/01 23:44:48, dfalcantara wrote:
> AIUI no value in reading these preferences on a background thread once we've
> opened them up.  They're stored in memory as soon as we call
> openSharedPreferences() the first time around.  I can't think of any other
> situations where we explicitly call getString and whatever on background
threads
> unless we're already on that thread.

This method has a possibly expensive bitmap unpack and decode for the icon. Is
that reason enough to keep it on a background thread I've refactored this to
make it work better with the follow up patch that will implement deep linking
(it now simply asserts that it's not on the UI thread).

https://codereview.chromium.org/1845233002/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappDataStorage.java:307:
* Returns the URL stored in this object, or "" if it is not stored.
On 2016/04/01 23:44:48, dfalcantara wrote:
> Should be more explicit bot here and in the previous case that URL_INVALID is
> returned instead of "".

Done.

https://codereview.chromium.org/1845233002/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappDataStorage.java:315:
* Updates this object from the fields in the suppled intent. Does not check if
the intent is
On 2016/04/01 23:44:48, dfalcantara wrote:
> nit: supplied

Done.

https://codereview.chromium.org/1845233002/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappDataStorage.java:343:
// and URL are either set or cleared together.
On 2016/04/01 23:44:48, dfalcantara wrote:
> Are we going to have to start worrying about versioning?  I'm guessing that
the
> next time a field is added here, you wouldn't want to key it on KEY_NAME.

Yeah, versioning is probably a good idea. Done.

https://codereview.chromium.org/1845233002/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappDataStorage.java:404:
public static class Factory {
On 2016/04/01 23:54:31, dfalcantara wrote:
> Is this thing still needed?

It's used in the add to homescreen and app banner tests to observe whether the
splash image is downloaded properly.

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

https://codereview.chromium.org/1845233002/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappRegistry.java:41:
* Called when a retrieval of the set of stored web app ids occurs.
On 2016/04/01 23:44:48, dfalcantara wrote:
> nit: IDs

Done.

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

https://codereview.chromium.org/1845233002/diff/20001/chrome/browser/android/...
chrome/browser/android/shortcut_helper.cc:87: uintptr_t callback_pointer =
reinterpret_cast<uintptr_t>(
On 2016/04/01 23:44:48, dfalcantara wrote:
> Don't we know whether the callback will be used at this point?  Does it make
> sense to just not create it, then pass 0 in that case?

That's a good point. This cleans up the mess here considerably.

[gone, 2016-04-04 19:04:03]

lgtm

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

https://chromiumcodereview.appspot.com/1845233002/diff/20001/chrome/android/j...
chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappDataStorage.java:191:
return context.getApplicationContext().getSharedPreferences(
On 2016/04/04 07:26:24, dominickn wrote:
> On 2016/04/01 23:54:31, dfalcantara wrote:
> > Ironically, this is probably the only call that would trigger a StrictMode
> > violation in this entire class as the preferences are read from disk.  All
the
> > other random preference setting on background threads is unnecessary (unless
> > additional work encoding or decoding is necessary).
> 
> A lot of the time, the pattern is a static method that opens a
WebappDataStorage
> and then calls a method on it. So the thing needs to be wrapped in an
AsyncTask
> for the object creation, but then once you have a WebappDataStorage you're
okay.
> 
> I've removed some of the thread asserts to make this more flexible for the
> follow up patch. The methods which do bitmap encoding or decoding I've kept
the
> asserts in.

Thanks!  I'm still seeing a potential StrictMode violation with your patch
though:

WebappRegistry->getWebappDataStorage()
onPostExecute = UI thread -> WebappDataStorage.open()
UI thread -> sFactory.create()
UI thread -> new WebappDataStorage()
UI thread -> WebappDataStorage#openSharedPreferences().

https://chromiumcodereview.appspot.com/1845233002/diff/20001/chrome/android/j...
chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappDataStorage.java:232:
public void createWebappLaunchIntent(final FetchCallback<Intent> callback) {
On 2016/04/04 07:26:24, dominickn wrote:
> On 2016/04/01 23:44:48, dfalcantara wrote:
> > AIUI no value in reading these preferences on a background thread once we've
> > opened them up.  They're stored in memory as soon as we call
> > openSharedPreferences() the first time around.  I can't think of any other
> > situations where we explicitly call getString and whatever on background
> threads
> > unless we're already on that thread.
> 
> This method has a possibly expensive bitmap unpack and decode for the icon. Is
> that reason enough to keep it on a background thread I've refactored this to
> make it work better with the follow up patch that will implement deep linking
> (it now simply asserts that it's not on the UI thread).

Arg, missed that call in that blob.  sgtm

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

https://chromiumcodereview.appspot.com/1845233002/diff/80001/chrome/android/j...
chrome/android/java/src/org/chromium/chrome/browser/ShortcutHelper.java:75: //
be correctled populated into the WebappRegistry/WebappDataStorage.
*correctly

https://chromiumcodereview.appspot.com/1845233002/diff/80001/chrome/android/j...
chrome/android/java/src/org/chromium/chrome/browser/ShortcutHelper.java:233: *
Webapp-capable shortcut intent for icon on home screen.
nit: Creates a shortcut to launch a web app on the home screen.?

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

https://chromiumcodereview.appspot.com/1845233002/diff/80001/chrome/android/j...
chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappDataStorage.java:227:
public Intent createWebappLaunchIntent() {
Need a javadoc here, too.

https://chromiumcodereview.appspot.com/1845233002/diff/80001/chrome/android/j...
chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappDataStorage.java:252:
* @param intent the web app intent to update this object to match.
1) Does it make sense to just combine this and the other variant of this?  The
only difference is that there is a callback, which you could just conditionally
null-check instead of asserting.

2) nit: intent The Intent to pull web app data from?

https://chromiumcodereview.appspot.com/1845233002/diff/80001/chrome/android/j...
chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappDataStorage.java:301:
* a valid web app launch intent.
nit: Leave a comment here about why you're stuck doing this on the background
thread.  It's hard to glean from it when a quick glance makes it seem that all
it's doing is updating prefs.

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

https://chromiumcodereview.appspot.com/1845233002/diff/80001/chrome/android/j...
chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappRegistry.java:52:
public void onWebappDataStorageRetrieved(WebappDataStorage storage);
nit: public is redundant here; the interface forces it to be.

https://chromiumcodereview.appspot.com/1845233002/diff/80001/chrome/android/j...
chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappRegistry.java:105:
storage = WebappDataStorage.open(context, webappId);
The new comment I left for Patch Set #2 starts here, which seems like it would
trigger a StrictMode violation.

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

https://chromiumcodereview.appspot.com/1845233002/diff/80001/chrome/android/j...
chrome/android/junit/src/org/chromium/chrome/browser/webapps/WebappRegistryTest.java:62:
private class Runner implements Runnable {
Can you use a CallbackHelper here instead of the random boolean?  It would make
testing cleaner if you didn't have to worry about clearing the boolean each
time.  If it's too painful, feel free to punt.

https://chromiumcodereview.appspot.com/1845233002/diff/80001/chrome/android/j...
chrome/android/junit/src/org/chromium/chrome/browser/webapps/WebappRegistryTest.java:305:
final String webapp1Url =  "https://www.google.com";
nit: Remove extra space.

https://chromiumcodereview.appspot.com/1845233002/diff/80001/chrome/android/j...
chrome/android/junit/src/org/chromium/chrome/browser/webapps/WebappRegistryTest.java:390:
final String webappUrl =  "http://www.google.com";
nit: extra space

[gone, 2016-04-04 19:04:30]

Er, not lgtm?  I don't know how to undo an accidental button click.  Blarg.

[dominickn, 2016-04-05 07:06:53]

Description was changed from

==========
Store standalone web app data in WebappDataStorage as well as the homescreen
intent.

Homescreen launch intents for standalone web apps on Android contain
several properties which WebappActivity requires to launch the app in
the web frame (e.g. icon, names, theme/background color). This structure
means that launching webapps from sources other than the homescreen
(e.g. a notification) won't work. The root problem is that Android does
not have an API for querying shortcuts on the home screen after they
have been added.

This CL mirrors the homescreen intent data into Android Shared
Preferences via WebappDataStorage, and refactors how homescreen launch
intents for standalone apps are created. Standalone web apps added to
homescreen before this CL will have their data copied into a
WebappDataStorage when they are next launched. By adding a new method to
load and create intents from a WebappDataStorage, this CL enables Chrome
to launch a web app to standalone mode without the homescreen intent.

The semantics of WebappDataStorage have also been changed. Now, the
only correct way to access a WebappDataStorage is via the
WebappRegistry. New methods to access storage objects have been
added to the registry, and all clients should use these.

The existing WebappDataStorage expiry mechanism is maintained in this
CL, ensuring that web apps which have not been launched recently will
have their stored data removed. Additionally, URLs, scopes, and last
access times are purged when the user clears history.

Additional and updated tests for this functionality are also added.

BUG=594872
==========

to

==========
Store standalone web app data in WebappDataStorage as well as the homescreen
intent.

Homescreen launch intents for standalone web apps on Android contain
several properties which WebappActivity requires to launch the app in
the web frame (e.g. icon, names, theme/background color). This structure
means that launching webapps from sources other than the homescreen
(e.g. a notification) won't work. The root problem is that Android does
not have an API for querying shortcuts on the home screen after they
have been added.

This CL mirrors the homescreen intent data into Android Shared
Preferences via WebappDataStorage, and refactors how homescreen launch
intents for standalone apps are created. Standalone web apps added to
homescreen before this CL will have their data copied into a
WebappDataStorage when they are next launched. A new scope field is
computed from the URL to identify the URLs over which a web app may
control. This CL enables Chrome to launch a web app to standalone
mode without the homescreen intent.

The semantics of WebappDataStorage have also changed. Now, the only
correct way to access a WebappDataStorage is via the WebappRegistry.
New methods to access storage objects have been added to the registry,
and all clients should use these.

The existing WebappDataStorage expiry mechanism is maintained in this
CL, ensuring that web apps which have not been launched recently will
have their stored data removed. Additionally, URLs, scopes, and last
access times are purged when the user clears history.

Additional and updated tests for this functionality are also added.

BUG=594872
==========

[dominickn, 2016-04-05 11:37:46]

Thanks! I think I've finally got the threading/strict mode correct in this
patch. I've also added a proper scope calculation and tests for it - the scope
check for now is the longest path in the web app's url.

https://codereview.chromium.org/1845233002/diff/80001/chrome/android/java/src...
File chrome/android/java/src/org/chromium/chrome/browser/ShortcutHelper.java
(right):

https://codereview.chromium.org/1845233002/diff/80001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/ShortcutHelper.java:75: //
be correctled populated into the WebappRegistry/WebappDataStorage.
On 2016/04/04 19:04:03, dfalcantara wrote:
> *correctly

Done.

https://codereview.chromium.org/1845233002/diff/80001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/ShortcutHelper.java:233: *
Webapp-capable shortcut intent for icon on home screen.
On 2016/04/04 19:04:03, dfalcantara wrote:
> nit: Creates a shortcut to launch a web app on the home screen.?

Done.

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

https://codereview.chromium.org/1845233002/diff/80001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappDataStorage.java:227:
public Intent createWebappLaunchIntent() {
On 2016/04/04 19:04:03, dfalcantara wrote:
> Need a javadoc here, too.

Done.

https://codereview.chromium.org/1845233002/diff/80001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappDataStorage.java:252:
* @param intent the web app intent to update this object to match.
On 2016/04/04 19:04:03, dfalcantara wrote:
> 1) Does it make sense to just combine this and the other variant of this?  The
> only difference is that there is a callback, which you could just
conditionally
> null-check instead of asserting.
> 
> 2) nit: intent The Intent to pull web app data from?

Done.

https://codereview.chromium.org/1845233002/diff/80001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappDataStorage.java:301:
* a valid web app launch intent.
On 2016/04/04 19:04:03, dfalcantara wrote:
> nit: Leave a comment here about why you're stuck doing this on the background
> thread.  It's hard to glean from it when a quick glance makes it seem that all
> it's doing is updating prefs.

Looking at it again, it probably doesn't need the thread assert.

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

https://codereview.chromium.org/1845233002/diff/80001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappRegistry.java:52:
public void onWebappDataStorageRetrieved(WebappDataStorage storage);
On 2016/04/04 19:04:03, dfalcantara wrote:
> nit: public is redundant here; the interface forces it to be.

Done.

https://codereview.chromium.org/1845233002/diff/80001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappRegistry.java:105:
storage = WebappDataStorage.open(context, webappId);
On 2016/04/04 19:04:03, dfalcantara wrote:
> The new comment I left for Patch Set #2 starts here, which seems like it would
> trigger a StrictMode violation.

Blargh onPostExecute is invoked on the UI thread. Fixed.

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

https://codereview.chromium.org/1845233002/diff/80001/chrome/android/junit/sr...
chrome/android/junit/src/org/chromium/chrome/browser/webapps/WebappRegistryTest.java:62:
private class Runner implements Runnable {
On 2016/04/04 19:04:03, dfalcantara wrote:
> Can you use a CallbackHelper here instead of the random boolean?  It would
make
> testing cleaner if you didn't have to worry about clearing the boolean each
> time.  If it's too painful, feel free to punt.

Refactoring to use CallbackHelper was a bit involved, but I made a new class and
it's a bit nicer now.

https://codereview.chromium.org/1845233002/diff/80001/chrome/android/junit/sr...
chrome/android/junit/src/org/chromium/chrome/browser/webapps/WebappRegistryTest.java:305:
final String webapp1Url =  "https://www.google.com";
On 2016/04/04 19:04:03, dfalcantara wrote:
> nit: Remove extra space.

Removed the whole line! #overachieving

https://codereview.chromium.org/1845233002/diff/80001/chrome/android/junit/sr...
chrome/android/junit/src/org/chromium/chrome/browser/webapps/WebappRegistryTest.java:390:
final String webappUrl =  "http://www.google.com";
On 2016/04/04 19:04:03, dfalcantara wrote:
> nit: extra space

Done.

[gone, 2016-04-05 20:40:15]

lgtm

Not seeing anything obviously egregious here...

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

https://codereview.chromium.org/1845233002/diff/140001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappDataStorage.java:59:
* thread.
Might be worth just putting a !ThreadUtils.assertOnUiThread().

https://codereview.chromium.org/1845233002/diff/140001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappDataStorage.java:221:
protected final Void doInBackground(Void... nothing) {
nit: fix indentation with temporary storage?

String bitmapString = ShortcutHelper.encodeBitmapAsString(splashScreenImage);
mPreferences.edit().putString(KEY_SPLASH_ICON, bitmapString).apply();

https://codereview.chromium.org/1845233002/diff/140001/chrome/android/junit/s...
File
chrome/android/junit/src/org/chromium/chrome/browser/webapps/WebappRegistryTest.java
(right):

https://codereview.chromium.org/1845233002/diff/140001/chrome/android/junit/s...
chrome/android/junit/src/org/chromium/chrome/browser/webapps/WebappRegistryTest.java:329:
assertTrue(mCallbackCalled);
Little bit worried about the same mCallbackCalled being set by different bits of
code, but you're probably fine.  Way around it could to have each
FetchStorageCallback store its own boolean, but it's probably alright.

[gone, 2016-04-05 20:45:42]

https://codereview.chromium.org/1845233002/diff/140001/chrome/android/junit/s...
File
chrome/android/junit/src/org/chromium/chrome/browser/ShortcutHelperTest.java
(right):

https://codereview.chromium.org/1845233002/diff/140001/chrome/android/junit/s...
chrome/android/junit/src/org/chromium/chrome/browser/ShortcutHelperTest.java:1:
// Copyright 2016 The Chromium Authors. All rights reserved.
Was this added to java_sources.gni?

[gone, 2016-04-05 20:46:43]

https://codereview.chromium.org/1845233002/diff/140001/chrome/android/junit/s...
File
chrome/android/junit/src/org/chromium/chrome/browser/ShortcutHelperTest.java
(right):

https://codereview.chromium.org/1845233002/diff/140001/chrome/android/junit/s...
chrome/android/junit/src/org/chromium/chrome/browser/ShortcutHelperTest.java:1:
// Copyright 2016 The Chromium Authors. All rights reserved.
On 2016/04/05 20:45:42, dfalcantara wrote:
> Was this added to java_sources.gni?

Bah, never mind.  Didn't realize this fell under a different target.

[dominickn, 2016-04-06 07:32:33]

The CQ bit was checked by dominickn@chromium.org

[dominickn, 2016-04-06 07:32:33]

The patchset sent to the CQ was uploaded after l-g-t-m from
dfalcantara@chromium.org
Link to the patchset: https://codereview.chromium.org/1845233002/#ps160001
(title: "Final nit")

[commit-bot: I haz the power, 2016-04-06 07:32:49]

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

[commit-bot: I haz the power, 2016-04-06 08:29:15]

Description was changed from

==========
Store standalone web app data in WebappDataStorage as well as the homescreen
intent.

Homescreen launch intents for standalone web apps on Android contain
several properties which WebappActivity requires to launch the app in
the web frame (e.g. icon, names, theme/background color). This structure
means that launching webapps from sources other than the homescreen
(e.g. a notification) won't work. The root problem is that Android does
not have an API for querying shortcuts on the home screen after they
have been added.

This CL mirrors the homescreen intent data into Android Shared
Preferences via WebappDataStorage, and refactors how homescreen launch
intents for standalone apps are created. Standalone web apps added to
homescreen before this CL will have their data copied into a
WebappDataStorage when they are next launched. A new scope field is
computed from the URL to identify the URLs over which a web app may
control. This CL enables Chrome to launch a web app to standalone
mode without the homescreen intent.

The semantics of WebappDataStorage have also changed. Now, the only
correct way to access a WebappDataStorage is via the WebappRegistry.
New methods to access storage objects have been added to the registry,
and all clients should use these.

The existing WebappDataStorage expiry mechanism is maintained in this
CL, ensuring that web apps which have not been launched recently will
have their stored data removed. Additionally, URLs, scopes, and last
access times are purged when the user clears history.

Additional and updated tests for this functionality are also added.

BUG=594872
==========

to

==========
Store standalone web app data in WebappDataStorage as well as the homescreen
intent.

Homescreen launch intents for standalone web apps on Android contain
several properties which WebappActivity requires to launch the app in
the web frame (e.g. icon, names, theme/background color). This structure
means that launching webapps from sources other than the homescreen
(e.g. a notification) won't work. The root problem is that Android does
not have an API for querying shortcuts on the home screen after they
have been added.

This CL mirrors the homescreen intent data into Android Shared
Preferences via WebappDataStorage, and refactors how homescreen launch
intents for standalone apps are created. Standalone web apps added to
homescreen before this CL will have their data copied into a
WebappDataStorage when they are next launched. A new scope field is
computed from the URL to identify the URLs over which a web app may
control. This CL enables Chrome to launch a web app to standalone
mode without the homescreen intent.

The semantics of WebappDataStorage have also changed. Now, the only
correct way to access a WebappDataStorage is via the WebappRegistry.
New methods to access storage objects have been added to the registry,
and all clients should use these.

The existing WebappDataStorage expiry mechanism is maintained in this
CL, ensuring that web apps which have not been launched recently will
have their stored data removed. Additionally, URLs, scopes, and last
access times are purged when the user clears history.

Additional and updated tests for this functionality are also added.

BUG=594872
==========

[commit-bot: I haz the power, 2016-04-06 08:29:16]

Committed patchset #9 (id:160001)

[commit-bot: I haz the power, 2016-04-06 08:30:18]

Description was changed from

==========
Store standalone web app data in WebappDataStorage as well as the homescreen
intent.

Homescreen launch intents for standalone web apps on Android contain
several properties which WebappActivity requires to launch the app in
the web frame (e.g. icon, names, theme/background color). This structure
means that launching webapps from sources other than the homescreen
(e.g. a notification) won't work. The root problem is that Android does
not have an API for querying shortcuts on the home screen after they
have been added.

This CL mirrors the homescreen intent data into Android Shared
Preferences via WebappDataStorage, and refactors how homescreen launch
intents for standalone apps are created. Standalone web apps added to
homescreen before this CL will have their data copied into a
WebappDataStorage when they are next launched. A new scope field is
computed from the URL to identify the URLs over which a web app may
control. This CL enables Chrome to launch a web app to standalone
mode without the homescreen intent.

The semantics of WebappDataStorage have also changed. Now, the only
correct way to access a WebappDataStorage is via the WebappRegistry.
New methods to access storage objects have been added to the registry,
and all clients should use these.

The existing WebappDataStorage expiry mechanism is maintained in this
CL, ensuring that web apps which have not been launched recently will
have their stored data removed. Additionally, URLs, scopes, and last
access times are purged when the user clears history.

Additional and updated tests for this functionality are also added.

BUG=594872
==========

to

==========
Store standalone web app data in WebappDataStorage as well as the homescreen
intent.

Homescreen launch intents for standalone web apps on Android contain
several properties which WebappActivity requires to launch the app in
the web frame (e.g. icon, names, theme/background color). This structure
means that launching webapps from sources other than the homescreen
(e.g. a notification) won't work. The root problem is that Android does
not have an API for querying shortcuts on the home screen after they
have been added.

This CL mirrors the homescreen intent data into Android Shared
Preferences via WebappDataStorage, and refactors how homescreen launch
intents for standalone apps are created. Standalone web apps added to
homescreen before this CL will have their data copied into a
WebappDataStorage when they are next launched. A new scope field is
computed from the URL to identify the URLs over which a web app may
control. This CL enables Chrome to launch a web app to standalone
mode without the homescreen intent.

The semantics of WebappDataStorage have also changed. Now, the only
correct way to access a WebappDataStorage is via the WebappRegistry.
New methods to access storage objects have been added to the registry,
and all clients should use these.

The existing WebappDataStorage expiry mechanism is maintained in this
CL, ensuring that web apps which have not been launched recently will
have their stored data removed. Additionally, URLs, scopes, and last
access times are purged when the user clears history.

Additional and updated tests for this functionality are also added.

BUG=594872

Committed: https://crrev.com/6509a4deca126b49eb9121fefedcfc092dee0631
Cr-Commit-Position: refs/heads/master@{#385402}
==========

[commit-bot: I haz the power, 2016-04-06 08:30:19]

Patchset 9 (id:??) landed as
https://crrev.com/6509a4deca126b49eb9121fefedcfc092dee0631
Cr-Commit-Position: refs/heads/master@{#385402}

[dominickn, 2016-04-06 09:23:44]

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

https://codereview.chromium.org/1845233002/diff/140001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappDataStorage.java:59:
* thread.
On 2016/04/05 20:40:15, dfalcantara wrote:
> Might be worth just putting a !ThreadUtils.assertOnUiThread().

As discussed, some existing tests call this method on the UI thread. I'll follow
up with another patch to fix those since I don't want to bloat this one any
more.

https://codereview.chromium.org/1845233002/diff/140001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappDataStorage.java:221:
protected final Void doInBackground(Void... nothing) {
On 2016/04/05 20:40:14, dfalcantara wrote:
> nit: fix indentation with temporary storage?
> 
> String bitmapString = ShortcutHelper.encodeBitmapAsString(splashScreenImage);
> mPreferences.edit().putString(KEY_SPLASH_ICON, bitmapString).apply();

Done.

https://codereview.chromium.org/1845233002/diff/140001/chrome/android/junit/s...
File
chrome/android/junit/src/org/chromium/chrome/browser/ShortcutHelperTest.java
(right):

https://codereview.chromium.org/1845233002/diff/140001/chrome/android/junit/s...
chrome/android/junit/src/org/chromium/chrome/browser/ShortcutHelperTest.java:1:
// Copyright 2016 The Chromium Authors. All rights reserved.
On 2016/04/05 20:46:43, dfalcantara wrote:
> On 2016/04/05 20:45:42, dfalcantara wrote:
> > Was this added to java_sources.gni?
> 
> Bah, never mind.  Didn't realize this fell under a different target.

Acknowledged.

https://codereview.chromium.org/1845233002/diff/140001/chrome/android/junit/s...
File
chrome/android/junit/src/org/chromium/chrome/browser/webapps/WebappRegistryTest.java
(right):

https://codereview.chromium.org/1845233002/diff/140001/chrome/android/junit/s...
chrome/android/junit/src/org/chromium/chrome/browser/webapps/WebappRegistryTest.java:329:
assertTrue(mCallbackCalled);
On 2016/04/05 20:40:15, dfalcantara wrote:
> Little bit worried about the same mCallbackCalled being set by different bits
of
> code, but you're probably fine.  Way around it could to have each
> FetchStorageCallback store its own boolean, but it's probably alright.

Acknowledged.