Clear webapp storage when site data is cleared

Clear webapp storage when site data is cleared

From a privacy perspective, all webapp data should be cleared
when site data is cleared by the user. Add the functionality
to do this.

Note: this CL does not delete the files themselves but no
identifying information about either the webapp or the user
is left in the SharedPreferences so there should be no problem.
This is because of a platform limitation which does not allow
SharedPreference consumers to delete the SharedPreference
file.

BUG=508627
Committed: https://crrev.com/afb72b4c970ae3a62777099822d81df36598cedc
Cr-Commit-Position: refs/heads/master@{#348370}

[Lalit Maganti, 2015-09-07 14:50:42]

lalitm@google.com changed reviewers:
+ dfalcantara@chromium.org, mlamouri@chromium.org

[Lalit Maganti, 2015-09-07 14:50:42]

Mounir and Dan: PTAL. I know I need to add tests for this but adding them across
JNI/thread boundaries is going to be tricky.

[mlamouri (slow - plz ping), 2015-09-08 10:03:00]

Except for some nits, it sounds good.

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

https://codereview.chromium.org/1329083002/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappDataStorage.java:90:
static void delete(final Context context, final String webappId) {
Shouldn't you add some method comments?

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

https://codereview.chromium.org/1329083002/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappRegistry.java:80:
public static void cleanupWebappsInBackground(Context context) {
nit: call it "unregisterAllWebapps" to keep it in sync with registerWebapp()?

https://codereview.chromium.org/1329083002/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappRegistry.java:86:
KEY_WEBAPP_SET, Collections.<String>emptySet());
These four lines seem to be a common pattern. Is there a way you can refactor
that?

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

https://codereview.chromium.org/1329083002/diff/20001/chrome/browser/browsing...
chrome/browser/browsing_data/browsing_data_remover.cc:770:
base::Bind(&WebappRegistry::CleanupWebappsOnIOThread,
nit: I would call the method ClearWebapps... or DeleteWebapps... or
UnregisterWebapps...

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

https://codereview.chromium.org/1329083002/diff/20001/chrome/browser/browsing...
chrome/browser/browsing_data/browsing_data_remover.h:85: REMOVE_WEBAPP_DATA = 1
<< 1,
You probably meant 1 << 19.

[mlamouri (slow - plz ping), 2015-09-08 10:03:14]

Oh, yeah, please, add tests :)

[Lalit Maganti, 2015-09-08 10:44:28]

Tests are in progress :)

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

https://codereview.chromium.org/1329083002/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappDataStorage.java:90:
static void delete(final Context context, final String webappId) {
On 2015/09/08 10:03:00, Mounir Lamouri wrote:
> Shouldn't you add some method comments?

I didn't because they are package private. I guess I could add some comment for
this method.

Done.

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

https://codereview.chromium.org/1329083002/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappRegistry.java:80:
public static void cleanupWebappsInBackground(Context context) {
On 2015/09/08 10:03:00, Mounir Lamouri wrote:
> nit: call it "unregisterAllWebapps" to keep it in sync with registerWebapp()?

Done but kept inBackground because of the assert.

https://codereview.chromium.org/1329083002/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappRegistry.java:86:
KEY_WEBAPP_SET, Collections.<String>emptySet());
On 2015/09/08 10:03:00, Mounir Lamouri wrote:
> These four lines seem to be a common pattern. Is there a way you can refactor
> that?

Done.

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

https://codereview.chromium.org/1329083002/diff/20001/chrome/browser/browsing...
chrome/browser/browsing_data/browsing_data_remover.cc:770:
base::Bind(&WebappRegistry::CleanupWebappsOnIOThread,
On 2015/09/08 10:03:00, Mounir Lamouri wrote:
> nit: I would call the method ClearWebapps... or DeleteWebapps... or
> UnregisterWebapps...

Gone with Unregister to match Java.

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

https://codereview.chromium.org/1329083002/diff/20001/chrome/browser/browsing...
chrome/browser/browsing_data/browsing_data_remover.h:85: REMOVE_WEBAPP_DATA = 1
<< 1,
On 2015/09/08 10:03:00, Mounir Lamouri wrote:
> You probably meant 1 << 19.

Huh. Not sure how the 9 got deleted. It was correct when I initially wrote the
patch :D

Fixed.

[gone, 2015-09-08 20:38:13]

I don't understand the data clearing bits well enough.  You'll need another
person to review that.

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

https://chromiumcodereview.appspot.com/1329083002/diff/60001/chrome/android/j...
chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappDataStorage.java:84:
* NOTE: Package private for use by WebappRegistry
nit: Remove the NOTE since it's useful.  Saying why a function
is visible is something that can be sussed out of the code review comments
and/or looking at where the function is used if the class ever has to be cleaned
up.  It's also one of those things that will go out of date really quickly.

https://chromiumcodereview.appspot.com/1329083002/diff/60001/chrome/android/j...
chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappDataStorage.java:88:
static void delete(final Context context, final String webappId) {
delete is too vague.  Sounds like you're deleting this WebappDataStorage.  Use
deleteDataForWebapp() or something.

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

https://chromiumcodereview.appspot.com/1329083002/diff/60001/chrome/android/j...
chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappRegistry.java:113:
public void onWebappIdsRetrieved(Set<String> readObject);
Move this public interface to the top.

https://chromiumcodereview.appspot.com/1329083002/diff/60001/chrome/android/j...
File
chrome/android/javatests/src/org/chromium/chrome/browser/preferences/privacy/ClearBrowsingDataDialogFragmentTest.java
(right):

https://chromiumcodereview.appspot.com/1329083002/diff/60001/chrome/android/j...
chrome/android/javatests/src/org/chromium/chrome/browser/preferences/privacy/ClearBrowsingDataDialogFragmentTest.java:30:
TestClearDataDialogFragment mFragment;
why isn't this private?

https://chromiumcodereview.appspot.com/1329083002/diff/60001/chrome/android/j...
chrome/android/javatests/src/org/chromium/chrome/browser/preferences/privacy/ClearBrowsingDataDialogFragmentTest.java:35:

nit: delete the newline.

https://chromiumcodereview.appspot.com/1329083002/diff/60001/chrome/android/j...
chrome/android/javatests/src/org/chromium/chrome/browser/preferences/privacy/ClearBrowsingDataDialogFragmentTest.java:47:
ClearBrowsingDataDialogFragment.DialogOption.CLEAR_COOKIES_AND_SITE_DATA));
nit: indentation is wrong.

https://chromiumcodereview.appspot.com/1329083002/diff/60001/chrome/android/j...
chrome/android/javatests/src/org/chromium/chrome/browser/preferences/privacy/ClearBrowsingDataDialogFragmentTest.java:64:
Button clearButton = ThreadUtils.runOnUiThreadBlocking(new Callable<Button>() {
If you're already going through the trouble of running everything on the UI
thread, can't you just combine all of the things in this section?  The only
thing performClickOnMainSync does is call v.performClick().  You may as well put
it in the runOnUiThreadBlocking call.

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

https://chromiumcodereview.appspot.com/1329083002/diff/60001/chrome/android/j...
chrome/android/junit/src/org/chromium/chrome/browser/webapps/WebappRegistryTest.java:159:

check if you actually succeeded in registering here.  if it didn't, this test
would still pass, wouldn't it?

https://chromiumcodereview.appspot.com/1329083002/diff/60001/chrome/android/j...
chrome/android/junit/src/org/chromium/chrome/browser/webapps/WebappRegistryTest.java:181:

same as above.

https://chromiumcodereview.appspot.com/1329083002/diff/60001/chrome/browser/a...
File chrome/browser/android/webapps/webapp_registry.h (right):

https://chromiumcodereview.appspot.com/1329083002/diff/60001/chrome/browser/a...
chrome/browser/android/webapps/webapp_registry.h:17: 
DB thread or IO thread?  Take your pick and rename accordingly.

[Lalit Maganti, 2015-09-09 15:27:46]

Dan: I've rebased the patch since review because of issues on my end (only
affects jni and gyp files though). I also had to rework threading because of a
race condition which I noticed with the ClearBrowsingData test being flaky.
Using AsyncTasks mitigates the issue and it should now be fixed.

bauerb@: can you please take a look at the browsing_data_remover files?

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

https://chromiumcodereview.appspot.com/1329083002/diff/60001/chrome/android/j...
chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappDataStorage.java:84:
* NOTE: Package private for use by WebappRegistry
On 2015/09/08 20:38:13, dfalcantara wrote:
> nit: Remove the NOTE since it's useful.  Saying why a function
> is visible is something that can be sussed out of the code review comments
> and/or looking at where the function is used if the class ever has to be
cleaned
> up.  It's also one of those things that will go out of date really quickly.

Done.

https://chromiumcodereview.appspot.com/1329083002/diff/60001/chrome/android/j...
chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappDataStorage.java:88:
static void delete(final Context context, final String webappId) {
On 2015/09/08 20:38:13, dfalcantara wrote:
> delete is too vague.  Sounds like you're deleting this WebappDataStorage.  Use
> deleteDataForWebapp() or something.

Done.

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

https://chromiumcodereview.appspot.com/1329083002/diff/60001/chrome/android/j...
chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappRegistry.java:113:
public void onWebappIdsRetrieved(Set<String> readObject);
On 2015/09/08 20:38:13, dfalcantara wrote:
> Move this public interface to the top.

Done.

https://chromiumcodereview.appspot.com/1329083002/diff/60001/chrome/android/j...
File
chrome/android/javatests/src/org/chromium/chrome/browser/preferences/privacy/ClearBrowsingDataDialogFragmentTest.java
(right):

https://chromiumcodereview.appspot.com/1329083002/diff/60001/chrome/android/j...
chrome/android/javatests/src/org/chromium/chrome/browser/preferences/privacy/ClearBrowsingDataDialogFragmentTest.java:30:
TestClearDataDialogFragment mFragment;
On 2015/09/08 20:38:13, dfalcantara wrote:
> why isn't this private?

My mistake. Done.

https://chromiumcodereview.appspot.com/1329083002/diff/60001/chrome/android/j...
chrome/android/javatests/src/org/chromium/chrome/browser/preferences/privacy/ClearBrowsingDataDialogFragmentTest.java:35:

On 2015/09/08 20:38:13, dfalcantara wrote:
> nit: delete the newline.

Done.

https://chromiumcodereview.appspot.com/1329083002/diff/60001/chrome/android/j...
chrome/android/javatests/src/org/chromium/chrome/browser/preferences/privacy/ClearBrowsingDataDialogFragmentTest.java:47:
ClearBrowsingDataDialogFragment.DialogOption.CLEAR_COOKIES_AND_SITE_DATA));
On 2015/09/08 20:38:13, dfalcantara wrote:
> nit: indentation is wrong.

Done.

https://chromiumcodereview.appspot.com/1329083002/diff/60001/chrome/android/j...
chrome/android/javatests/src/org/chromium/chrome/browser/preferences/privacy/ClearBrowsingDataDialogFragmentTest.java:64:
Button clearButton = ThreadUtils.runOnUiThreadBlocking(new Callable<Button>() {
On 2015/09/08 20:38:13, dfalcantara wrote:
> If you're already going through the trouble of running everything on the UI
> thread, can't you just combine all of the things in this section?  The only
> thing performClickOnMainSync does is call v.performClick().  You may as well
put
> it in the runOnUiThreadBlocking call.

Done.

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

https://chromiumcodereview.appspot.com/1329083002/diff/60001/chrome/android/j...
chrome/android/junit/src/org/chromium/chrome/browser/webapps/WebappRegistryTest.java:159:

On 2015/09/08 20:38:13, dfalcantara wrote:
> check if you actually succeeded in registering here.  if it didn't, this test
> would still pass, wouldn't it?

Changed test to explictly add to registry in test file itself so it doesn't end
up like we're testing register as well as unregister in one method.

https://chromiumcodereview.appspot.com/1329083002/diff/60001/chrome/android/j...
chrome/android/junit/src/org/chromium/chrome/browser/webapps/WebappRegistryTest.java:181:

On 2015/09/08 20:38:13, dfalcantara wrote:
> same as above.

Ditto.

https://chromiumcodereview.appspot.com/1329083002/diff/60001/chrome/browser/a...
File chrome/browser/android/webapps/webapp_registry.h (right):

https://chromiumcodereview.appspot.com/1329083002/diff/60001/chrome/browser/a...
chrome/browser/android/webapps/webapp_registry.h:17: 
On 2015/09/08 20:38:13, dfalcantara wrote:
> DB thread or IO thread?  Take your pick and rename accordingly.

This has been redone to make use of AsyncTask so no longer relevant.

[Lalit Maganti, 2015-09-09 16:47:55]

lalitm@google.com changed reviewers:
+ bauerb@chromium.org

[Lalit Maganti, 2015-09-09 16:47:56]

Actually +bauerb this time. Please see previous comment.

[gone, 2015-09-09 23:30:40]

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

https://chromiumcodereview.appspot.com/1329083002/diff/100001/chrome/android/...
chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappRegistry.java:33:
* Called when a retrieval of the stored web apps occurs.
nit: You used "webapp" in the added comment for WebappDataStorage, but you used
"web apps" on the comment here -- you should pick one.  Doesn't help that your
class JavaDoc says "webapps" but the JavaDoc for registerWebapp says "web app".

https://chromiumcodereview.appspot.com/1329083002/diff/100001/chrome/android/...
chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappRegistry.java:82:
/** Deletes the data of all webapps as well as the registry tracking the
webapps. */
nit:
replace "as well as" with 
", as well as" or "and"

Also, same comment about settling on either webapps or web apps for new edits.

https://chromiumcodereview.appspot.com/1329083002/diff/100001/chrome/android/...
chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappRegistry.java:121:
private static native void nativeOnWebappsUnregistered(long callbackPointer);
native call definitions always go at the bottom.

https://chromiumcodereview.appspot.com/1329083002/diff/100001/chrome/android/...
File
chrome/android/javatests/src/org/chromium/chrome/browser/preferences/privacy/ClearBrowsingDataDialogFragmentTest.java
(right):

https://chromiumcodereview.appspot.com/1329083002/diff/100001/chrome/android/...
chrome/android/javatests/src/org/chromium/chrome/browser/preferences/privacy/ClearBrowsingDataDialogFragmentTest.java:33:
mCallbackCalled = false;
nit: Remove. Java member booleans are default initialized to false.  That kind
of thing should be put in setUp(), anyway.

https://chromiumcodereview.appspot.com/1329083002/diff/100001/chrome/android/...
chrome/android/javatests/src/org/chromium/chrome/browser/preferences/privacy/ClearBrowsingDataDialogFragmentTest.java:46:

Confirm you actually registered the app before checking if it's gone.

https://chromiumcodereview.appspot.com/1329083002/diff/100001/chrome/android/...
chrome/android/javatests/src/org/chromium/chrome/browser/preferences/privacy/ClearBrowsingDataDialogFragmentTest.java:58:
}
nit: newline; start of new step in the test.

https://chromiumcodereview.appspot.com/1329083002/diff/100001/chrome/android/...
chrome/android/javatests/src/org/chromium/chrome/browser/preferences/privacy/ClearBrowsingDataDialogFragmentTest.java:73:
}));
nit: newline; new phase.

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

