Fix up _some_web app directory StrictMode violations

Fix up _some_ web app directory StrictMode violations

* Enable Webapp document instancing for any devices running >= L.
  Tablets running Lollipop don't have any real reason to be
  excluded.

* Shifts code around so that directory accesses are all done on a
  background thread.

* webapp -> web app where it makes sense to change them in comments..

* Moves a function for recursively deleting files into a new file
  called FileUtilities.java.  I wanted to amend TestFileUtil.java,
  but that's accessible only from tests.

* Adds unit tests for the WebappDirectoryManager.

BUG=525781
Committed: https://crrev.com/8f7955c0e5a30d3d626f3cfbda53b2fab2717611
Cr-Commit-Position: refs/heads/master@{#350807}

[gone, 2015-09-22 16:09:50]

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

[gone, 2015-09-22 16:09:51]

Peter: Do you feel up to doing this review?

[Peter Wen, 2015-09-22 17:15:26]

lgtm

I presume doing cleanup in onStartWithNative is not too late? :)

[Peter Wen, 2015-09-22 17:15:26]

lgtm

I presume doing cleanup in onStartWithNative is not too late? :)

[Peter Wen, 2015-09-22 17:16:31]

Opps, comments were missing from previous reply. I'm not a committer yet, so I
believe you'll need another reviewer.

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

https://chromiumcodereview.appspot.com/1354323003/diff/40001/chrome/android/j...
chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappDirectoryManager.java:56:
assert Build.VERSION.SDK_INT >= Build.VERSION_CODES.LOLLIPOP;
Is this assert useful since the boolean right above it is the exact same value?

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

https://chromiumcodereview.appspot.com/1354323003/diff/40001/chrome/android/j...
chrome/android/javatests/src/org/chromium/chrome/browser/webapps/WebappDirectoryManagerTest.java:26:
private static final String TAG = "cr.webapps.WebappDirectoryManagerTest";
Probably doesn't matter for test code, but I think we're replacing cr.* with
cr_*.

[Lalit Maganti, 2015-09-22 17:21:58]

lalitm@google.com changed reviewers:
+ lalitm@google.com

[Lalit Maganti, 2015-09-22 17:21:58]

https://chromiumcodereview.appspot.com/1354323003/diff/40001/base/android/jav...
File base/android/java/src/org/chromium/base/FileUtilities.java (right):

