Handle origin-specific password deletion in BrowsingDataRemover

Handle origin-specific password deletion in BrowsingDataRemover

If an origin is given to BrowsingDataRemover's Remove() method, we restrict
deletion by origin in addition to time. Otherwise, the existing, time-only
logic applies.

Test-wise, we extract the password setup code from RemovePasswordStatistics
into a new dedicated helper class called RemovePasswordsTester and reuse it for
the new test.

R=mkwst@chromium.org
BUG=113973
Committed: https://crrev.com/9a211cf16c8d22492026277a112b09c53142ac7c
Cr-Commit-Position: refs/heads/master@{#373768}

[Timo Reimann, 2016-02-01 10:07:20]

This is the final CL which builds upon the origin-specific deletion
functionality previously created for all password store implementations.

Mike, could you PTAL? Thanks!

https://codereview.chromium.org/1651913003/diff/1/chrome/browser/browsing_dat...
File chrome/browser/browsing_data/browsing_data_remover_unittest.cc (right):

https://codereview.chromium.org/1651913003/diff/1/chrome/browser/browsing_dat...
chrome/browser/browsing_data/browsing_data_remover_unittest.cc:888:
testing::NiceMock<password_manager::MockPasswordStore>>);
Note that I additionally wrapped the mocked password store into a NiceMock here
in order to prevent emitting warnings for NotifyLoginsChanged being called for
the new test. While this is specific to the new test, the Google Mock
documentation recommends to use NiceMocks by default[1]. Thus, it seemed fine to
me to put it into the general password helper class.

[1]
https://github.com/google/googlemock/blob/master/googlemock/docs/CookBook.md#...
(last paragraph)

[Mike West, 2016-02-04 18:25:12]

Thanks! Comments inline.

https://codereview.chromium.org/1651913003/diff/1/chrome/browser/browsing_dat...
File chrome/browser/browsing_data/browsing_data_remover.cc (right):

https://codereview.chromium.org/1651913003/diff/1/chrome/browser/browsing_dat...
chrome/browser/browsing_data/browsing_data_remover.cc:704: if
(remove_origin.unique())
Why "unique"? What if I pass in a `data:` URL, for example? I think this should
probably be checking something like `remove_url.empty()` or something similar,
if the goal is to clear out origin-based data iff an origin is provided.

Nit: Braces around multi-line blocks.

https://codereview.chromium.org/1651913003/diff/1/chrome/browser/browsing_dat...
chrome/browser/browsing_data/browsing_data_remover.cc:707: else
Nit: Ditto.

https://codereview.chromium.org/1651913003/diff/1/chrome/browser/browsing_dat...
File chrome/browser/browsing_data/browsing_data_remover_unittest.cc (right):

https://codereview.chromium.org/1651913003/diff/1/chrome/browser/browsing_dat...
chrome/browser/browsing_data/browsing_data_remover_unittest.cc:2200:
BrowsingDataRemover::REMOVE_PASSWORDS, kOrigin1);
Can you add a test that passes in a URL that produces a unique origin?
`data:text/html,...` for instance shouldn't actually remove any data from the
system.

[Timo Reimann, 2016-02-05 01:12:38]

https://codereview.chromium.org/1651913003/diff/1/chrome/browser/browsing_dat...
File chrome/browser/browsing_data/browsing_data_remover.cc (right):

https://codereview.chromium.org/1651913003/diff/1/chrome/browser/browsing_dat...
chrome/browser/browsing_data/browsing_data_remover.cc:704: if
(remove_origin.unique())
On 2016/02/04 18:25:11, Mike West (OOO until 2nd) wrote:
> Why "unique"? What if I pass in a `data:` URL, for example? I think this
should
> probably be checking something like `remove_url.empty()` or something similar,
> if the goal is to clear out origin-based data iff an origin is provided.

The "unique" check was meant as a temporary solution and supposed to be replaced
at some point in the future. See our discussion we had back in July [1] when I
added the functionality to remove downloads by origins. It also resulted in a
TODO for you placed at [2].

Checking the URL for emptiness does sound more reasonable though. Ideally, I'd
want a property of |remove_origin| as opposed to |remove_url| to be checked to
indicate origin-based deletion since the two variables could theoretically start
to deviate in the course of the (fairly biggish) RemoveImpl implementation
(e.g., |remove_url| getting reassigned). OTOH, |remove_url| is const, so we
should be fairly safe.
Thus, I replaced remove_origin.unique() with remove_url.is_empty(), constified
|remove_origin| for increased safety, and updated the header file comment.

Hope this matches what you had in mind.
 
> Nit: Braces around multi-line blocks.

Done.

[1] https://codereview.chromium.org/1251243003/patch/1/10003
[2]
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/bro...

https://codereview.chromium.org/1651913003/diff/1/chrome/browser/browsing_dat...
chrome/browser/browsing_data/browsing_data_remover.cc:707: else
On 2016/02/04 18:25:11, Mike West (OOO until 2nd) wrote:
> Nit: Ditto.

Done.

https://codereview.chromium.org/1651913003/diff/1/chrome/browser/browsing_dat...
File chrome/browser/browsing_data/browsing_data_remover_unittest.cc (right):

https://codereview.chromium.org/1651913003/diff/1/chrome/browser/browsing_dat...
chrome/browser/browsing_data/browsing_data_remover_unittest.cc:2200:
BrowsingDataRemover::REMOVE_PASSWORDS, kOrigin1);
On 2016/02/04 18:25:11, Mike West (OOO until 2nd) wrote:
> Can you add a test that passes in a URL that produces a unique origin?
> `data:text/html,...` for instance shouldn't actually remove any data from the
> system.

(Assuming you meant that it shouldn't remove any *origin-specific* data, I went
ahead and:)

Added tests for both the previously existing download case and the password one.
I'm passing in GURL() to account for the switch to checking for emptiness.

[Mike West, 2016-02-05 07:43:30]

LGTM, thank you.

[Timo Reimann, 2016-02-05 07:48:06]

The CQ bit was checked by ttr314@googlemail.com

[commit-bot: I haz the power, 2016-02-05 07:48:34]

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

[commit-bot: I haz the power, 2016-02-05 08:34:03]

Committed patchset #2 (id:20001)

[commit-bot: I haz the power, 2016-02-05 08:35:19]

Description was changed from

==========
Handle origin-specific password deletion in BrowsingDataRemover

If an origin is given to BrowsingDataRemover's Remove() method, we restrict
deletion by origin in addition to time. Otherwise, the existing, time-only
logic applies.

Test-wise, we extract the password setup code from RemovePasswordStatistics
into a new dedicated helper class called RemovePasswordsTester and reuse it for
the new test.

R=mkwst@chromium.org
BUG=113973
==========

to

==========
Handle origin-specific password deletion in BrowsingDataRemover

If an origin is given to BrowsingDataRemover's Remove() method, we restrict
deletion by origin in addition to time. Otherwise, the existing, time-only
logic applies.

Test-wise, we extract the password setup code from RemovePasswordStatistics
into a new dedicated helper class called RemovePasswordsTester and reuse it for
the new test.

R=mkwst@chromium.org
BUG=113973

Committed: https://crrev.com/9a211cf16c8d22492026277a112b09c53142ac7c
Cr-Commit-Position: refs/heads/master@{#373768}
==========

[commit-bot: I haz the power, 2016-02-05 08:35:20]

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