https://chromiumcodereview.appspot.com/1329083002/diff/100001/chrome/android/...
chrome/android/junit/src/org/chromium/chrome/browser/webapps/WebappRegistryTest.java:158:

You should actually check if the file exists before checking that it's gone. In
this case, you're not really registering something, so would the file even have
been made?

https://chromiumcodereview.appspot.com/1329083002/diff/100001/chrome/browser/...
File chrome/browser/android/webapps/webapp_registry.cc (right):

https://chromiumcodereview.appspot.com/1329083002/diff/100001/chrome/browser/...
chrome/browser/android/webapps/webapp_registry.cc:29: // Declared in JNI header
file.
nit: remove unhelpful "Declared in JNI header" comment

https://chromiumcodereview.appspot.com/1329083002/diff/100001/chrome/browser/...
chrome/browser/android/webapps/webapp_registry.cc:33: base::Closure* callback =
reinterpret_cast<base::Closure*>(jcallback);
again, my familiarity with native code is lacking, but is it possible for this
callback to have become invalid in between when the java call was made and when
this function is called?

https://chromiumcodereview.appspot.com/1329083002/diff/100001/chrome/browser/...
File chrome/browser/android/webapps/webapp_registry.h (right):

https://chromiumcodereview.appspot.com/1329083002/diff/100001/chrome/browser/...
chrome/browser/android/webapps/webapp_registry.h:19: // Cleans up data stored by
webapps.
nit: webapps web app yadda yadda