https://chromiumcodereview.appspot.com/1354323003/diff/40001/base/android/jav...
base/android/java/src/org/chromium/base/FileUtilities.java:12: public class
FileUtilities {
Out of curiousity: why Utilities in full? In Java (and Chrome) the convention is
to use Util/Utils

[gone, 2015-09-22 18:49:55]

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

[gone, 2015-09-22 18:49:56]

If anything, doing it in onStartWithNative might be a little too early.  I
wanted to punt it as long as possible, but given that the thing is a big
AsyncTask now, I guess it won't matter too much.

+Yaron for OWNERS.

https://chromiumcodereview.appspot.com/1354323003/diff/40001/base/android/jav...
File base/android/java/src/org/chromium/base/FileUtilities.java (right):

https://chromiumcodereview.appspot.com/1354323003/diff/40001/base/android/jav...
base/android/java/src/org/chromium/base/FileUtilities.java:12: public class
FileUtilities {
On 2015/09/22 17:21:58, Lalit Maganti wrote:
> Out of curiousity: why Utilities in full? In Java (and Chrome) the convention
is
> to use Util/Utils

Former internal repo wonkiness... Renamed this one to FileUtils for base
consistency.

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

https://chromiumcodereview.appspot.com/1354323003/diff/40001/chrome/android/j...
chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappDirectoryManager.java:56:
assert Build.VERSION.SDK_INT >= Build.VERSION_CODES.LOLLIPOP;
On 2015/09/22 17:16:31, Peter Wen wrote:
> Is this assert useful since the boolean right above it is the exact same
value?

Yeah, not as useful as it used to be.  Removing.

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

https://chromiumcodereview.appspot.com/1354323003/diff/40001/chrome/android/j...
chrome/android/javatests/src/org/chromium/chrome/browser/webapps/WebappDirectoryManagerTest.java:26:
private static final String TAG = "cr.webapps.WebappDirectoryManagerTest";
On 2015/09/22 17:16:31, Peter Wen wrote:
> Probably doesn't matter for test code, but I think we're replacing cr.* with
> cr_*.

Done.

[Peter Wen, 2015-09-22 19:24:55]

One more small nit. :)

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

https://chromiumcodereview.appspot.com/1354323003/diff/60001/chrome/android/j...
chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappActivity.java:134:
if (mCleanupTask.getStatus() == AsyncTask.Status.PENDING)
mCleanupTask.execute(this);
Since no other AsyncTasks depend on cleanup finishing, this can be moved to
executeOnExecutor(AsyncTask.THREAD_POOL_EXECUTOR, this).

[Yaron, 2015-09-22 20:28:08]

FYI, I'll be OOO tomorrow so if you re-post comments I'll review on Thursday

https://chromiumcodereview.appspot.com/1354323003/diff/60001/base/android/jav...
File base/android/java/src/org/chromium/base/FileUtils.java (right):

https://chromiumcodereview.appspot.com/1354323003/diff/60001/base/android/jav...
base/android/java/src/org/chromium/base/FileUtils.java:19: if
(currentFile.isDirectory()) {
Assert that we aren't on UI thread?

https://chromiumcodereview.appspot.com/1354323003/diff/60001/base/android/jav...
base/android/java/src/org/chromium/base/FileUtils.java:30: } catch
(SecurityException e) {
Why is this added? Have you seen it?

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

https://chromiumcodereview.appspot.com/1354323003/diff/60001/chrome/android/j...
chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappActivity.java:134:
if (mCleanupTask.getStatus() == AsyncTask.Status.PENDING)
mCleanupTask.execute(this);
On 2015/09/22 19:24:55, Peter Wen wrote:
> Since no other AsyncTasks depend on cleanup finishing, this can be moved to
> executeOnExecutor(AsyncTask.THREAD_POOL_EXECUTOR, this).

+1

https://chromiumcodereview.appspot.com/1354323003/diff/60001/chrome/android/j...
File
chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappDirectoryManager.java
(right):

https://chromiumcodereview.appspot.com/1354323003/diff/60001/chrome/android/j...
chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappDirectoryManager.java:42:
private static final String TAG = "WebappDirectoryManag";
er

https://chromiumcodereview.appspot.com/1354323003/diff/60001/chrome/android/j...
File
chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappLauncherActivity.java
(right):

https://chromiumcodereview.appspot.com/1354323003/diff/60001/chrome/android/j...
chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappLauncherActivity.java:61:
if (Build.VERSION.SDK_INT < Build.VERSION_CODES.LOLLIPOP) {
So you can actually reach this on a tablet? I mean I guess we have webapps there
but I thought there isn't document mode

[gone, 2015-09-23 11:07:04]

https://chromiumcodereview.appspot.com/1354323003/diff/60001/base/android/jav...
File base/android/java/src/org/chromium/base/FileUtils.java (right):

https://chromiumcodereview.appspot.com/1354323003/diff/60001/base/android/jav...
base/android/java/src/org/chromium/base/FileUtils.java:19: if
(currentFile.isDirectory()) {
On 2015/09/22 20:28:08, Yaron (OOO 9.23) wrote:
> Assert that we aren't on UI thread?

Done.

https://chromiumcodereview.appspot.com/1354323003/diff/60001/base/android/jav...
base/android/java/src/org/chromium/base/FileUtils.java:30: } catch
(SecurityException e) {
On 2015/09/22 20:28:08, Yaron (OOO 9.23) wrote:
> Why is this added? Have you seen it?

Was worried about Android M throwing a fit with the new permissions model.  I
haven't seen it in practice, but I wouldn't be surprised if we end up seeing
something about it at some point in the crash stacks.  Can be swayed to take it
out but it seems safer to keep it, given that the API docs mention it can
happen.

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

https://chromiumcodereview.appspot.com/1354323003/diff/60001/chrome/android/j...
chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappActivity.java:134:
if (mCleanupTask.getStatus() == AsyncTask.Status.PENDING)
mCleanupTask.execute(this);
On 2015/09/22 20:28:08, Yaron (OOO 9.23) wrote:
> On 2015/09/22 19:24:55, Peter Wen wrote:
> > Since no other AsyncTasks depend on cleanup finishing, this can be moved to
> > executeOnExecutor(AsyncTask.THREAD_POOL_EXECUTOR, this).
> 
> +1

Done.

https://chromiumcodereview.appspot.com/1354323003/diff/60001/chrome/android/j...
File
chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappDirectoryManager.java
(right):

https://chromiumcodereview.appspot.com/1354323003/diff/60001/chrome/android/j...
chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappDirectoryManager.java:42:
private static final String TAG = "WebappDirectoryManag";
On 2015/09/22 20:28:08, Yaron (OOO 9.23) wrote:
> er

Doesn't fit; dgn@'s made the max 20 chars, now.

https://chromiumcodereview.appspot.com/1354323003/diff/60001/chrome/android/j...
File
chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappLauncherActivity.java
(right):

https://chromiumcodereview.appspot.com/1354323003/diff/60001/chrome/android/j...
chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappLauncherActivity.java:61:
if (Build.VERSION.SDK_INT < Build.VERSION_CODES.LOLLIPOP) {
On 2015/09/22 20:28:08, Yaron (OOO 9.23) wrote:
> So you can actually reach this on a tablet? I mean I guess we have webapps
there
> but I thought there isn't document mode

That's the funny thing; the check really should have always been "is on L" since
we really only need the Android APIs:

https://chrome-internal-review.googlesource.com/#/c/191492/3/java/apps/chrome...

For historical reasons, maybe the definition of eligible changed when we decided
against launching document mode on tablets?

AFAIK there's nothing specific to Clank's document mode that we really rely on
for web apps.

[gone, 2015-09-24 09:44:59]

Bots inexplicably keep crapping out on the tests.  Will need to take a look to
understand why.  Will poke the thread when I've got something substantial.

[gone, 2015-09-24 16:09:36]

Well, tests finally look alright, but I ended up refactoring a bunch more of the
class than I'd intended.  If you guys could take a look when you get a chance,
that'd be great.

[Lalit Maganti, 2015-09-24 16:12:16]

Yay for removing subclassing of AsyncTask from WebappDirectoyManager! Makes a
lot more sense that way :)

[Peter Wen, 2015-09-24 16:17:56]

Nice refactor!

https://chromiumcodereview.appspot.com/1354323003/diff/120001/chrome/android/...
File
chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappDirectoryManager.java
(right):

https://chromiumcodereview.appspot.com/1354323003/diff/120001/chrome/android/...
chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappDirectoryManager.java:59:
if (mCleanupTask != null) return mCleanupTask;
Do we need to deal with multiple concurrent cleanUpDirectories calls by creating
a list of cleanup tasks?

[gone, 2015-09-24 17:31:34]

https://chromiumcodereview.appspot.com/1354323003/diff/120001/chrome/android/...
File
chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappDirectoryManager.java
(right):

https://chromiumcodereview.appspot.com/1354323003/diff/120001/chrome/android/...
chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappDirectoryManager.java:59:
if (mCleanupTask != null) return mCleanupTask;
On 2015/09/24 16:17:56, Peter Wen wrote:
> Do we need to deal with multiple concurrent cleanUpDirectories calls by
creating
> a list of cleanup tasks?

I think we're alright:
1) Only one cleanup task will try to clean up old directories because of the
static AtomicBoolean
2) Any cleanup task after the first will just try to delete one particular
directory (the one for the WebappActivity that owns it).  The task never
explicitly tries to keep directories alive, unless the task is canceled.

Worst case is that the very first cleanup task deletes the directory for another
WebappActivity that starts up before the task finishes.  All that means is that
the web app will lose the TabState for the other web app, resulting in either:
1) The WebappActivity sends the user back to the web app's original URL
2) The WebappActivity is still running when its directory was deleted (which it
was going to do anyway), and saves out its TabState again when it is
backgrounded.

