Refactor BrowsingDataRemover to mock out WebappRegistry.

Refactor BrowsingDataRemover to mock out WebappRegistry.

WebappRegistry makes JNI calls from the browsing data remover. This
interferes with C++ unit tests, as the Java side code is not initialised
in these tests. Any C++ unit test which calls through the JNI will hang
and time out.

This CL refactors the static methods in WebappRegistry into instance
methods, and adds a scoped pointer to WebappRegistry as a member on
BrowsingDataRemover. This allows the class to be mocked out at test time
to prevent the JNI call; thus browsing data remover tests which trigger
JNI calls can be run on Android. A follow-up CL will add a JNI call on
history removal to the browsing data remover; this pathway is currently
used in C++ unit tests and will hang without the mocking implemented in
this CL.

WebappRegistry only exists on Android, and there is no reason for it to
exist on a desktop build. Thus, this patch uses #if BUILDFLAG to ensure
the registry pointer and method calls are only present on Android.

BUG=570579
Committed: https://crrev.com/ce31ade9a645c56b5d5be16acffa6b8f81e8748e
Cr-Commit-Position: refs/heads/master@{#380541}

[dominickn, 2016-03-09 06:24:17]

Description was changed from

==========
Refactor BrowsingDataRemover to mock out WebappRegistry.

WebappRegistry makes JNI calls from the browsing data remover. This
interferes with C++ unit tests, as the Java side code is not initialised
in these tests. Any C++ unit test which calls through the JNI will hang
and time out.

This CL refactors the static methods in WebappRegistry into instance
methods, and adds a scoped pointer to WebappRegistry as a member on
BrowsingDataRemover. This allows the class to be mocked out at test time
to prevent the JNI call; thus browsing data remover tests which trigger
JNI calls can be run on Android. A follow-up CL will add a JNI call on
history removal to the browsing data remover; this pathway is currently
used in C++ unit tests and will hang without the mocking implemented in
this CL.

WebappRegistry only exists on Android, and there is no reason for it to
exist on a desktop build. Thus, this patch uses #if BUILDFLAG to ensure
the registry pointer and method calls are only present on Android.

BUG=570579
==========

to

==========
Refactor BrowsingDataRemover to mock out WebappRegistry.

WebappRegistry makes JNI calls from the browsing data remover. This
interferes with C++ unit tests, as the Java side code is not initialised
in these tests. Any C++ unit test which calls through the JNI will hang
and time out.

This CL refactors the static methods in WebappRegistry into instance
methods, and adds a scoped pointer to WebappRegistry as a member on
BrowsingDataRemover. This allows the class to be mocked out at test time
to prevent the JNI call; thus browsing data remover tests which trigger
JNI calls can be run on Android. A follow-up CL will add a JNI call on
history removal to the browsing data remover; this pathway is currently
used in C++ unit tests and will hang without the mocking implemented in
this CL.

WebappRegistry only exists on Android, and there is no reason for it to
exist on a desktop build. Thus, this patch uses #if BUILDFLAG to ensure
the registry pointer and method calls are only present on Android.

BUG=570579
==========

[dominickn, 2016-03-09 06:24:18]

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

[dominickn, 2016-03-09 06:25:33]

mkwst: PTAL at the browsing data remover

dfalcantara: PTAL at the webapp registry


Thanks! This is an enabling CL for https://codereview.chromium.org/1749603002.
Currently that CL times out on Android unit_tests due to the issue that this CL
is resolving.

[Mike West, 2016-03-09 07:18:37]

BDR LGTM. Thanks!

[gone, 2016-03-10 00:18:06]

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

https://chromiumcodereview.appspot.com/1776903002/diff/20001/chrome/browser/b...
chrome/browser/browsing_data/browsing_data_remover_unittest.cc:274:
callback.Run();
Should this be a delayed task, like with the real call?  This will immediately
run the callback on the same thread.

[dominickn, 2016-03-10 00:30:45]

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

https://codereview.chromium.org/1776903002/diff/20001/chrome/browser/browsing...
chrome/browser/browsing_data/browsing_data_remover_unittest.cc:274:
callback.Run();
On 2016/03/10 00:18:06, dfalcantara wrote:
> Should this be a delayed task, like with the real call?  This will immediately
> run the callback on the same thread.

My reasoning was that I wanted to block as little as possible in C++ unit tests.
This mocking should only happen in circumstances where the Java calls won't
work. Do you think this is valid?

[gone, 2016-03-10 00:45:07]

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

https://codereview.chromium.org/1776903002/diff/20001/chrome/browser/browsing...
chrome/browser/browsing_data/browsing_data_remover_unittest.cc:274:
callback.Run();
On 2016/03/10 00:30:45, dominickn wrote:
> On 2016/03/10 00:18:06, dfalcantara wrote:
> > Should this be a delayed task, like with the real call?  This will
immediately
> > run the callback on the same thread.
> 
> My reasoning was that I wanted to block as little as possible in C++ unit
tests.
> This mocking should only happen in circumstances where the Java calls won't
> work. Do you think this is valid?

Although the Java calls won't work, I'd normally argue that getting your mocked
class to work as similarly as possible could catch different behavior.  If, for
example, the real code did something non-trivial immediately when the mock
callback was called, this could make things occur in a different order than in
the real world.

In this case, however, I'm not sure there's much benefit with the current code
path because the BrowsingDataRemover is already waiting for other things
asynchronously.

So, your call.  I can swing either way.

[dominickn, 2016-03-10 01:48:22]

Acknowledged. I've changed it to use a delayed task, thanks!

mkwst, do you want to sanity check the change in BDR? And dfalcantara, any other
changes in the WebappRegistry?

[gone, 2016-03-10 01:50:38]

Nah, you're golden.  Thanks!

lgtm

[Mike West, 2016-03-10 08:45:40]

BDR still LGTM. :)