[Lalit Maganti, 2015-09-10 09:41:58]

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

https://codereview.chromium.org/1329083002/diff/100001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappRegistry.java:33:
* Called when a retrieval of the stored web apps occurs.
On 2015/09/09 23:30:39, dfalcantara wrote:
> nit: You used "webapp" in the added comment for WebappDataStorage, but you
used
> "web apps" on the comment here -- you should pick one.  Doesn't help that your
> class JavaDoc says "webapps" but the JavaDoc for registerWebapp says "web
app".

OK I choose "web app" for all text for consistency and correctness reasons. I'll
change any other instances of this I find.

https://codereview.chromium.org/1329083002/diff/100001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappRegistry.java:82:
/** Deletes the data of all webapps as well as the registry tracking the
webapps. */
On 2015/09/09 23:30:39, dfalcantara wrote:
> nit:
> replace "as well as" with 
> ", as well as" or "and"
> 
> Also, same comment about settling on either webapps or web apps for new edits.

Done.

https://codereview.chromium.org/1329083002/diff/100001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappRegistry.java:121:
private static native void nativeOnWebappsUnregistered(long callbackPointer);
On 2015/09/09 23:30:39, dfalcantara wrote:
> native call definitions always go at the bottom.

Done.

https://codereview.chromium.org/1329083002/diff/100001/chrome/android/javates...
File
chrome/android/javatests/src/org/chromium/chrome/browser/preferences/privacy/ClearBrowsingDataDialogFragmentTest.java
(right):