Easiest remedy is to make it a SERIAL_EXECUTOR task instead, but I don't think
it's really worth the hassle.

[Peter Wen, 2015-09-24 17:43:53]

On 2015/09/24 17:31:34, dfalcantara wrote:
> I think we're alright:
> 1) Only one cleanup task will try to clean up old directories because of the
> static AtomicBoolean
> 2) Any cleanup task after the first will just try to delete one particular
> directory (the one for the WebappActivity that owns it).  The task never
> explicitly tries to keep directories alive, unless the task is canceled.
> 
> Worst case is that the very first cleanup task deletes the directory for
another
> WebappActivity that starts up before the task finishes.  All that means is
that
> the web app will lose the TabState for the other web app, resulting in either:
> 1) The WebappActivity sends the user back to the web app's original URL
> 2) The WebappActivity is still running when its directory was deleted (which
it
> was going to do anyway), and saves out its TabState again when it is
> backgrounded.
> 
> Easiest remedy is to make it a SERIAL_EXECUTOR task instead, but I don't think
> it's really worth the hassle.

Agreed. It's a race condition that is both rare and with no real consequences,
so it's worth the speed increase as completing the task earlier (with the
threadpool) will even decrease the chance of the race condition occurring.

[Yaron, 2015-09-24 18:31:01]

https://chromiumcodereview.appspot.com/1354323003/diff/60001/base/android/jav...
File base/android/java/src/org/chromium/base/FileUtils.java (right):