[dominickn, 2016-03-10 23:15:26]

The CQ bit was checked by dominickn@chromium.org

[commit-bot: I haz the power, 2016-03-10 23:16:36]

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

[commit-bot: I haz the power, 2016-03-11 00:27:58]

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

[commit-bot: I haz the power, 2016-03-11 00:27:59]

Try jobs failed on following builders:
  linux_chromium_asan_rel_ng on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[dominickn, 2016-03-11 00:30:24]

The CQ bit was checked by dominickn@chromium.org

[commit-bot: I haz the power, 2016-03-11 00:32:00]

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

[commit-bot: I haz the power, 2016-03-11 01:43:18]

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

[commit-bot: I haz the power, 2016-03-11 01:43:18]

Try jobs failed on following builders:
  linux_chromium_asan_rel_ng on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[dominickn, 2016-03-11 04:28:08]

The CQ bit was checked by dominickn@chromium.org

[commit-bot: I haz the power, 2016-03-11 04:28:57]

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

[commit-bot: I haz the power, 2016-03-11 05:39:34]

Description was changed from

==========
Refactor BrowsingDataRemover to mock out WebappRegistry.

WebappRegistry makes JNI calls from the browsing data remover. This
interferes with C++ unit tests, as the Java side code is not initialised
in these tests. Any C++ unit test which calls through the JNI will hang
and time out.

This CL refactors the static methods in WebappRegistry into instance
methods, and adds a scoped pointer to WebappRegistry as a member on
BrowsingDataRemover. This allows the class to be mocked out at test time
to prevent the JNI call; thus browsing data remover tests which trigger
JNI calls can be run on Android. A follow-up CL will add a JNI call on
history removal to the browsing data remover; this pathway is currently
used in C++ unit tests and will hang without the mocking implemented in
this CL.

WebappRegistry only exists on Android, and there is no reason for it to
exist on a desktop build. Thus, this patch uses #if BUILDFLAG to ensure
the registry pointer and method calls are only present on Android.

BUG=570579
==========

to

==========
Refactor BrowsingDataRemover to mock out WebappRegistry.

WebappRegistry makes JNI calls from the browsing data remover. This
interferes with C++ unit tests, as the Java side code is not initialised
in these tests. Any C++ unit test which calls through the JNI will hang
and time out.

This CL refactors the static methods in WebappRegistry into instance
methods, and adds a scoped pointer to WebappRegistry as a member on
BrowsingDataRemover. This allows the class to be mocked out at test time
to prevent the JNI call; thus browsing data remover tests which trigger
JNI calls can be run on Android. A follow-up CL will add a JNI call on
history removal to the browsing data remover; this pathway is currently
used in C++ unit tests and will hang without the mocking implemented in
this CL.

WebappRegistry only exists on Android, and there is no reason for it to
exist on a desktop build. Thus, this patch uses #if BUILDFLAG to ensure
the registry pointer and method calls are only present on Android.

BUG=570579
==========

[commit-bot: I haz the power, 2016-03-11 05:39:35]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2016-03-11 05:41:21]

Description was changed from

==========
Refactor BrowsingDataRemover to mock out WebappRegistry.

WebappRegistry makes JNI calls from the browsing data remover. This
interferes with C++ unit tests, as the Java side code is not initialised
in these tests. Any C++ unit test which calls through the JNI will hang
and time out.

This CL refactors the static methods in WebappRegistry into instance
methods, and adds a scoped pointer to WebappRegistry as a member on
BrowsingDataRemover. This allows the class to be mocked out at test time
to prevent the JNI call; thus browsing data remover tests which trigger
JNI calls can be run on Android. A follow-up CL will add a JNI call on
history removal to the browsing data remover; this pathway is currently
used in C++ unit tests and will hang without the mocking implemented in
this CL.

WebappRegistry only exists on Android, and there is no reason for it to
exist on a desktop build. Thus, this patch uses #if BUILDFLAG to ensure
the registry pointer and method calls are only present on Android.

BUG=570579
==========

to

==========
Refactor BrowsingDataRemover to mock out WebappRegistry.

WebappRegistry makes JNI calls from the browsing data remover. This
interferes with C++ unit tests, as the Java side code is not initialised
in these tests. Any C++ unit test which calls through the JNI will hang
and time out.

This CL refactors the static methods in WebappRegistry into instance
methods, and adds a scoped pointer to WebappRegistry as a member on
BrowsingDataRemover. This allows the class to be mocked out at test time
to prevent the JNI call; thus browsing data remover tests which trigger
JNI calls can be run on Android. A follow-up CL will add a JNI call on
history removal to the browsing data remover; this pathway is currently
used in C++ unit tests and will hang without the mocking implemented in
this CL.

WebappRegistry only exists on Android, and there is no reason for it to
exist on a desktop build. Thus, this patch uses #if BUILDFLAG to ensure
the registry pointer and method calls are only present on Android.

BUG=570579

Committed: https://crrev.com/ce31ade9a645c56b5d5be16acffa6b8f81e8748e
Cr-Commit-Position: refs/heads/master@{#380541}
==========

[commit-bot: I haz the power, 2016-03-11 05:41:22]

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