https://codereview.chromium.org/1329083002/diff/100001/chrome/android/javates...
chrome/android/javatests/src/org/chromium/chrome/browser/preferences/privacy/ClearBrowsingDataDialogFragmentTest.java:33:
mCallbackCalled = false;
On 2015/09/09 23:30:40, dfalcantara wrote:
> nit: Remove. Java member booleans are default initialized to false.  That kind
> of thing should be put in setUp(), anyway.

Done.

https://codereview.chromium.org/1329083002/diff/100001/chrome/android/javates...
chrome/android/javatests/src/org/chromium/chrome/browser/preferences/privacy/ClearBrowsingDataDialogFragmentTest.java:46:

On 2015/09/09 23:30:39, dfalcantara wrote:
> Confirm you actually registered the app before checking if it's gone.

Done.

https://codereview.chromium.org/1329083002/diff/100001/chrome/android/javates...
chrome/android/javatests/src/org/chromium/chrome/browser/preferences/privacy/ClearBrowsingDataDialogFragmentTest.java:58:
}
On 2015/09/09 23:30:39, dfalcantara wrote:
> nit: newline; start of new step in the test.

Done.

https://codereview.chromium.org/1329083002/diff/100001/chrome/android/javates...
chrome/android/javatests/src/org/chromium/chrome/browser/preferences/privacy/ClearBrowsingDataDialogFragmentTest.java:73:
}));
On 2015/09/09 23:30:39, dfalcantara wrote:
> nit: newline; new phase.