https://chromiumcodereview.appspot.com/1354323003/diff/60001/base/android/jav...
base/android/java/src/org/chromium/base/FileUtils.java:30: } catch
(SecurityException e) {
On 2015/09/23 11:07:03, dfalcantara wrote:
> On 2015/09/22 20:28:08, Yaron (OOO 9.23) wrote:
> > Why is this added? Have you seen it?
> 
> Was worried about Android M throwing a fit with the new permissions model.  I
> haven't seen it in practice, but I wouldn't be surprised if we end up seeing
> something about it at some point in the crash stacks.  Can be swayed to take
it
> out but it seems safer to keep it, given that the API docs mention it can
> happen.

I don't see it in API docs. Perhaps I'm blind? I guess I'm fine with it staying.

https://chromiumcodereview.appspot.com/1354323003/diff/60001/chrome/android/j...
File
chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappDirectoryManager.java
(right):

https://chromiumcodereview.appspot.com/1354323003/diff/60001/chrome/android/j...
chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappDirectoryManager.java:42:
private static final String TAG = "WebappDirectoryManag";
On 2015/09/23 11:07:03, dfalcantara wrote:
> On 2015/09/22 20:28:08, Yaron (OOO 9.23) wrote:
> > er
> 
> Doesn't fit; dgn@'s made the max 20 chars, now.

Doh. I probably reviewed that too.. sgtm

https://chromiumcodereview.appspot.com/1354323003/diff/120001/chrome/android/...
File
chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappDirectoryManager.java
(right):

https://chromiumcodereview.appspot.com/1354323003/diff/120001/chrome/android/...
chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappDirectoryManager.java:32:
* Running this task should be called by WebappActivities after they have
restored all the data
This seems out of place. What "task"? Move to member variable/function?

https://chromiumcodereview.appspot.com/1354323003/diff/120001/chrome/android/...
chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappDirectoryManager.java:159:
// Temporarily allowing disk access while fixing. TODO: http://crbug.com/525781
Wait, wasn't the point of this to get rid of StrictMode violations. It's still
here. Is that the next CL?

[Yaron, 2015-09-24 19:30:45]

lgtm with prev comments

https://codereview.chromium.org/1354323003/diff/120001/chrome/android/java/sr...
File
chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappDirectoryManager.java
(right):

https://codereview.chromium.org/1354323003/diff/120001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappDirectoryManager.java:159:
// Temporarily allowing disk access while fixing. TODO: http://crbug.com/525781
On 2015/09/24 18:31:01, Yaron wrote:
> Wait, wasn't the point of this to get rid of StrictMode violations. It's still
> here. Is that the next CL?

Got it - one fixed, one more to go.

[gone, 2015-09-24 19:37:11]

https://codereview.chromium.org/1354323003/diff/120001/chrome/android/java/sr...
File
chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappDirectoryManager.java
(right):

https://codereview.chromium.org/1354323003/diff/120001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappDirectoryManager.java:32:
* Running this task should be called by WebappActivities after they have
restored all the data
On 2015/09/24 18:31:01, Yaron wrote:
> This seems out of place. What "task"? Move to member variable/function?

Moved it downward.  Make more sense?

https://codereview.chromium.org/1354323003/diff/120001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappDirectoryManager.java:159:
// Temporarily allowing disk access while fixing. TODO: http://crbug.com/525781
On 2015/09/24 18:31:01, Yaron wrote:
> Wait, wasn't the point of this to get rid of StrictMode violations. It's still
> here. Is that the next CL?

Got rid of one; can't get rid of this one at the moment.

[Yaron, 2015-09-24 19:45:21]

https://codereview.chromium.org/1354323003/diff/120001/chrome/android/java/sr...
File
chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappDirectoryManager.java
(right):

https://codereview.chromium.org/1354323003/diff/120001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappDirectoryManager.java:32:
* Running this task should be called by WebappActivities after they have
restored all the data
On 2015/09/24 19:37:11, dfalcantara wrote:
> On 2015/09/24 18:31:01, Yaron wrote:
> > This seems out of place. What "task"? Move to member variable/function?
> 
> Moved it downward.  Make more sense?

yes, thanks

[gone, 2015-09-25 10:18:17]

The CQ bit was checked by dfalcantara@chromium.org

[gone, 2015-09-25 10:18:18]

The patchset sent to the CQ was uploaded after l-g-t-m from wnwen@chromium.org,
yfriedman@chromium.org
Link to the patchset:
https://chromiumcodereview.appspot.com/1354323003/#ps160001 (title: " ")

[commit-bot: I haz the power, 2015-09-25 10:18:28]

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

[commit-bot: I haz the power, 2015-09-25 11:42:09]

Committed patchset #9 (id:160001)

[commit-bot: I haz the power, 2015-09-25 11:42:52]

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