Done.

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

https://codereview.chromium.org/1329083002/diff/100001/chrome/android/junit/s...
chrome/android/junit/src/org/chromium/chrome/browser/webapps/WebappRegistryTest.java:158:

On 2015/09/09 23:30:40, dfalcantara wrote:
> You should actually check if the file exists before checking that it's gone.
In
> this case, you're not really registering something, so would the file even
have
> been made?

Yeah this test is sort of broken. Fixed.

https://codereview.chromium.org/1329083002/diff/100001/chrome/browser/android...
File chrome/browser/android/webapps/webapp_registry.cc (right):

https://codereview.chromium.org/1329083002/diff/100001/chrome/browser/android...
chrome/browser/android/webapps/webapp_registry.cc:29: // Declared in JNI header
file.
On 2015/09/09 23:30:40, dfalcantara wrote:
> nit: remove unhelpful "Declared in JNI header" comment

Done.

https://codereview.chromium.org/1329083002/diff/100001/chrome/browser/android...
chrome/browser/android/webapps/webapp_registry.cc:33: base::Closure* callback =
reinterpret_cast<base::Closure*>(jcallback);
On 2015/09/09 23:30:40, dfalcantara wrote:
> again, my familiarity with native code is lacking, but is it possible for this
> callback to have become invalid in between when the java call was made and
when
> this function is called?

AFAIK: no the pointer cannot be invalid until delete is called on it. And the
only place where delete is called is after the callback is run.

https://codereview.chromium.org/1329083002/diff/100001/chrome/browser/android...
File chrome/browser/android/webapps/webapp_registry.h (right):

https://codereview.chromium.org/1329083002/diff/100001/chrome/browser/android...
chrome/browser/android/webapps/webapp_registry.h:19: // Cleans up data stored by
webapps.
On 2015/09/09 23:30:40, dfalcantara wrote:
> nit: webapps web app yadda yadda

Done.

[Bernhard Bauer, 2015-09-10 09:46:24]

https://codereview.chromium.org/1329083002/diff/100001/chrome/browser/android...
File chrome/browser/android/webapps/webapp_registry.h (right):

https://codereview.chromium.org/1329083002/diff/100001/chrome/browser/android...
chrome/browser/android/webapps/webapp_registry.h:23: WebappRegistry() = delete;
DISALLOW_IMPLICIT_CONSTRUCTORS will do this (including
DISALLOW_COPY_AND_ASSIGN).

https://codereview.chromium.org/1329083002/diff/100001/chrome/browser/browsin...
File chrome/browser/browsing_data/browsing_data_remover.h (right):

https://codereview.chromium.org/1329083002/diff/100001/chrome/browser/browsin...
chrome/browser/browsing_data/browsing_data_remover.h:85: REMOVE_WEBAPP_DATA = 1
<< 19,
Would it make sense to only define this on Android?

[Lalit Maganti, 2015-09-10 10:01:38]

https://codereview.chromium.org/1329083002/diff/100001/chrome/browser/android...
File chrome/browser/android/webapps/webapp_registry.h (right):

https://codereview.chromium.org/1329083002/diff/100001/chrome/browser/android...
chrome/browser/android/webapps/webapp_registry.h:23: WebappRegistry() = delete;
On 2015/09/10 09:46:24, Bernhard Bauer wrote:
> DISALLOW_IMPLICIT_CONSTRUCTORS will do this (including
> DISALLOW_COPY_AND_ASSIGN).

Done.

https://codereview.chromium.org/1329083002/diff/100001/chrome/browser/browsin...
File chrome/browser/browsing_data/browsing_data_remover.h (right):

https://codereview.chromium.org/1329083002/diff/100001/chrome/browser/browsin...
chrome/browser/browsing_data/browsing_data_remover.h:85: REMOVE_WEBAPP_DATA = 1
<< 19,
On 2015/09/10 09:46:24, Bernhard Bauer wrote:
> Would it make sense to only define this on Android?

I guess it would actually. Changing the REMOVE_SITE_DATA to be ifdeffed might
prove a bit tricky with the formatting though.

Also other constants (e.g. REMOVE_PLUGIN_DATA) are used in only in ifdeffed
regions of code (e.g. ENABLE_PLUGINS) so there should be no harm in leaving as
is.

[mlamouri (slow - plz ping), 2015-09-10 11:45:35]

lgtm with comments applied.

Thanks! :)

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

https://codereview.chromium.org/1329083002/diff/180001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappDataStorage.java:82:
* Deletes the data for a webapp by clearing all the information inside the
SharedPreference
nit: if you change the other "webapp" to "web app", you might want to not add a
new one ;)
nit: SharedPreferences

https://codereview.chromium.org/1329083002/diff/180001/chrome/android/javates...
File
chrome/android/javatests/src/org/chromium/chrome/browser/preferences/privacy/ClearBrowsingDataDialogFragmentTest.java
(right):

https://codereview.chromium.org/1329083002/diff/180001/chrome/android/javates...
chrome/android/javatests/src/org/chromium/chrome/browser/preferences/privacy/ClearBrowsingDataDialogFragmentTest.java:5:
package org.chromium.chrome.browser.preferences.privacy;
I didn't review that file, I assume that Bernhard will.

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

https://codereview.chromium.org/1329083002/diff/180001/chrome/android/junit/s...
chrome/android/junit/src/org/chromium/chrome/browser/webapps/WebappRegistryTest.java:182:
Map<String, ?> actual = webAppPrefs.getAll();
Map<String, ?> is an interesting notation. It says how much I know about Java :)

https://codereview.chromium.org/1329083002/diff/180001/chrome/browser/browsin...
File chrome/browser/browsing_data/browsing_data_remover.h (right):

https://codereview.chromium.org/1329083002/diff/180001/chrome/browser/browsin...
chrome/browser/browsing_data/browsing_data_remover.h:92: // storage, webSQL,
service workers, cache storage, plugin data, and webapp
nit: web app?

[Lalit Maganti, 2015-09-10 16:37:02]

bauerb@: there are 3 failing tests because of the browsing_data_api for Chrome
extensions. However, this API is not even relevant to Android since there are no
extensions on Android. Should I fix the tests or add in the necessary
boilerplate?

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

https://codereview.chromium.org/1329083002/diff/180001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappDataStorage.java:82:
* Deletes the data for a webapp by clearing all the information inside the
SharedPreference
On 2015/09/10 11:45:35, Mounir Lamouri wrote:
> nit: if you change the other "webapp" to "web app", you might want to not add
a
> new one ;)
> nit: SharedPreferences

Done.

https://codereview.chromium.org/1329083002/diff/180001/chrome/android/javates...
File
chrome/android/javatests/src/org/chromium/chrome/browser/preferences/privacy/ClearBrowsingDataDialogFragmentTest.java
(right):

https://codereview.chromium.org/1329083002/diff/180001/chrome/android/javates...
chrome/android/javatests/src/org/chromium/chrome/browser/preferences/privacy/ClearBrowsingDataDialogFragmentTest.java:5:
package org.chromium.chrome.browser.preferences.privacy;
On 2015/09/10 11:45:35, Mounir Lamouri wrote:
> I didn't review that file, I assume that Bernhard will.

Acknowledged.

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

https://codereview.chromium.org/1329083002/diff/180001/chrome/android/junit/s...
chrome/android/junit/src/org/chromium/chrome/browser/webapps/WebappRegistryTest.java:182:
Map<String, ?> actual = webAppPrefs.getAll();
On 2015/09/10 11:45:35, Mounir Lamouri wrote:
> Map<String, ?> is an interesting notation. It says how much I know about Java
:)

Yeah generics in Java can get insane. This is still pretty tame :P

https://codereview.chromium.org/1329083002/diff/180001/chrome/browser/browsin...
File chrome/browser/browsing_data/browsing_data_remover.h (right):

https://codereview.chromium.org/1329083002/diff/180001/chrome/browser/browsin...
chrome/browser/browsing_data/browsing_data_remover.h:92: // storage, webSQL,
service workers, cache storage, plugin data, and webapp
On 2015/09/10 11:45:35, Mounir Lamouri wrote:
> nit: web app?

Done.

[Bernhard Bauer, 2015-09-10 16:58:00]

On 2015/09/10 16:37:02, Lalit Maganti wrote:
> bauerb@: there are 3 failing tests because of the browsing_data_api for Chrome
> extensions. However, this API is not even relevant to Android since there are
no
> extensions on Android. Should I fix the tests or add in the necessary
> boilerplate?

What tests are those? I think the right thing to do here would be to not build
them if extensions are disabled.

[Lalit Maganti, 2015-09-10 17:07:23]

On 2015/09/10 16:58:00, Bernhard Bauer wrote:
> On 2015/09/10 16:37:02, Lalit Maganti wrote:
> > bauerb@: there are 3 failing tests because of the browsing_data_api for
Chrome
> > extensions. However, this API is not even relevant to Android since there
are
> no
> > extensions on Android. Should I fix the tests or add in the necessary
> > boilerplate?
> 
> What tests are those? I think the right thing to do here would be to not build
> them if extensions are disabled.

Tests located at
chrome/browser/extensions/api/browsing_data/browsing_data_test.cc. You're an
owner in the folder so it's you're decision to make :)

[Lalit Maganti, 2015-09-10 17:12:07]

On 2015/09/10 17:07:23, Lalit Maganti wrote:
> On 2015/09/10 16:58:00, Bernhard Bauer wrote:
> > On 2015/09/10 16:37:02, Lalit Maganti wrote:
> > > bauerb@: there are 3 failing tests because of the browsing_data_api for
> Chrome
> > > extensions. However, this API is not even relevant to Android since there
> are
> > no
> > > extensions on Android. Should I fix the tests or add in the necessary
> > > boilerplate?
> > 
> > What tests are those? I think the right thing to do here would be to not
build
> > them if extensions are disabled.
> 
> Tests located at
> chrome/browser/extensions/api/browsing_data/browsing_data_test.cc. You're an
> owner in the folder so it's you're decision to make :)

Oh BTW they're failing on desktop too where the tests should be enabled so
another option may be to ifdef like you suggested. That would also probably fix
the failures.

[Bernhard Bauer, 2015-09-10 19:13:00]

On 2015/09/10 17:12:07, Lalit Maganti wrote:
> On 2015/09/10 17:07:23, Lalit Maganti wrote:
> > On 2015/09/10 16:58:00, Bernhard Bauer wrote:
> > > On 2015/09/10 16:37:02, Lalit Maganti wrote:
> > > > bauerb@: there are 3 failing tests because of the browsing_data_api for
> > Chrome
> > > > extensions. However, this API is not even relevant to Android since
there
> > are
> > > no
> > > > extensions on Android. Should I fix the tests or add in the necessary
> > > > boilerplate?
> > > 
> > > What tests are those? I think the right thing to do here would be to not
> build
> > > them if extensions are disabled.
> > 
> > Tests located at
> > chrome/browser/extensions/api/browsing_data/browsing_data_test.cc. You're an
> > owner in the folder so it's you're decision to make :)
> 
> Oh BTW they're failing on desktop too where the tests should be enabled so
> another option may be to ifdef like you suggested. That would also probably
fix
> the failures.

Oh, yes, in that case I think we should do the #ifdef thing. It would be weird
if an Android-only feature has an effect on desktop :)

[Lalit Maganti, 2015-09-10 20:07:52]

On 2015/09/10 19:13:00, Bernhard Bauer wrote:
> On 2015/09/10 17:12:07, Lalit Maganti wrote:
> > On 2015/09/10 17:07:23, Lalit Maganti wrote:
> > > On 2015/09/10 16:58:00, Bernhard Bauer wrote:
> > > > On 2015/09/10 16:37:02, Lalit Maganti wrote:
> > > > > bauerb@: there are 3 failing tests because of the browsing_data_api
for
> > > Chrome
> > > > > extensions. However, this API is not even relevant to Android since
> there
> > > are
> > > > no
> > > > > extensions on Android. Should I fix the tests or add in the necessary
> > > > > boilerplate?
> > > > 
> > > > What tests are those? I think the right thing to do here would be to not
> > build
> > > > them if extensions are disabled.
> > > 
> > > Tests located at
> > > chrome/browser/extensions/api/browsing_data/browsing_data_test.cc. You're
an
> > > owner in the folder so it's you're decision to make :)
> > 
> > Oh BTW they're failing on desktop too where the tests should be enabled so
> > another option may be to ifdef like you suggested. That would also probably
> fix
> > the failures.
> 
> Oh, yes, in that case I think we should do the #ifdef thing. It would be weird
> if an Android-only feature has an effect on desktop :)

Done.

[gone, 2015-09-10 20:12:50]

lgtm @ bauerb

https://codereview.chromium.org/1329083002/diff/200001/chrome/android/java/sr...
File
chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappRegistry.java
(right):

https://codereview.chromium.org/1329083002/diff/200001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappRegistry.java:53:
// Update the last used time of the web app data storage so we can guarantee
that
web app data storage -> {@link WebappDataStorage}

[Lalit Maganti, 2015-09-10 20:57:10]

As well as the nit, I've had to make some changes to defeat FindBugs and
Proguard.

https://codereview.chromium.org/1329083002/diff/200001/chrome/android/java/sr...
File
chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappRegistry.java
(right):

https://codereview.chromium.org/1329083002/diff/200001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappRegistry.java:53:
// Update the last used time of the web app data storage so we can guarantee
that
On 2015/09/10 20:12:50, dfalcantara wrote:
> web app data storage -> {@link WebappDataStorage}

Done.

[Bernhard Bauer, 2015-09-10 21:33:47]

LGTM!

On 2015/09/10 20:57:10, Lalit Maganti wrote:
> As well as the nit, I've had to make some changes to defeat FindBugs and
> Proguard.

FindBugs and Proguard may only be temporarily silenced, but never defeated.

[Lalit Maganti, 2015-09-11 09:24:55]

Last change was for a stupid mistake I made in the tests. Fixed and sending to
CQ.

[Lalit Maganti, 2015-09-11 09:25:00]

The CQ bit was checked by lalitm@google.com

[Lalit Maganti, 2015-09-11 09:25:00]

The patchset sent to the CQ was uploaded after l-g-t-m from
mlamouri@chromium.org, dfalcantara@chromium.org, bauerb@chromium.org
Link to the patchset: https://codereview.chromium.org/1329083002/#ps260001
(title: "Fix silly test mistake")

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

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

[commit-bot: I haz the power, 2015-09-11 10:03:07]

Committed patchset #14 (id:260001)

[commit-bot: I haz the power, 2015-09-11 10:03:50]

Patchset 14 (id:??) landed as
https://crrev.com/afb72b4c970ae3a62777099822d81df36598cedc
Cr-Commit-Position: refs/heads/master@{#348370}

[commit-bot: I haz the power, 2015-09-23 12:21:00]

Patchset 14 (id:??) landed as
https://crrev.com/afb72b4c970ae3a62777099822d81df36598cedc
Cr-Commit-Position: refs/heads/master@{#348370}