Support restricting browsing data removal for downloads by origin.

Support restricting browsing data removal for downloads by origin.

This CL adds support for restricting removal of downloads by origin. We do so
by extending DownloadManager's API and passing in the origin.

BUG=113969
TEST=added unit tests

Committed: https://crrev.com/81dc54b2ec32a33e6a7081f5537c53dc9863a153
Cr-Commit-Position: refs/heads/master@{#342187}

[Timo Reimann, 2015-07-22 19:30:18]

ttr314@googlemail.com changed reviewers:
+ mkwst@chromium.org, phajdan.jr@chromium.org

[Timo Reimann, 2015-07-22 19:41:21]

Please have a look at my CL and let me know if there's anything missing.

There's one testing-related issue I wasn't absolutely certain about: I decided
to place new tests with DownloadManagerTest (as opposed to
BrowsingDataRemoverTest) in order to allow for more concise, isolated testing
with less boilerplating involved. What's then missing in BrowsingDataRemoverTest
though is a verification that DownloadManager is called properly (i.e., with the
set of restricted URLs passed in).
We could build a RemoveDownloadTester helper class in BrowsingDataRemoverTest
similar to the other helpers; however, that would probably lead to duplication
of parts of the new tests in DownloadManagerTest. A mock of DownloadManager
where we can just verify that the manager is invoked with proper parameters
seems more appropriate to me. Unfortunately, I could not find an easy way to
inject a different DownloadManager into BrowsingDataRemover, even though a
mocked DownloadManager already exists (MockDownloadManager).

Any advice/pointers on how I should close the remaining test coverage gap (if
deemed necessary) would be appreciated.

[Mike West, 2015-07-22 20:15:46]

On 2015/07/22 at 19:41:21, ttr314 wrote:
> Please have a look at my CL and let me know if there's anything missing.

Looks pretty good! I left a few comments.

> Any advice/pointers on how I should close the remaining test coverage gap (if
deemed necessary) would be appreciated.

Could you override GetDownloadManager on
content/public/test/test_browser_context.h?

[Mike West, 2015-07-22 20:16:04]

Er, actually leaving comments.

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

https://codereview.chromium.org/1251243003/diff/1/chrome/browser/browsing_dat...
chrome/browser/browsing_data/browsing_data_remover.cc:254: const GURL& origin,
Please add a TODO to replace this with `url::Origin`.

https://codereview.chromium.org/1251243003/diff/1/chrome/browser/browsing_dat...
chrome/browser/browsing_data/browsing_data_remover.cc:291: std::set<GURL>
restrict_urls;
1. Why do we need a set here if we're adding a single URL?
2. Please use `url::Origin` for new code.
3. I know you're just moving this from it's current location, so you're not
responsible for the name, but it's bad. :) How about `origins_to_clear` or
something similar?

https://codereview.chromium.org/1251243003/diff/1/content/browser/download/do...
File content/browser/download/download_manager_impl.h (right):

https://codereview.chromium.org/1251243003/diff/1/content/browser/download/do...
content/browser/download/download_manager_impl.h:69: int
RemoveDownloadsBetween(const std::set<GURL>& restricted_urls,
Rename here as well (and I still don't understand why this is a set).

https://codereview.chromium.org/1251243003/diff/1/content/browser/download/do...
content/browser/download/download_manager_impl.h:148: bool IsRestrictedUrl(const
GURL& url,
1. You can move this to an static function in an anonymous namespace in the .cc
file. No need to expose it on the header.

2. If you change from 'restricted', you could rename this to something like
'IsRemovableURL' and simply return `true` if the set is empty rather than
DCHECKing.

https://codereview.chromium.org/1251243003/diff/1/content/public/browser/down...
File content/public/browser/download_manager.h (right):

https://codereview.chromium.org/1251243003/diff/1/content/public/browser/down...
content/public/browser/download_manager.h:114: // Remove downloads after
remove_begin (inclusive) and before remove_end
Poke at the comment to describe the new parameter.

[Timo Reimann, 2015-07-22 23:21:39]

On 2015/07/22 at 20:15:46, mkwst wrote:
> On 2015/07/22 at 19:41:21, ttr314 wrote:
> > Please have a look at my CL and let me know if there's anything missing.
> 
> Looks pretty good! I left a few comments.
> 
> > Any advice/pointers on how I should close the remaining test coverage gap
(if deemed necessary) would be appreciated.
> 
> Could you override GetDownloadManager on
content/public/test/test_browser_context.h?

Will have a look, thanks!

[Timo Reimann, 2015-07-22 23:22:15]

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

https://codereview.chromium.org/1251243003/diff/1/chrome/browser/browsing_dat...
chrome/browser/browsing_data/browsing_data_remover.cc:291: std::set<GURL>
restrict_urls;
On 2015/07/22 at 20:16:03, Mike West wrote:
> 1. Why do we need a set here if we're adding a single URL?

According to git history, the set was added back in 2010 and passed to the
history service unmodified
(https://chromium.googlesource.com/chromium/src/+/21f4d251210a4408da8a3279510e...).
My impression is that a set was chosen just to satisfy ExpireHistoryBetween()'s
contract easily.
One reason I can think of that would justify sticking to a set (or a container
in general) is that we may want to pass in multiple origins at one point. In
fact, the issue I plan to work on after #113621 (issue #78093) will require
passing in multiple origin filters in terms of match patterns, which seems to be
related to supporting multiple origins. WDYT?

> 2. Please use `url::Origin` for new code.

Could you please show me where that is defined? I was only able to find a TODO
of yours saying "Land 'url::Origin'" here:
https://code.google.com/p/chromium/codesearch#chromium/src/url/scheme_host_po...
.

> 3. I know you're just moving this from it's current location, so you're not
responsible for the name, but it's bad. :) How about `origins_to_clear` or
something similar?

Glad you mention it, I actually wanted to raise the subject as well. I find it a
bit hard to properly name a container/variable whose non-empty state indicates
"remove just these" while the empty one should mean "remove all" (as opposed to
"remove none"). Cannot really come up with something better though, so let's go
with `origins_to_clear`.

Will keep your other comments touching set and url::Origin changes on hold until
we have resolved the two so that I can make any changes in one go.

https://codereview.chromium.org/1251243003/diff/1/content/browser/download/do...
File content/browser/download/download_manager_impl.h (right):

https://codereview.chromium.org/1251243003/diff/1/content/browser/download/do...
content/browser/download/download_manager_impl.h:148: bool IsRestrictedUrl(const
GURL& url,
On 2015/07/22 at 20:16:03, Mike West wrote:
> 1. You can move this to an static function in an anonymous namespace in the
.cc file. No need to expose it on the header.

Done.

> 2. If you change from 'restricted', you could rename this to something like
'IsRemovableURL' and simply return `true` if the set is empty rather than
DCHECKing.

Had it like this at one point. Not really sure anymore why I changed it. Think I
found it to be more consistent with the other checks in
DownloadManagerImpl::RemoveDownloadsBetween, but design-wise I actually prefer
hiding the logic behind the function as well.
Will update as soon as we're clear on the set-vs-no-set matter.

[Mike West, 2015-07-23 09:56:50]

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

https://codereview.chromium.org/1251243003/diff/1/chrome/browser/browsing_dat...
chrome/browser/browsing_data/browsing_data_remover.cc:291: std::set<GURL>
restrict_urls;
On 2015/07/22 at 23:22:15, Timo Reimann wrote:
> On 2015/07/22 at 20:16:03, Mike West wrote:
> > 1. Why do we need a set here if we're adding a single URL?
> 
> According to git history, the set was added back in 2010 and passed to the
history service unmodified
(https://chromium.googlesource.com/chromium/src/+/21f4d251210a4408da8a3279510e...).
My impression is that a set was chosen just to satisfy ExpireHistoryBetween()'s
contract easily.
> One reason I can think of that would justify sticking to a set (or a container
in general) is that we may want to pass in multiple origins at one point. In
fact, the issue I plan to work on after #113621 (issue #78093) will require
passing in multiple origin filters in terms of match patterns, which seems to be
related to supporting multiple origins. WDYT?

If there's a reasonable use case for multiple origins, great, keep them. In this
file, however, we're passing in a single origin, and that's pretty much it. Is
there a use case _here_ for a set of origins?

It's not clear why you'll need match patterns for 78093, but I suppose we can
talk about that when we get there. :)

> > 2. Please use `url::Origin` for new code.
> 
> Could you please show me where that is defined? I was only able to find a TODO
of yours saying "Land 'url::Origin'" here:
https://code.google.com/p/chromium/codesearch#chromium/src/url/scheme_host_po...
.

I just landed it yesterday, so... :) It looks like CodeSearch hasn't caught up
yet; it's in `url/origin.h`, right next to `scheme_host_port.h`.

[Timo Reimann, 2015-07-23 23:11:07]

Patch #3 uploaded which replaces the set of GURLs by a single Origin. Adapted
and improved DownloadManagerTest as well, so this deserves another review round.

See my responses to your comments for details.

Thanks!

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

https://codereview.chromium.org/1251243003/diff/1/chrome/browser/browsing_dat...
chrome/browser/browsing_data/browsing_data_remover.cc:291: std::set<GURL>
restrict_urls;
On 2015/07/23 at 09:56:49, Mike West wrote:
> On 2015/07/22 at 23:22:15, Timo Reimann wrote:
> > On 2015/07/22 at 20:16:03, Mike West wrote:
> > > 1. Why do we need a set here if we're adding a single URL?
> > 
> > According to git history, the set was added back in 2010 and passed to the
history service unmodified
(https://chromium.googlesource.com/chromium/src/+/21f4d251210a4408da8a3279510e...).
My impression is that a set was chosen just to satisfy ExpireHistoryBetween()'s
contract easily.
> > One reason I can think of that would justify sticking to a set (or a
container in general) is that we may want to pass in multiple origins at one
point. In fact, the issue I plan to work on after #113621 (issue #78093) will
require passing in multiple origin filters in terms of match patterns, which
seems to be related to supporting multiple origins. WDYT?
> 
> If there's a reasonable use case for multiple origins, great, keep them. In
this file, however, we're passing in a single origin, and that's pretty much it.
Is there a use case _here_ for a set of origins?

As of now, only for history::HistoryService::ExpireLocalAndRemoteHistoryBetween
which expects a set. Otherwise, there's no need. I tried to model things after
the history service but realize now that we can keep it simple.

I moved the create-and-populate-set logic back to where it was before and pass a
single origin into DownloadManager::RemoveDownloadsBetween.

> It's not clear why you'll need match patterns for 78093, but I suppose we can
talk about that when we get there. :)

Sure! For the sake of completeness, here's the API proposal document I put
together a while ago:
https://docs.google.com/document/d/17hWuwDVG7OOuEfLfA59CChbLZrbfeJ_yr_PY03XoL...
. Feel free to have a look.

> > > 2. Please use `url::Origin` for new code.
> > 
> > Could you please show me where that is defined? I was only able to find a
TODO of yours saying "Land 'url::Origin'" here:
https://code.google.com/p/chromium/codesearch#chromium/src/url/scheme_host_po...
.
> 
> I just landed it yesterday, so... :) It looks like CodeSearch hasn't caught up
yet; it's in `url/origin.h`, right next to `scheme_host_port.h`.

Found it. Updated.

I also did a bit of renaming in BrowsingDataRemover to make sure that variable
names containing `origin` and `url` are actually Origin and GURL types,
respectively. Moreover, I removed the BrowsingDataRemover::remove_origin_ member
variable since it is never referenced outside of the
BrowsingDataRemover::RemoveImpl method, where it is redefined from the stack
immediately.

https://codereview.chromium.org/1251243003/diff/1/content/browser/download/do...
File content/browser/download/download_manager_impl.h (right):

https://codereview.chromium.org/1251243003/diff/1/content/browser/download/do...
content/browser/download/download_manager_impl.h:69: int
RemoveDownloadsBetween(const std::set<GURL>& restricted_urls,
On 2015/07/22 at 20:16:03, Mike West wrote:
> Rename here as well (and I still don't understand why this is a set).

Renamed and replaced the set by Origin.

https://codereview.chromium.org/1251243003/diff/1/content/browser/download/do...
content/browser/download/download_manager_impl.h:148: bool IsRestrictedUrl(const
GURL& url,
On 2015/07/22 at 23:22:15, Timo Reimann wrote:
> On 2015/07/22 at 20:16:03, Mike West wrote:
> > 2. If you change from 'restricted', you could rename this to something like
'IsRemovableURL' and simply return `true` if the set is empty rather than
DCHECKing.
> 
> Had it like this at one point. Not really sure anymore why I changed it. Think
I found it to be more consistent with the other checks in
DownloadManagerImpl::RemoveDownloadsBetween, but design-wise I actually prefer
hiding the logic behind the function as well.
> Will update as soon as we're clear on the set-vs-no-set matter.

Renamed and reimplemented the method. I use the unique (empty) origin to
represent the "clear all origins" state, and otherwise check for same origin. I
considered using pointers as well to define origin absence, but the unique state
approach seemed more appropriate to me. LMK what you think.

https://codereview.chromium.org/1251243003/diff/1/content/public/browser/down...
File content/public/browser/download_manager.h (right):

https://codereview.chromium.org/1251243003/diff/1/content/public/browser/down...
content/public/browser/download_manager.h:114: // Remove downloads after
remove_begin (inclusive) and before remove_end
On 2015/07/22 at 20:16:03, Mike West wrote:
> Poke at the comment to describe the new parameter.

Done.

[Mike West, 2015-07-24 06:56:39]

LGTM % nit.

Thanks!

https://codereview.chromium.org/1251243003/diff/1/content/browser/download/do...
File content/browser/download/download_manager_impl.h (right):

https://codereview.chromium.org/1251243003/diff/1/content/browser/download/do...
content/browser/download/download_manager_impl.h:148: bool IsRestrictedUrl(const
GURL& url,
On 2015/07/23 at 23:11:07, Timo Reimann wrote:
> On 2015/07/22 at 23:22:15, Timo Reimann wrote:
> > On 2015/07/22 at 20:16:03, Mike West wrote:
> > > 2. If you change from 'restricted', you could rename this to something
like 'IsRemovableURL' and simply return `true` if the set is empty rather than
DCHECKing.
> > 
> > Had it like this at one point. Not really sure anymore why I changed it.
Think I found it to be more consistent with the other checks in
DownloadManagerImpl::RemoveDownloadsBetween, but design-wise I actually prefer
hiding the logic behind the function as well.
> > Will update as soon as we're clear on the set-vs-no-set matter.
> 
> Renamed and reimplemented the method. I use the unique (empty) origin to
represent the "clear all origins" state, and otherwise check for same origin. I
considered using pointers as well to define origin absence, but the unique state
approach seemed more appropriate to me. LMK what you think.

This worries me a bit; we eventually want to build some sort of "Forget This
Site" button somewhere into the UI. If a user clicked that button on a sandboxed
site, I'd like to make sure we don't delete _all her data ever_. For the moment,
this is fine, but please add a comment to the `.h` file explaining the behavior,
with a `// TODO(mkwst): Reconsider this behavior if/when we build a "forget this
site" feature.` comment.

[Timo Reimann, 2015-07-24 07:10:45]

On 2015/07/24 at 06:56:39, mkwst wrote:
> LGTM % nit.
> 
> Thanks!
> 
>
https://codereview.chromium.org/1251243003/diff/1/content/browser/download/do...
> File content/browser/download/download_manager_impl.h (right):
> 
>
https://codereview.chromium.org/1251243003/diff/1/content/browser/download/do...
> content/browser/download/download_manager_impl.h:148: bool
IsRestrictedUrl(const GURL& url,
> On 2015/07/23 at 23:11:07, Timo Reimann wrote:
> > On 2015/07/22 at 23:22:15, Timo Reimann wrote:
> > > On 2015/07/22 at 20:16:03, Mike West wrote:
> > > > 2. If you change from 'restricted', you could rename this to something
like 'IsRemovableURL' and simply return `true` if the set is empty rather than
DCHECKing.
> > > 
> > > Had it like this at one point. Not really sure anymore why I changed it.
Think I found it to be more consistent with the other checks in
DownloadManagerImpl::RemoveDownloadsBetween, but design-wise I actually prefer
hiding the logic behind the function as well.
> > > Will update as soon as we're clear on the set-vs-no-set matter.
> > 
> > Renamed and reimplemented the method. I use the unique (empty) origin to
represent the "clear all origins" state, and otherwise check for same origin. I
considered using pointers as well to define origin absence, but the unique state
approach seemed more appropriate to me. LMK what you think.
> 
> This worries me a bit; we eventually want to build some sort of "Forget This
Site" button somewhere into the UI. If a user clicked that button on a sandboxed
site, I'd like to make sure we don't delete _all her data ever_. For the moment,
this is fine, but please add a comment to the `.h` file explaining the behavior,
with a `// TODO(mkwst): Reconsider this behavior if/when we build a "forget this
site" feature.` comment.

Maybe I'm missing something here, but wouldn't a click on such a button
eventually cause BrowsingDataRenover::RemoveImpl to be called with the origin of
the site in question, and thus make sure we delete just that site's browsing
data?

[Mike West, 2015-07-24 07:12:11]

On 2015/07/24 at 07:10:45, ttr314 wrote:
> On 2015/07/24 at 06:56:39, mkwst wrote:
> > LGTM % nit.
> > 
> > Thanks!
> > 
> >
https://codereview.chromium.org/1251243003/diff/1/content/browser/download/do...
> > File content/browser/download/download_manager_impl.h (right):
> > 
> >
https://codereview.chromium.org/1251243003/diff/1/content/browser/download/do...
> > content/browser/download/download_manager_impl.h:148: bool
IsRestrictedUrl(const GURL& url,
> > On 2015/07/23 at 23:11:07, Timo Reimann wrote:
> > > On 2015/07/22 at 23:22:15, Timo Reimann wrote:
> > > > On 2015/07/22 at 20:16:03, Mike West wrote:
> > > > > 2. If you change from 'restricted', you could rename this to something
like 'IsRemovableURL' and simply return `true` if the set is empty rather than
DCHECKing.
> > > > 
> > > > Had it like this at one point. Not really sure anymore why I changed it.
Think I found it to be more consistent with the other checks in
DownloadManagerImpl::RemoveDownloadsBetween, but design-wise I actually prefer
hiding the logic behind the function as well.
> > > > Will update as soon as we're clear on the set-vs-no-set matter.
> > > 
> > > Renamed and reimplemented the method. I use the unique (empty) origin to
represent the "clear all origins" state, and otherwise check for same origin. I
considered using pointers as well to define origin absence, but the unique state
approach seemed more appropriate to me. LMK what you think.
> > 
> > This worries me a bit; we eventually want to build some sort of "Forget This
Site" button somewhere into the UI. If a user clicked that button on a sandboxed
site, I'd like to make sure we don't delete _all her data ever_. For the moment,
this is fine, but please add a comment to the `.h` file explaining the behavior,
with a `// TODO(mkwst): Reconsider this behavior if/when we build a "forget this
site" feature.` comment.
> 
> Maybe I'm missing something here, but wouldn't a click on such a button
eventually cause BrowsingDataRenover::RemoveImpl to be called with the origin of
the site in question, and thus make sure we delete just that site's browsing
data?

Exactly. If the page is sandboxed into a unique origin (via
`Content-Security-Policy: sandbox`, for instance), then we'd send a unique
origin to the BrowsingDataRemover, which would cause data to be removed from
_all_ origins in the current implementation. That would be bad. :)

[Timo Reimann, 2015-07-24 07:22:10]

On 2015/07/24 at 07:12:11, mkwst wrote:
> On 2015/07/24 at 07:10:45, ttr314 wrote:
> > On 2015/07/24 at 06:56:39, mkwst wrote:
> > > LGTM % nit.
> > > 
> > > Thanks!
> > > 
> > >
https://codereview.chromium.org/1251243003/diff/1/content/browser/download/do...
> > > File content/browser/download/download_manager_impl.h (right):
> > > 
> > >
https://codereview.chromium.org/1251243003/diff/1/content/browser/download/do...
> > > content/browser/download/download_manager_impl.h:148: bool
IsRestrictedUrl(const GURL& url,
> > > On 2015/07/23 at 23:11:07, Timo Reimann wrote:
> > > > On 2015/07/22 at 23:22:15, Timo Reimann wrote:
> > > > > On 2015/07/22 at 20:16:03, Mike West wrote:
> > > > > > 2. If you change from 'restricted', you could rename this to
something like 'IsRemovableURL' and simply return `true` if the set is empty
rather than DCHECKing.
> > > > > 
> > > > > Had it like this at one point. Not really sure anymore why I changed
it. Think I found it to be more consistent with the other checks in
DownloadManagerImpl::RemoveDownloadsBetween, but design-wise I actually prefer
hiding the logic behind the function as well.
> > > > > Will update as soon as we're clear on the set-vs-no-set matter.
> > > > 
> > > > Renamed and reimplemented the method. I use the unique (empty) origin to
represent the "clear all origins" state, and otherwise check for same origin. I
considered using pointers as well to define origin absence, but the unique state
approach seemed more appropriate to me. LMK what you think.
> > > 
> > > This worries me a bit; we eventually want to build some sort of "Forget
This Site" button somewhere into the UI. If a user clicked that button on a
sandboxed site, I'd like to make sure we don't delete _all her data ever_. For
the moment, this is fine, but please add a comment to the `.h` file explaining
the behavior, with a `// TODO(mkwst): Reconsider this behavior if/when we build
a "forget this site" feature.` comment.
> > 
> > Maybe I'm missing something here, but wouldn't a click on such a button
eventually cause BrowsingDataRenover::RemoveImpl to be called with the origin of
the site in question, and thus make sure we delete just that site's browsing
data?
> 
> Exactly. If the page is sandboxed into a unique origin (via
`Content-Security-Policy: sandbox`, for instance), then we'd send a unique
origin to the BrowsingDataRemover, which would cause data to be removed from
_all_ origins in the current implementation. That would be bad. :)

Gotcha, that does sound bad. I didn't realize a sandboxed site corresponds to a
unique origin in this context.

As we still have a bunch of browsing data types left that need same-origin
removal restrictions too, would you still be fine with me applying the same
logic as I tackle the remaining types? That would lead to a fair number of
similar TODOs for you being left in the code, so I am wondering if you'd rather
prefer having a more general solution (such as the pointer idea or something
else) now. I'd be happy with taking either direction.

[Mike West, 2015-07-24 07:27:04]

On 2015/07/24 at 07:22:10, ttr314 wrote:
> On 2015/07/24 at 07:12:11, mkwst wrote:
> > On 2015/07/24 at 07:10:45, ttr314 wrote:
> > > On 2015/07/24 at 06:56:39, mkwst wrote:
> > > > LGTM % nit.
> > > > 
> > > > Thanks!
> > > > 
> > > >
https://codereview.chromium.org/1251243003/diff/1/content/browser/download/do...
> > > > File content/browser/download/download_manager_impl.h (right):
> > > > 
> > > >
https://codereview.chromium.org/1251243003/diff/1/content/browser/download/do...
> > > > content/browser/download/download_manager_impl.h:148: bool
IsRestrictedUrl(const GURL& url,
> > > > On 2015/07/23 at 23:11:07, Timo Reimann wrote:
> > > > > On 2015/07/22 at 23:22:15, Timo Reimann wrote:
> > > > > > On 2015/07/22 at 20:16:03, Mike West wrote:
> > > > > > > 2. If you change from 'restricted', you could rename this to
something like 'IsRemovableURL' and simply return `true` if the set is empty
rather than DCHECKing.
> > > > > > 
> > > > > > Had it like this at one point. Not really sure anymore why I changed
it. Think I found it to be more consistent with the other checks in
DownloadManagerImpl::RemoveDownloadsBetween, but design-wise I actually prefer
hiding the logic behind the function as well.
> > > > > > Will update as soon as we're clear on the set-vs-no-set matter.
> > > > > 
> > > > > Renamed and reimplemented the method. I use the unique (empty) origin
to represent the "clear all origins" state, and otherwise check for same origin.
I considered using pointers as well to define origin absence, but the unique
state approach seemed more appropriate to me. LMK what you think.
> > > > 
> > > > This worries me a bit; we eventually want to build some sort of "Forget
This Site" button somewhere into the UI. If a user clicked that button on a
sandboxed site, I'd like to make sure we don't delete _all her data ever_. For
the moment, this is fine, but please add a comment to the `.h` file explaining
the behavior, with a `// TODO(mkwst): Reconsider this behavior if/when we build
a "forget this site" feature.` comment.
> > > 
> > > Maybe I'm missing something here, but wouldn't a click on such a button
eventually cause BrowsingDataRenover::RemoveImpl to be called with the origin of
the site in question, and thus make sure we delete just that site's browsing
data?
> > 
> > Exactly. If the page is sandboxed into a unique origin (via
`Content-Security-Policy: sandbox`, for instance), then we'd send a unique
origin to the BrowsingDataRemover, which would cause data to be removed from
_all_ origins in the current implementation. That would be bad. :)
> 
> Gotcha, that does sound bad. I didn't realize a sandboxed site corresponds to
a unique origin in this context.
> 
> As we still have a bunch of browsing data types left that need same-origin
removal restrictions too, would you still be fine with me applying the same
logic as I tackle the remaining types? That would lead to a fair number of
similar TODOs for you being left in the code, so I am wondering if you'd rather
prefer having a more general solution (such as the pointer idea or something
else) now. I'd be happy with taking either direction.

A better option is probably to disallow passing a unique origin into
`::Remove()`, which would make it the feature's problem and not the
BrowsingDataRemover's problem. *shrug* Let's worry about it when it's a problem.
Let a million TODO(mkwst)'s bloom.

[Timo Reimann, 2015-07-24 07:42:20]

On 2015/07/24 at 07:27:04, mkwst wrote:
> On 2015/07/24 at 07:22:10, ttr314 wrote:
> > On 2015/07/24 at 07:12:11, mkwst wrote:
> > > On 2015/07/24 at 07:10:45, ttr314 wrote:
> > > > On 2015/07/24 at 06:56:39, mkwst wrote:
> > > > > LGTM % nit.
> > > > > 
> > > > > Thanks!
> > > > > 
> > > > >
https://codereview.chromium.org/1251243003/diff/1/content/browser/download/do...
> > > > > File content/browser/download/download_manager_impl.h (right):
> > > > > 
> > > > >
https://codereview.chromium.org/1251243003/diff/1/content/browser/download/do...
> > > > > content/browser/download/download_manager_impl.h:148: bool
IsRestrictedUrl(const GURL& url,
> > > > > On 2015/07/23 at 23:11:07, Timo Reimann wrote:
> > > > > > On 2015/07/22 at 23:22:15, Timo Reimann wrote:
> > > > > > > On 2015/07/22 at 20:16:03, Mike West wrote:
> > > > > > > > 2. If you change from 'restricted', you could rename this to
something like 'IsRemovableURL' and simply return `true` if the set is empty
rather than DCHECKing.
> > > > > > > 
> > > > > > > Had it like this at one point. Not really sure anymore why I
changed it. Think I found it to be more consistent with the other checks in
DownloadManagerImpl::RemoveDownloadsBetween, but design-wise I actually prefer
hiding the logic behind the function as well.
> > > > > > > Will update as soon as we're clear on the set-vs-no-set matter.
> > > > > > 
> > > > > > Renamed and reimplemented the method. I use the unique (empty)
origin to represent the "clear all origins" state, and otherwise check for same
origin. I considered using pointers as well to define origin absence, but the
unique state approach seemed more appropriate to me. LMK what you think.
> > > > > 
> > > > > This worries me a bit; we eventually want to build some sort of
"Forget This Site" button somewhere into the UI. If a user clicked that button
on a sandboxed site, I'd like to make sure we don't delete _all her data ever_.
For the moment, this is fine, but please add a comment to the `.h` file
explaining the behavior, with a `// TODO(mkwst): Reconsider this behavior
if/when we build a "forget this site" feature.` comment.
> > > > 
> > > > Maybe I'm missing something here, but wouldn't a click on such a button
eventually cause BrowsingDataRenover::RemoveImpl to be called with the origin of
the site in question, and thus make sure we delete just that site's browsing
data?
> > > 
> > > Exactly. If the page is sandboxed into a unique origin (via
`Content-Security-Policy: sandbox`, for instance), then we'd send a unique
origin to the BrowsingDataRemover, which would cause data to be removed from
_all_ origins in the current implementation. That would be bad. :)
> > 
> > Gotcha, that does sound bad. I didn't realize a sandboxed site corresponds
to a unique origin in this context.
> > 
> > As we still have a bunch of browsing data types left that need same-origin
removal restrictions too, would you still be fine with me applying the same
logic as I tackle the remaining types? That would lead to a fair number of
similar TODOs for you being left in the code, so I am wondering if you'd rather
prefer having a more general solution (such as the pointer idea or something
else) now. I'd be happy with taking either direction.
> 
> A better option is probably to disallow passing a unique origin into
`::Remove()`, which would make it the feature's problem and not the
BrowsingDataRemover's problem. *shrug* Let's worry about it when it's a problem.
Let a million TODO(mkwst)'s bloom.

:)

Just to be sure: Would you like me to leave the comment in download_manager.h or
browsing_data_remover.h?

[Mike West, 2015-07-24 07:42:48]

On 2015/07/24 at 07:42:20, ttr314 wrote:
> On 2015/07/24 at 07:27:04, mkwst wrote:
> > On 2015/07/24 at 07:22:10, ttr314 wrote:
> > > On 2015/07/24 at 07:12:11, mkwst wrote:
> > > > On 2015/07/24 at 07:10:45, ttr314 wrote:
> > > > > On 2015/07/24 at 06:56:39, mkwst wrote:
> > > > > > LGTM % nit.
> > > > > > 
> > > > > > Thanks!
> > > > > > 
> > > > > >
https://codereview.chromium.org/1251243003/diff/1/content/browser/download/do...
> > > > > > File content/browser/download/download_manager_impl.h (right):
> > > > > > 
> > > > > >
https://codereview.chromium.org/1251243003/diff/1/content/browser/download/do...
> > > > > > content/browser/download/download_manager_impl.h:148: bool
IsRestrictedUrl(const GURL& url,
> > > > > > On 2015/07/23 at 23:11:07, Timo Reimann wrote:
> > > > > > > On 2015/07/22 at 23:22:15, Timo Reimann wrote:
> > > > > > > > On 2015/07/22 at 20:16:03, Mike West wrote:
> > > > > > > > > 2. If you change from 'restricted', you could rename this to
something like 'IsRemovableURL' and simply return `true` if the set is empty
rather than DCHECKing.
> > > > > > > > 
> > > > > > > > Had it like this at one point. Not really sure anymore why I
changed it. Think I found it to be more consistent with the other checks in
DownloadManagerImpl::RemoveDownloadsBetween, but design-wise I actually prefer
hiding the logic behind the function as well.
> > > > > > > > Will update as soon as we're clear on the set-vs-no-set matter.
> > > > > > > 
> > > > > > > Renamed and reimplemented the method. I use the unique (empty)
origin to represent the "clear all origins" state, and otherwise check for same
origin. I considered using pointers as well to define origin absence, but the
unique state approach seemed more appropriate to me. LMK what you think.
> > > > > > 
> > > > > > This worries me a bit; we eventually want to build some sort of
"Forget This Site" button somewhere into the UI. If a user clicked that button
on a sandboxed site, I'd like to make sure we don't delete _all her data ever_.
For the moment, this is fine, but please add a comment to the `.h` file
explaining the behavior, with a `// TODO(mkwst): Reconsider this behavior
if/when we build a "forget this site" feature.` comment.
> > > > > 
> > > > > Maybe I'm missing something here, but wouldn't a click on such a
button eventually cause BrowsingDataRenover::RemoveImpl to be called with the
origin of the site in question, and thus make sure we delete just that site's
browsing data?
> > > > 
> > > > Exactly. If the page is sandboxed into a unique origin (via
`Content-Security-Policy: sandbox`, for instance), then we'd send a unique
origin to the BrowsingDataRemover, which would cause data to be removed from
_all_ origins in the current implementation. That would be bad. :)
> > > 
> > > Gotcha, that does sound bad. I didn't realize a sandboxed site corresponds
to a unique origin in this context.
> > > 
> > > As we still have a bunch of browsing data types left that need same-origin
removal restrictions too, would you still be fine with me applying the same
logic as I tackle the remaining types? That would lead to a fair number of
similar TODOs for you being left in the code, so I am wondering if you'd rather
prefer having a more general solution (such as the pointer idea or something
else) now. I'd be happy with taking either direction.
> > 
> > A better option is probably to disallow passing a unique origin into
`::Remove()`, which would make it the feature's problem and not the
BrowsingDataRemover's problem. *shrug* Let's worry about it when it's a problem.
Let a million TODO(mkwst)'s bloom.
> 
> :)
> 
> Just to be sure: Would you like me to leave the comment in download_manager.h
or browsing_data_remover.h?

BrowsingDataRemover. I'll never look in DownloadManager ever again in my entire
life. :)

[Timo Reimann, 2015-07-24 08:24:28]

Final patch set for what we've discussed so far: Adds a TODO and does some more
clean-up (missing renames, obsolete #include's).

Still need to add testing for the BrowsingDataRemover changes as described in my
first comment. I have an idea how to do that, will follow up shortly.

[Timo Reimann, 2015-07-24 08:28:30]

@phajdan.jr, would you like to have a look at the DownloadManager changes too?
Not sure if Mike's review is sufficient or whether an OWNER of DownloadManager
should chip in as well for process-related reasons.

Please let me know. Thanks!

[Mike West, 2015-07-24 08:33:57]

On 2015/07/24 at 08:28:30, ttr314 wrote:
> @phajdan.jr, would you like to have a look at the DownloadManager changes too?
Not sure if Mike's review is sufficient or whether an OWNER of DownloadManager
should chip in as well for process-related reasons.

Yes, you'll need an OWNER of DownloadManager to sign off on the patch before
landing it.

[Paweł Hajdan Jr., 2015-07-24 09:44:17]

https://codereview.chromium.org/1251243003/diff/60001/content/browser/downloa...
File content/browser/download/download_manager_impl.cc (right):

https://codereview.chromium.org/1251243003/diff/60001/content/browser/downloa...
content/browser/download/download_manager_impl.cc:592: if
(IsRemovableURL(download->GetURL(), origin_to_clear) &&
IsRemovableURL is located far from this method which makes reading the code
harder.

How about just expanding the logic here, e.g.:

if (!origin_to_clear.IsSameOriginWith(download->GetURL()) &&
!origin_to_clear.unique())
  continue;

... ?

[Timo Reimann, 2015-07-24 14:48:13]

On 2015/07/24 at 09:44:17, phajdan.jr wrote:
>
https://codereview.chromium.org/1251243003/diff/60001/content/browser/downloa...
> File content/browser/download/download_manager_impl.cc (right):
> 
>
https://codereview.chromium.org/1251243003/diff/60001/content/browser/downloa...
> content/browser/download/download_manager_impl.cc:592: if
(IsRemovableURL(download->GetURL(), origin_to_clear) &&
> IsRemovableURL is located far from this method which makes reading the code
harder.
> 
> How about just expanding the logic here, e.g.:
> 
> if (!origin_to_clear.IsSameOriginWith(download->GetURL()) &&
!origin_to_clear.unique())
>   continue;
> 
> ... ?

As IsSameOriginWith does not take a GURL, we'd need to wrap it in an Origin. The
resulting if statement would be

if (!origin_to_clear.IsSameOriginWith(url::Origin(download->GetURL())) &&
!origin_to_clear.unique())
  continue;

Personally, I find the inline approach a bit harder to read and understand due
to the double negations and the fact that iterating through the downloads now
involves two different kinds of logic. (Skip if one condition holds, remove if
another one does.)

What do you think about just moving the method closer to its invocation so that
the logic stays fairly readable but isn't too remote from the caller?

[Paweł Hajdan Jr., 2015-07-27 10:57:47]

On 2015/07/24 at 14:48:13, ttr314 wrote:
> On 2015/07/24 at 09:44:17, phajdan.jr wrote:
> >
https://codereview.chromium.org/1251243003/diff/60001/content/browser/downloa...
> > File content/browser/download/download_manager_impl.cc (right):
> > 
> >
https://codereview.chromium.org/1251243003/diff/60001/content/browser/downloa...
> > content/browser/download/download_manager_impl.cc:592: if
(IsRemovableURL(download->GetURL(), origin_to_clear) &&
> > IsRemovableURL is located far from this method which makes reading the code
harder.
> > 
> > How about just expanding the logic here, e.g.:
> > 
> > if (!origin_to_clear.IsSameOriginWith(download->GetURL()) &&
!origin_to_clear.unique())
> >   continue;
> > 
> > ... ?
> 
> As IsSameOriginWith does not take a GURL, we'd need to wrap it in an Origin.
The resulting if statement would be
> 
> if (!origin_to_clear.IsSameOriginWith(url::Origin(download->GetURL())) &&
!origin_to_clear.unique())
>   continue;
> 
> Personally, I find the inline approach a bit harder to read and understand due
to the double negations and the fact that iterating through the downloads now
involves two different kinds of logic. (Skip if one condition holds, remove if
another one does.)
> 
> What do you think about just moving the method closer to its invocation so
that the logic stays fairly readable but isn't too remote from the caller?

Okay, either of these would work for me.

[Timo Reimann, 2015-07-28 00:59:58]

On 2015/07/27 at 10:57:47, phajdan.jr wrote:
> On 2015/07/24 at 14:48:13, ttr314 wrote:
> > On 2015/07/24 at 09:44:17, phajdan.jr wrote:
> > >
https://codereview.chromium.org/1251243003/diff/60001/content/browser/downloa...
> > > File content/browser/download/download_manager_impl.cc (right):
> > > 
> > >
https://codereview.chromium.org/1251243003/diff/60001/content/browser/downloa...
> > > content/browser/download/download_manager_impl.cc:592: if
(IsRemovableURL(download->GetURL(), origin_to_clear) &&
> > > IsRemovableURL is located far from this method which makes reading the
code harder.
> > > 
> > > How about just expanding the logic here, e.g.:
> > > 
> > > if (!origin_to_clear.IsSameOriginWith(download->GetURL()) &&
!origin_to_clear.unique())
> > >   continue;
> > > 
> > > ... ?
> > 
> > As IsSameOriginWith does not take a GURL, we'd need to wrap it in an Origin.
The resulting if statement would be
> > 
> > if (!origin_to_clear.IsSameOriginWith(url::Origin(download->GetURL())) &&
!origin_to_clear.unique())
> >   continue;
> > 
> > Personally, I find the inline approach a bit harder to read and understand
due to the double negations and the fact that iterating through the downloads
now involves two different kinds of logic. (Skip if one condition holds, remove
if another one does.)
> > 
> > What do you think about just moving the method closer to its invocation so
that the logic stays fairly readable but isn't too remote from the caller?
> 
> Okay, either of these would work for me.

Alright, done.

[Timo Reimann, 2015-07-28 01:48:50]

ttr314@googlemail.com changed required reviewers:
+ mkwst@chromium.org

[Timo Reimann, 2015-07-28 01:48:50]

Added a final BrowsingDataRemoverTest to verify that the DownloadManager is
called with the right origin.

Apart from the fact that I'd welcome feedback on the general approach
(especially since it involves exposing a previously private key name), I have
the major issue that the test isn't working yet. I've spent a few hours in the
debugger and tried to figure out the root cause, but for the life of mine I
can't find the problem. Can someone please have a look and hopefully let me know
what's going wrong?

The basic idea is to add another tester to BrowsingDataRemoverTest that calls
`SaveUserData` on the testing profile in order to inject a mocked
DownloadManager. This seems to work well while I'm still setting up the test
fixture (see EXPECT call). However, once I am calling
`BrowserContext::GetDownloadManager(profile_)` from
`BrowsingDataRemover::RemoveImpl`, `GetUserData` won't return the mock anymore
and thus will cause the test to fail by a DCHECK. (See GTest output snippet
below.)
Curiously, I verified that the mock is actually still contained in the
`user_data_` DataMap. My vague guess is that the underlying linked_ptr is
somehow responsible for the observed behavior; then again, I could be totally
wrong.

Here's the relevant GTest output:

Note: Google Test filter = BrowsingDataRemoverTest.RemoveSameOriginDownloads
[==========] Running 1 test from 1 test case.
[----------] Global test environment set-up.
[----------] 1 test from BrowsingDataRemoverTest
[ RUN      ] BrowsingDataRemoverTest.RemoveSameOriginDownloads

[98675:1287:0728/033922:209732240863542:FATAL:browser_context.cc(118)] Check
failed: rdh. 
0   libbase.dylib                       0x0000000121d10e3f
base::debug::StackTrace::StackTrace() + 47
1   libbase.dylib                       0x0000000121d10e93
base::debug::StackTrace::StackTrace() + 35
2   libbase.dylib                       0x0000000121d733d3
logging::LogMessage::~LogMessage() + 67
3   libbase.dylib                       0x0000000121d72313
logging::LogMessage::~LogMessage() + 35
4   libcontent.dylib                    0x0000000126168a22
content::BrowserContext::GetDownloadManager(content::BrowserContext*) + 530
5   unit_tests                          0x000000010af45a66
BrowsingDataRemover::RemoveImpl(int, GURL const&, int) + 3654
6   unit_tests                          0x0000000108534b08
BrowsingDataRemoverTest::BlockUntilOriginDataRemoved(BrowsingDataRemover::TimePeriod,
int, GURL const&) + 248
7   unit_tests                          0x000000010852e668
BrowsingDataRemoverTest_RemoveSameOriginDownloads_Test::TestBody() + 280


Thanks in advance!

[Timo Reimann, 2015-07-30 14:00:30]

On 2015/07/28 at 01:48:50, Timo Reimann wrote:
> Added a final BrowsingDataRemoverTest to verify that the DownloadManager is
called with the right origin.
> 
> Apart from the fact that I'd welcome feedback on the general approach
(especially since it involves exposing a previously private key name), I have
the major issue that the test isn't working yet. I've spent a few hours in the
debugger and tried to figure out the root cause, but for the life of mine I
can't find the problem. Can someone please have a look and hopefully let me know
what's going wrong?
> 
> The basic idea is to add another tester to BrowsingDataRemoverTest that calls
`SaveUserData` on the testing profile in order to inject a mocked
DownloadManager. This seems to work well while I'm still setting up the test
fixture (see EXPECT call). However, once I am calling
`BrowserContext::GetDownloadManager(profile_)` from
`BrowsingDataRemover::RemoveImpl`, `GetUserData` won't return the mock anymore
and thus will cause the test to fail by a DCHECK. (See GTest output snippet
below.)
> Curiously, I verified that the mock is actually still contained in the
`user_data_` DataMap. My vague guess is that the underlying linked_ptr is
somehow responsible for the observed behavior; then again, I could be totally
wrong.
> 
> Here's the relevant GTest output:
> 
> Note: Google Test filter = BrowsingDataRemoverTest.RemoveSameOriginDownloads
> [==========] Running 1 test from 1 test case.
> [----------] Global test environment set-up.
> [----------] 1 test from BrowsingDataRemoverTest
> [ RUN      ] BrowsingDataRemoverTest.RemoveSameOriginDownloads
> 
> [98675:1287:0728/033922:209732240863542:FATAL:browser_context.cc(118)] Check
failed: rdh. 
> 0   libbase.dylib                       0x0000000121d10e3f
base::debug::StackTrace::StackTrace() + 47
> 1   libbase.dylib                       0x0000000121d10e93
base::debug::StackTrace::StackTrace() + 35
> 2   libbase.dylib                       0x0000000121d733d3
logging::LogMessage::~LogMessage() + 67
> 3   libbase.dylib                       0x0000000121d72313
logging::LogMessage::~LogMessage() + 35
> 4   libcontent.dylib                    0x0000000126168a22
content::BrowserContext::GetDownloadManager(content::BrowserContext*) + 530
> 5   unit_tests                          0x000000010af45a66
BrowsingDataRemover::RemoveImpl(int, GURL const&, int) + 3654
> 6   unit_tests                          0x0000000108534b08
BrowsingDataRemoverTest::BlockUntilOriginDataRemoved(BrowsingDataRemover::TimePeriod,
int, GURL const&) + 248
> 7   unit_tests                          0x000000010852e668
BrowsingDataRemoverTest_RemoveSameOriginDownloads_Test::TestBody() + 280
> 
> 
> Thanks in advance!


Did anyone have had a chance to look at my approach and the issue?

Glad for any feedback. Thanks!

[Mike West, 2015-07-31 15:23:49]

mkwst@chromium.org changed reviewers:
+ bauerb@chromium.org, markusheintz@chromium.org

[Mike West, 2015-07-31 15:23:49]

mkwst@chromium.org changed required reviewers:
- mkwst@chromium.org

[Mike West, 2015-07-31 15:23:49]

> Did anyone have had a chance to look at my approach and the issue?
> 
> Glad for any feedback. Thanks!

Sorry, I was out of office most of this week, and will be out all next week.
Summertime. :)

Perhaps Markus or Bernhard can help out with the test questions in the meantime?
CCing them both.

(removing my "*", though I think I've already approved the general approach)

[Bernhard Bauer, 2015-07-31 16:36:57]

On 2015/07/31 15:23:49, Mike West (OOO until 10th) wrote:
> > Did anyone have had a chance to look at my approach and the issue?
> > 
> > Glad for any feedback. Thanks!
> 
> Sorry, I was out of office most of this week, and will be out all next week.
> Summertime. :)
> 
> Perhaps Markus or Bernhard can help out with the test questions in the
meantime?
> CCing them both.

SupportsUserData actually takes const void* for the key, i.e. it looks at
addresses, not at string values that might be at those addresses. In addition,
you've defined the key as const in a header, and const variables by default get
internal linkage, which means you end up with a copy in each translation unit,
each with their own address.

(Really, those keys shouldn't be const char[], but just a void pointer pointing
to the variable itself, to prevent people from assuming their value is
relevant.)

But in this specific case, you also shouldn't expose the key as part of the
public content API at all, but instead add an actual method that can inject a
DownloadManager (and name the method ...ForTesting so that a presubmit check can
enforce it's only called in testing code).

[Bernhard Bauer, 2015-07-31 16:37:46]

bauerb@chromium.org changed required reviewers:
+ bauerb@chromium.org

[Timo Reimann, 2015-08-02 20:55:29]

ttr314@googlemail.com changed reviewers:
+ creis@chromium.org

[Timo Reimann, 2015-08-02 20:55:29]

ttr314@googlemail.com changed required reviewers:
+ creis@chromium.org

[Timo Reimann, 2015-08-02 20:57:24]

On 2015/07/31 at 16:36:57, bauerb wrote:
> On 2015/07/31 15:23:49, Mike West (OOO until 10th) wrote:
> > > Did anyone have had a chance to look at my approach and the issue?
> > > 
> > > Glad for any feedback. Thanks!
> > 
> > Sorry, I was out of office most of this week, and will be out all next week.
> > Summertime. :)
> > 
> > Perhaps Markus or Bernhard can help out with the test questions in the
meantime?
> > CCing them both.
> 
> SupportsUserData actually takes const void* for the key, i.e. it looks at
addresses, not at string values that might be at those addresses. In addition,
you've defined the key as const in a header, and const variables by default get
internal linkage, which means you end up with a copy in each translation unit,
each with their own address.
> 
> (Really, those keys shouldn't be const char[], but just a void pointer
pointing to the variable itself, to prevent people from assuming their value is
relevant.)
> 
> But in this specific case, you also shouldn't expose the key as part of the
public content API at all, but instead add an actual method that can inject a
DownloadManager (and name the method ...ForTesting so that a presubmit check can
enforce it's only called in testing code).

Thanks for the prompt help, Bernhard; shame on me for failing at C/C++ linkage.

I extended BrowserContext by a test helper method to inject a DownloadManager
instance (along with ChromeDownloadManagerDelegate to make the system-under-test
pass). The tests are green now.
GMock-matching an Origin actually turned out to be harder than I expected. I
decided to go with a custom matcher for reasons left as a comment in
BrowsingDataRemoverTest. Supposedly, we may want to move the matcher to a more
general location if the need arises to call it from other tests as well.
Bernhard, let me know if you think the approach is correct.

Added a content owner as additional reviewer. Charlie, could you please have a
look at the changes I made to browser_context.{h,cc} and download_manager.h?
Appreciated!

[Charlie Reis, 2015-08-03 18:33:19]

creis@chromium.org changed reviewers:
+ rdsmith@chromium.org

[Charlie Reis, 2015-08-03 18:33:19]

I'm afraid I'm not a good reviewer for DownloadManager.  rdsmith@, can you take
a look?

In general, there's a few style things to resolve.  I'd suggest running git cl
format over it to fix them automatically.

https://codereview.chromium.org/1251243003/diff/140001/content/browser/browse...
File content/browser/browser_context.cc (right):

https://codereview.chromium.org/1251243003/diff/140001/content/browser/browse...
content/browser/browser_context.cc:101: download_manager);
style nit: This should all fit on one line.

https://codereview.chromium.org/1251243003/diff/140001/content/browser/browse...
content/browser/browser_context.cc:319: BrowserContext* browser_context,
DownloadManager* download_manager) {
Style nit: "For function declarations and definitions, put each argument on a
separate line unless the whole declaration fits on one line."
http://www.chromium.org/developers/coding-style

[Randy Smith (Not in Mondays), 2015-08-03 19:09:30]

https://codereview.chromium.org/1251243003/diff/140001/content/public/browser...
File content/public/browser/download_manager.h (right):

https://codereview.chromium.org/1251243003/diff/140001/content/public/browser...
content/public/browser/download_manager.h:120: // unique origin causes all
origins to be removed.
I'm uncomfortable with using unique origins to indicate "all origins".  Could
you instead add a separate interface (maybe |RemoveDownloadByOrigin|) that just
removes origins that match the passed origin without the extra semantics for
unique origins?

[Timo Reimann, 2015-08-03 20:09:33]

Last patch just fixes formatting issues.

https://codereview.chromium.org/1251243003/diff/140001/content/public/browser...
File content/public/browser/download_manager.h (right):

https://codereview.chromium.org/1251243003/diff/140001/content/public/browser...
content/public/browser/download_manager.h:120: // unique origin causes all
origins to be removed.
On 2015/08/03 at 19:09:30, rdsmith wrote:
> I'm uncomfortable with using unique origins to indicate "all origins".  Could
you instead add a separate interface (maybe |RemoveDownloadByOrigin|) that just
removes origins that match the passed origin without the extra semantics for
unique origins?

Sounds legitimate; especially since mkwst raised similar concerns already.

Internally, I guess we'd want |RemoveDownloadsBetween| and
|RemoveDownloadsByOrigin| to be based on the same implementation with some means
to indicate absence/presence of an origin? If so, I suppose a simple pointer
(with NULL being absence and non-NULL being presence) could fit the purpose
unless we want to have something more sophisticated.

[Randy Smith (Not in Mondays), 2015-08-03 20:16:35]

On 2015/08/03 20:09:33, Timo Reimann wrote:
> Last patch just fixes formatting issues.
> 
>
https://codereview.chromium.org/1251243003/diff/140001/content/public/browser...
> File content/public/browser/download_manager.h (right):
> 
>
https://codereview.chromium.org/1251243003/diff/140001/content/public/browser...
> content/public/browser/download_manager.h:120: // unique origin causes all
> origins to be removed.
> On 2015/08/03 at 19:09:30, rdsmith wrote:
> > I'm uncomfortable with using unique origins to indicate "all origins". 
Could
> you instead add a separate interface (maybe |RemoveDownloadByOrigin|) that
just
> removes origins that match the passed origin without the extra semantics for
> unique origins?
> 
> Sounds legitimate; especially since mkwst raised similar concerns already.
> 
> Internally, I guess we'd want |RemoveDownloadsBetween| and
> |RemoveDownloadsByOrigin| to be based on the same implementation with some
means
> to indicate absence/presence of an origin? If so, I suppose a simple pointer
> (with NULL being absence and non-NULL being presence) could fit the purpose
> unless we want to have something more sophisticated.

I think I'd prefer a boolean, just because a pointer implies output parameter to
me, as opposed to just "may be null".  Alternatively, we could just take a test
function (either base::Bind() or function pointer) that takes a DownloadItemImpl
and returns a boolean.  

Part of me would like to reduce the public interface surface to a single
function following one of those patterns, but that's mission creep.  Heck, the
Remove* functions use only public interfaces, so we could actually just nuke all
of those functions and make the callers do the work using GetAllDownloads();
there aren't that many callers for this functionality.  However, that's *really*
mission creep.

So yeah, internal function with boolean or test function?

[Timo Reimann, 2015-08-03 22:00:00]

On 2015/08/03 at 20:16:35, rdsmith wrote:
> On 2015/08/03 20:09:33, Timo Reimann wrote:
> > Last patch just fixes formatting issues.
> > 
> >
https://codereview.chromium.org/1251243003/diff/140001/content/public/browser...
> > File content/public/browser/download_manager.h (right):
> > 
> >
https://codereview.chromium.org/1251243003/diff/140001/content/public/browser...
> > content/public/browser/download_manager.h:120: // unique origin causes all
> > origins to be removed.
> > On 2015/08/03 at 19:09:30, rdsmith wrote:
> > > I'm uncomfortable with using unique origins to indicate "all origins". 
Could
> > you instead add a separate interface (maybe |RemoveDownloadByOrigin|) that
just
> > removes origins that match the passed origin without the extra semantics for
> > unique origins?
> > 
> > Sounds legitimate; especially since mkwst raised similar concerns already.
> > 
> > Internally, I guess we'd want |RemoveDownloadsBetween| and
> > |RemoveDownloadsByOrigin| to be based on the same implementation with some
means
> > to indicate absence/presence of an origin? If so, I suppose a simple pointer
> > (with NULL being absence and non-NULL being presence) could fit the purpose
> > unless we want to have something more sophisticated.
> 
> I think I'd prefer a boolean, just because a pointer implies output parameter
to me, as opposed to just "may be null".  Alternatively, we could just take a
test function (either base::Bind() or function pointer) that takes a
DownloadItemImpl and returns a boolean.  
> 
> Part of me would like to reduce the public interface surface to a single
function following one of those patterns, but that's mission creep.  Heck, the
Remove* functions use only public interfaces, so we could actually just nuke all
of those functions and make the callers do the work using GetAllDownloads();
there aren't that many callers for this functionality.  However, that's *really*
mission creep.
> 
> So yeah, internal function with boolean or test function?

I guess a pointer is the closest built-in thing I can think of in C/C++ when it
comes to representing an optional. Unfortunately, it's a pretty bad one, let
alone due to its overloaded meanings.

(As a side consideration, I wonder if Chromium folks ever considered providing a
real monad implementation.)

How would the boolean approach work? As a flag to indicate whether a given
origin should be considered during removal or not?

I'm fine with tracking and maybe even actively pursuing a greater effort to
reduce the API's surface once I manage to land this CL. :-)

[Bernhard Bauer, 2015-08-04 07:49:20]

LGTM with a nit below:

On 2015/08/03 22:00:00, Timo Reimann wrote:
> On 2015/08/03 at 20:16:35, rdsmith wrote:
> > On 2015/08/03 20:09:33, Timo Reimann wrote:
> > > Last patch just fixes formatting issues.
> > > 
> > >
>
https://codereview.chromium.org/1251243003/diff/140001/content/public/browser...
> > > File content/public/browser/download_manager.h (right):
> > > 
> > >
>
https://codereview.chromium.org/1251243003/diff/140001/content/public/browser...
> > > content/public/browser/download_manager.h:120: // unique origin causes all
> > > origins to be removed.
> > > On 2015/08/03 at 19:09:30, rdsmith wrote:
> > > > I'm uncomfortable with using unique origins to indicate "all origins". 
> Could
> > > you instead add a separate interface (maybe |RemoveDownloadByOrigin|) that
> just
> > > removes origins that match the passed origin without the extra semantics
for
> > > unique origins?
> > > 
> > > Sounds legitimate; especially since mkwst raised similar concerns already.
> > > 
> > > Internally, I guess we'd want |RemoveDownloadsBetween| and
> > > |RemoveDownloadsByOrigin| to be based on the same implementation with some
> means
> > > to indicate absence/presence of an origin? If so, I suppose a simple
pointer
> > > (with NULL being absence and non-NULL being presence) could fit the
purpose
> > > unless we want to have something more sophisticated.
> > 
> > I think I'd prefer a boolean, just because a pointer implies output
parameter
> to me, as opposed to just "may be null".  Alternatively, we could just take a
> test function (either base::Bind() or function pointer) that takes a
> DownloadItemImpl and returns a boolean.  
> > 
> > Part of me would like to reduce the public interface surface to a single
> function following one of those patterns, but that's mission creep.  Heck, the
> Remove* functions use only public interfaces, so we could actually just nuke
all
> of those functions and make the callers do the work using GetAllDownloads();
> there aren't that many callers for this functionality.  However, that's
*really*
> mission creep.
> > 
> > So yeah, internal function with boolean or test function?
> 
> I guess a pointer is the closest built-in thing I can think of in C/C++ when
it
> comes to representing an optional. Unfortunately, it's a pretty bad one, let
> alone due to its overloaded meanings.
> 
> (As a side consideration, I wonder if Chromium folks ever considered providing
a
> real monad implementation.)

There is https://codereview.chromium.org/1245163002/ currently for review 😃

> How would the boolean approach work? As a flag to indicate whether a given
> origin should be considered during removal or not?
> 
> I'm fine with tracking and maybe even actively pursuing a greater effort to
> reduce the API's surface once I manage to land this CL. :-)

https://codereview.chromium.org/1251243003/diff/160001/content/browser/downlo...
File content/browser/download/download_manager_impl.cc (right):

https://codereview.chromium.org/1251243003/diff/160001/content/browser/downlo...
content/browser/download/download_manager_impl.cc:572: static bool
IsRemovableURL(const GURL& url,
Move this into an anonymous namespace instead of declaring it as static.

[Randy Smith (Not in Mondays), 2015-08-04 12:30:28]

On 2015/08/03 22:00:00, Timo Reimann wrote:
> On 2015/08/03 at 20:16:35, rdsmith wrote:
> > On 2015/08/03 20:09:33, Timo Reimann wrote:
> > > Last patch just fixes formatting issues.
> > > 
> > >
>
https://codereview.chromium.org/1251243003/diff/140001/content/public/browser...
> > > File content/public/browser/download_manager.h (right):
> > > 
> > >
>
https://codereview.chromium.org/1251243003/diff/140001/content/public/browser...
> > > content/public/browser/download_manager.h:120: // unique origin causes all
> > > origins to be removed.
> > > On 2015/08/03 at 19:09:30, rdsmith wrote:
> > > > I'm uncomfortable with using unique origins to indicate "all origins". 
> Could
> > > you instead add a separate interface (maybe |RemoveDownloadByOrigin|) that
> just
> > > removes origins that match the passed origin without the extra semantics
for
> > > unique origins?
> > > 
> > > Sounds legitimate; especially since mkwst raised similar concerns already.
> > > 
> > > Internally, I guess we'd want |RemoveDownloadsBetween| and
> > > |RemoveDownloadsByOrigin| to be based on the same implementation with some
> means
> > > to indicate absence/presence of an origin? If so, I suppose a simple
pointer
> > > (with NULL being absence and non-NULL being presence) could fit the
purpose
> > > unless we want to have something more sophisticated.
> > 
> > I think I'd prefer a boolean, just because a pointer implies output
parameter
> to me, as opposed to just "may be null".  Alternatively, we could just take a
> test function (either base::Bind() or function pointer) that takes a
> DownloadItemImpl and returns a boolean.  
> > 
> > Part of me would like to reduce the public interface surface to a single
> function following one of those patterns, but that's mission creep.  Heck, the
> Remove* functions use only public interfaces, so we could actually just nuke
all
> of those functions and make the callers do the work using GetAllDownloads();
> there aren't that many callers for this functionality.  However, that's
*really*
> mission creep.
> > 
> > So yeah, internal function with boolean or test function?
> 
> I guess a pointer is the closest built-in thing I can think of in C/C++ when
it
> comes to representing an optional. Unfortunately, it's a pretty bad one, let
> alone due to its overloaded meanings.
> 
> (As a side consideration, I wonder if Chromium folks ever considered providing
a
> real monad implementation.)
> 
> How would the boolean approach work? As a flag to indicate whether a given
> origin should be considered during removal or not?
> 
> I'm fine with tracking and maybe even actively pursuing a greater effort to
> reduce the API's surface once I manage to land this CL. :-)

Yes, boolean would be "pay attention to arg C only when arg B is true". 
Sub-ideal, but to my mind slightly less sub-ideal than a pointer.

[Timo Reimann, 2015-08-04 15:00:04]

On 2015/08/04 at 07:49:20, bauerb wrote:

> > (As a side consideration, I wonder if Chromium folks ever considered
providing a
> > real monad implementation.)
> 
> There is https://codereview.chromium.org/1245163002/ currently for review 😃

Sweet. :)

[Timo Reimann, 2015-08-04 15:00:26]

https://codereview.chromium.org/1251243003/diff/160001/content/browser/downlo...
File content/browser/download/download_manager_impl.cc (right):

https://codereview.chromium.org/1251243003/diff/160001/content/browser/downlo...
content/browser/download/download_manager_impl.cc:572: static bool
IsRemovableURL(const GURL& url,
On 2015/08/04 at 07:49:20, Bernhard Bauer wrote:
> Move this into an anonymous namespace instead of declaring it as static.

Originally, I had it in an anonymous namespace and accidentally moved it out
again when I addressed phajdan.jr's concern to locate the function closer to the
call site.

I could wrap it in an anonymous namespace again where it is now, but that would
lead to the file having two anonymous namespaces (one at the very top and
another one just to accommodate IsRemovableURL). Not sure if this is common
practice in Chromium though.

If it's not, then I suggest we wait until I have resolved rdsmith's points. This
will likely end up in another refactoring of DownloadManager and friends,
thereby possibly rendering the function in question obsolete. WDYT?

[Bernhard Bauer, 2015-08-04 15:05:06]

https://codereview.chromium.org/1251243003/diff/160001/content/browser/downlo...
File content/browser/download/download_manager_impl.cc (right):

https://codereview.chromium.org/1251243003/diff/160001/content/browser/downlo...
content/browser/download/download_manager_impl.cc:572: static bool
IsRemovableURL(const GURL& url,
On 2015/08/04 15:00:26, Timo Reimann wrote:
> On 2015/08/04 at 07:49:20, Bernhard Bauer wrote:
> > Move this into an anonymous namespace instead of declaring it as static.
> 
> Originally, I had it in an anonymous namespace and accidentally moved it out
> again when I addressed phajdan.jr's concern to locate the function closer to
the
> call site.
> 
> I could wrap it in an anonymous namespace again where it is now, but that
would
> lead to the file having two anonymous namespaces (one at the very top and
> another one just to accommodate IsRemovableURL). Not sure if this is common
> practice in Chromium though.
> 
> If it's not, then I suggest we wait until I have resolved rdsmith's points.
This
> will likely end up in another refactoring of DownloadManager and friends,
> thereby possibly rendering the function in question obsolete. WDYT?

I have seen separate anonymous namespaces; I don't think that's a big problem
(unless you subscribe to the notion that everything local to the file should
come before the stuff that's declared in the header, but that would directly
contradict what Paweł suggested, and it would equally apply to static vs.
anonymous namespace).

[Timo Reimann, 2015-08-04 15:13:24]

On 2015/08/04 at 12:30:28, rdsmith wrote:
> On 2015/08/03 22:00:00, Timo Reimann wrote:
> > On 2015/08/03 at 20:16:35, rdsmith wrote:
> > > On 2015/08/03 20:09:33, Timo Reimann wrote:
> > > > Last patch just fixes formatting issues.
> > > > 
> > > >
> >
https://codereview.chromium.org/1251243003/diff/140001/content/public/browser...
> > > > File content/public/browser/download_manager.h (right):
> > > > 
> > > >
> >
https://codereview.chromium.org/1251243003/diff/140001/content/public/browser...
> > > > content/public/browser/download_manager.h:120: // unique origin causes
all
> > > > origins to be removed.
> > > > On 2015/08/03 at 19:09:30, rdsmith wrote:
> > > > > I'm uncomfortable with using unique origins to indicate "all origins".

> > Could
> > > > you instead add a separate interface (maybe |RemoveDownloadByOrigin|)
that
> > just
> > > > removes origins that match the passed origin without the extra semantics
for
> > > > unique origins?
> > > > 
> > > > Sounds legitimate; especially since mkwst raised similar concerns
already.
> > > > 
> > > > Internally, I guess we'd want |RemoveDownloadsBetween| and
> > > > |RemoveDownloadsByOrigin| to be based on the same implementation with
some
> > means
> > > > to indicate absence/presence of an origin? If so, I suppose a simple
pointer
> > > > (with NULL being absence and non-NULL being presence) could fit the
purpose
> > > > unless we want to have something more sophisticated.
> > > 
> > > I think I'd prefer a boolean, just because a pointer implies output
parameter
> > to me, as opposed to just "may be null".  Alternatively, we could just take
a
> > test function (either base::Bind() or function pointer) that takes a
> > DownloadItemImpl and returns a boolean.  
> > > 
> > > Part of me would like to reduce the public interface surface to a single
> > function following one of those patterns, but that's mission creep.  Heck,
the
> > Remove* functions use only public interfaces, so we could actually just nuke
all
> > of those functions and make the callers do the work using GetAllDownloads();
> > there aren't that many callers for this functionality.  However, that's
*really*
> > mission creep.
> > > 
> > > So yeah, internal function with boolean or test function?
> > 
> > I guess a pointer is the closest built-in thing I can think of in C/C++ when
it
> > comes to representing an optional. Unfortunately, it's a pretty bad one, let
> > alone due to its overloaded meanings.
> > 
> > (As a side consideration, I wonder if Chromium folks ever considered
providing a
> > real monad implementation.)
> > 
> > How would the boolean approach work? As a flag to indicate whether a given
> > origin should be considered during removal or not?
> > 
> > I'm fine with tracking and maybe even actively pursuing a greater effort to
> > reduce the API's surface once I manage to land this CL. :-)
> 
> Yes, boolean would be "pay attention to arg C only when arg B is true". 
Sub-ideal, but to my mind slightly less sub-ideal than a pointer.

Got it. Personally, I'd likely prefer the test function approach as it seems to
come with more advantages in the long run. (Higher flexibility; enables the
caller to define custom logics; no need to maintain a variable that's only
relevant when another boolean says so.) It also seems nicer from a design
perspective.

The major downside I see is that it may be more complex to implement compared to
the boolean solution. To me, however, the benefits outweigh.

So test function?

[Timo Reimann, 2015-08-04 15:23:17]

Move IsRemovableURL into anonymous namespace.

[Timo Reimann, 2015-08-04 15:23:47]

On 2015/08/04 at 15:05:06, bauerb wrote:
>
https://codereview.chromium.org/1251243003/diff/160001/content/browser/downlo...
> File content/browser/download/download_manager_impl.cc (right):
> 
>
https://codereview.chromium.org/1251243003/diff/160001/content/browser/downlo...
> content/browser/download/download_manager_impl.cc:572: static bool
IsRemovableURL(const GURL& url,
> On 2015/08/04 15:00:26, Timo Reimann wrote:
> > On 2015/08/04 at 07:49:20, Bernhard Bauer wrote:
> > > Move this into an anonymous namespace instead of declaring it as static.
> > 
> > Originally, I had it in an anonymous namespace and accidentally moved it out
> > again when I addressed phajdan.jr's concern to locate the function closer to
the
> > call site.
> > 
> > I could wrap it in an anonymous namespace again where it is now, but that
would
> > lead to the file having two anonymous namespaces (one at the very top and
> > another one just to accommodate IsRemovableURL). Not sure if this is common
> > practice in Chromium though.
> > 
> > If it's not, then I suggest we wait until I have resolved rdsmith's points.
This
> > will likely end up in another refactoring of DownloadManager and friends,
> > thereby possibly rendering the function in question obsolete. WDYT?
> 
> I have seen separate anonymous namespaces; I don't think that's a big problem
(unless you subscribe to the notion that everything local to the file should
come before the stuff that's declared in the header, but that would directly
contradict what Paweł suggested, and it would equally apply to static vs.
anonymous namespace).

Makes sense.

Done.

[Randy Smith (Not in Mondays), 2015-08-04 15:27:33]

On 2015/08/04 15:13:24, Timo Reimann wrote:
> On 2015/08/04 at 12:30:28, rdsmith wrote:
> > On 2015/08/03 22:00:00, Timo Reimann wrote:
> > > On 2015/08/03 at 20:16:35, rdsmith wrote:
> > > > On 2015/08/03 20:09:33, Timo Reimann wrote:
> > > > > Last patch just fixes formatting issues.
> > > > > 
> > > > >
> > >
>
https://codereview.chromium.org/1251243003/diff/140001/content/public/browser...
> > > > > File content/public/browser/download_manager.h (right):
> > > > > 
> > > > >
> > >
>
https://codereview.chromium.org/1251243003/diff/140001/content/public/browser...
> > > > > content/public/browser/download_manager.h:120: // unique origin causes
> all
> > > > > origins to be removed.
> > > > > On 2015/08/03 at 19:09:30, rdsmith wrote:
> > > > > > I'm uncomfortable with using unique origins to indicate "all
origins".
> 
> > > Could
> > > > > you instead add a separate interface (maybe |RemoveDownloadByOrigin|)
> that
> > > just
> > > > > removes origins that match the passed origin without the extra
semantics
> for
> > > > > unique origins?
> > > > > 
> > > > > Sounds legitimate; especially since mkwst raised similar concerns
> already.
> > > > > 
> > > > > Internally, I guess we'd want |RemoveDownloadsBetween| and
> > > > > |RemoveDownloadsByOrigin| to be based on the same implementation with
> some
> > > means
> > > > > to indicate absence/presence of an origin? If so, I suppose a simple
> pointer
> > > > > (with NULL being absence and non-NULL being presence) could fit the
> purpose
> > > > > unless we want to have something more sophisticated.
> > > > 
> > > > I think I'd prefer a boolean, just because a pointer implies output
> parameter
> > > to me, as opposed to just "may be null".  Alternatively, we could just
take
> a
> > > test function (either base::Bind() or function pointer) that takes a
> > > DownloadItemImpl and returns a boolean.  
> > > > 
> > > > Part of me would like to reduce the public interface surface to a single
> > > function following one of those patterns, but that's mission creep.  Heck,
> the
> > > Remove* functions use only public interfaces, so we could actually just
nuke
> all
> > > of those functions and make the callers do the work using
GetAllDownloads();
> > > there aren't that many callers for this functionality.  However, that's
> *really*
> > > mission creep.
> > > > 
> > > > So yeah, internal function with boolean or test function?
> > > 
> > > I guess a pointer is the closest built-in thing I can think of in C/C++
when
> it
> > > comes to representing an optional. Unfortunately, it's a pretty bad one,
let
> > > alone due to its overloaded meanings.
> > > 
> > > (As a side consideration, I wonder if Chromium folks ever considered
> providing a
> > > real monad implementation.)
> > > 
> > > How would the boolean approach work? As a flag to indicate whether a given
> > > origin should be considered during removal or not?
> > > 
> > > I'm fine with tracking and maybe even actively pursuing a greater effort
to
> > > reduce the API's surface once I manage to land this CL. :-)
> > 
> > Yes, boolean would be "pay attention to arg C only when arg B is true". 
> Sub-ideal, but to my mind slightly less sub-ideal than a pointer.
> 
> Got it. Personally, I'd likely prefer the test function approach as it seems
to
> come with more advantages in the long run. (Higher flexibility; enables the
> caller to define custom logics; no need to maintain a variable that's only
> relevant when another boolean says so.) It also seems nicer from a design
> perspective.
> 
> The major downside I see is that it may be more complex to implement compared
to
> the boolean solution. To me, however, the benefits outweigh.
> 
> So test function?

Yep, I like it better too.  Thanks!

[Timo Reimann, 2015-08-04 22:29:25]

Add separate method to remove downloads by origin.

[Timo Reimann, 2015-08-04 22:36:30]

On 2015/08/04 at 15:27:33, rdsmith wrote:
> On 2015/08/04 15:13:24, Timo Reimann wrote:
> > On 2015/08/04 at 12:30:28, rdsmith wrote:
> > > On 2015/08/03 22:00:00, Timo Reimann wrote:
> > > > On 2015/08/03 at 20:16:35, rdsmith wrote:
> > > > > On 2015/08/03 20:09:33, Timo Reimann wrote:
> > > > > > Last patch just fixes formatting issues.
> > > > > > 
> > > > > >
> > > >
> >
https://codereview.chromium.org/1251243003/diff/140001/content/public/browser...
> > > > > > File content/public/browser/download_manager.h (right):
> > > > > > 
> > > > > >
> > > >
> >
https://codereview.chromium.org/1251243003/diff/140001/content/public/browser...
> > > > > > content/public/browser/download_manager.h:120: // unique origin
causes
> > all
> > > > > > origins to be removed.
> > > > > > On 2015/08/03 at 19:09:30, rdsmith wrote:
> > > > > > > I'm uncomfortable with using unique origins to indicate "all
origins".
> > 
> > > > Could
> > > > > > you instead add a separate interface (maybe
|RemoveDownloadByOrigin|)
> > that
> > > > just
> > > > > > removes origins that match the passed origin without the extra
semantics
> > for
> > > > > > unique origins?
> > > > > > 
> > > > > > Sounds legitimate; especially since mkwst raised similar concerns
> > already.
> > > > > > 
> > > > > > Internally, I guess we'd want |RemoveDownloadsBetween| and
> > > > > > |RemoveDownloadsByOrigin| to be based on the same implementation
with
> > some
> > > > means
> > > > > > to indicate absence/presence of an origin? If so, I suppose a simple
> > pointer
> > > > > > (with NULL being absence and non-NULL being presence) could fit the
> > purpose
> > > > > > unless we want to have something more sophisticated.
> > > > > 
> > > > > I think I'd prefer a boolean, just because a pointer implies output
> > parameter
> > > > to me, as opposed to just "may be null".  Alternatively, we could just
take
> > a
> > > > test function (either base::Bind() or function pointer) that takes a
> > > > DownloadItemImpl and returns a boolean.  
> > > > > 
> > > > > Part of me would like to reduce the public interface surface to a
single
> > > > function following one of those patterns, but that's mission creep. 
Heck,
> > the
> > > > Remove* functions use only public interfaces, so we could actually just
nuke
> > all
> > > > of those functions and make the callers do the work using
GetAllDownloads();
> > > > there aren't that many callers for this functionality.  However, that's
> > *really*
> > > > mission creep.
> > > > > 
> > > > > So yeah, internal function with boolean or test function?
> > > > 
> > > > I guess a pointer is the closest built-in thing I can think of in C/C++
when
> > it
> > > > comes to representing an optional. Unfortunately, it's a pretty bad one,
let
> > > > alone due to its overloaded meanings.
> > > > 
> > > > (As a side consideration, I wonder if Chromium folks ever considered
> > providing a
> > > > real monad implementation.)
> > > > 
> > > > How would the boolean approach work? As a flag to indicate whether a
given
> > > > origin should be considered during removal or not?
> > > > 
> > > > I'm fine with tracking and maybe even actively pursuing a greater effort
to
> > > > reduce the API's surface once I manage to land this CL. :-)
> > > 
> > > Yes, boolean would be "pay attention to arg C only when arg B is true". 
> > Sub-ideal, but to my mind slightly less sub-ideal than a pointer.
> > 
> > Got it. Personally, I'd likely prefer the test function approach as it seems
to
> > come with more advantages in the long run. (Higher flexibility; enables the
> > caller to define custom logics; no need to maintain a variable that's only
> > relevant when another boolean says so.) It also seems nicer from a design
> > perspective.
> > 
> > The major downside I see is that it may be more complex to implement
compared to
> > the boolean solution. To me, however, the benefits outweigh.
> > 
> > So test function?
> 
> Yep, I like it better too.  Thanks!

Alright, done. Hope this is roughly what you had in mind as well.

Basically, I added another public method |RemoveDownloadsByOriginAndTime| and
introduced two callbacks internally to cover the with/without origin use cases.
A future CL could theoretically expose the callback-based removal method
|RemoveDownloads(const DownloadRemover&)|, thereby enabling callers to define
removal constraints more flexibly.

Please let me know what you think. Thanks!

[Randy Smith (Not in Mondays), 2015-08-05 16:02:04]

*download* LGTM; comments below are suggestions you can take or not as you wish.
 Thanks!

https://codereview.chromium.org/1251243003/diff/200001/content/browser/downlo...
File content/browser/download/download_manager_impl.h (right):

https://codereview.chromium.org/1251243003/diff/200001/content/browser/downlo...
content/browser/download/download_manager_impl.h:12: #include "base/callback.h"
nit, suggestion: I think this could be callback_forward.h and you could just
include callback.h in the .cc file.

https://codereview.chromium.org/1251243003/diff/200001/content/browser/downlo...
File content/browser/download/download_manager_impl_unittest.cc (right):

https://codereview.chromium.org/1251243003/diff/200001/content/browser/downlo...
content/browser/download/download_manager_impl_unittest.cc:715:
EXPECT_CALL(GetMockDownloadItem(0), Remove());
Do you need a
::testing::Mock::VerifyAndClearExpectations(GetMockDownloadItem(0)) at some
point later in this function to make sure that this call actually happened?

[Timo Reimann, 2015-08-05 20:07:46]

https://codereview.chromium.org/1251243003/diff/200001/content/browser/downlo...
File content/browser/download/download_manager_impl.h (right):

https://codereview.chromium.org/1251243003/diff/200001/content/browser/downlo...
content/browser/download/download_manager_impl.h:12: #include "base/callback.h"
On 2015/08/05 at 16:02:04, rdsmith wrote:
> nit, suggestion: I think this could be callback_forward.h and you could just
include callback.h in the .cc file.

Nice, didn't know about callback_foward.h.

Done.

https://codereview.chromium.org/1251243003/diff/200001/content/browser/downlo...
File content/browser/download/download_manager_impl_unittest.cc (right):

https://codereview.chromium.org/1251243003/diff/200001/content/browser/downlo...
content/browser/download/download_manager_impl_unittest.cc:715:
EXPECT_CALL(GetMockDownloadItem(0), Remove());
On 2015/08/05 at 16:02:04, rdsmith wrote:
> Do you need a
::testing::Mock::VerifyAndClearExpectations(GetMockDownloadItem(0)) at some
point later in this function to make sure that this call actually happened?

I notice that none of the other (similar looking) tests is using
VerifyAndClearExpectations. Do you think it's something that just the new test
should do for some specific reason, or rather all tests in general? The existing
tests seem to rely on the mock fixture to do proper cleanup to ensure
expectation verification.

[Randy Smith (Not in Mondays), 2015-08-05 20:18:11]

On 2015/08/05 20:07:46, Timo Reimann wrote:
>
https://codereview.chromium.org/1251243003/diff/200001/content/browser/downlo...
> File content/browser/download/download_manager_impl.h (right):
> 
>
https://codereview.chromium.org/1251243003/diff/200001/content/browser/downlo...
> content/browser/download/download_manager_impl.h:12: #include
"base/callback.h"
> On 2015/08/05 at 16:02:04, rdsmith wrote:
> > nit, suggestion: I think this could be callback_forward.h and you could just
> include callback.h in the .cc file.
> 
> Nice, didn't know about callback_foward.h.
> 
> Done.
> 
>
https://codereview.chromium.org/1251243003/diff/200001/content/browser/downlo...
> File content/browser/download/download_manager_impl_unittest.cc (right):
> 
>
https://codereview.chromium.org/1251243003/diff/200001/content/browser/downlo...
> content/browser/download/download_manager_impl_unittest.cc:715:
> EXPECT_CALL(GetMockDownloadItem(0), Remove());
> On 2015/08/05 at 16:02:04, rdsmith wrote:
> > Do you need a
> ::testing::Mock::VerifyAndClearExpectations(GetMockDownloadItem(0)) at some
> point later in this function to make sure that this call actually happened?
> 
> I notice that none of the other (similar looking) tests is using
> VerifyAndClearExpectations. Do you think it's something that just the new test
> should do for some specific reason, or rather all tests in general? The
existing
> tests seem to rely on the mock fixture to do proper cleanup to ensure
> expectation verification.

I think it's mostly that I'm not on top of how the mock framework is used by the
chromium test framework; if the mock fixture is invoked at some point to verify
expectations (and I have a vague memory that you're right, it is) I'm happy.

[Timo Reimann, 2015-08-05 20:55:00]

The CQ bit was checked by ttr314@googlemail.com

[Timo Reimann, 2015-08-05 20:55:01]

The patchset sent to the CQ was uploaded after l-g-t-m from mkwst@chromium.org,
bauerb@chromium.org, rdsmith@chromium.org
Link to the patchset: https://codereview.chromium.org/1251243003/#ps220001
(title: "Use base/callback_forward.h.")

[Timo Reimann, 2015-08-05 20:55:08]

The CQ bit was unchecked by ttr314@googlemail.com

[Timo Reimann, 2015-08-05 20:55:56]

The CQ bit was checked by ttr314@googlemail.com

[commit-bot: I haz the power, 2015-08-05 20:57:19]

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

[commit-bot: I haz the power, 2015-08-05 20:57:20]

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

[commit-bot: I haz the power, 2015-08-05 20:57:21]

All required reviewers (with asterisk prefixes) have not yet approved this CL.

No L-G-T-M from a valid reviewer yet. Only full committers are accepted.
Even if an L-G-T-M may have been provided, it was from a non-committer,
_not_ a full super star committer.
See http://www.chromium.org/getting-involved/become-a-committer
Note that this has nothing to do with OWNERS files.

[Timo Reimann, 2015-08-05 20:58:37]

ttr314@googlemail.com changed required reviewers:
+ rdsmith@chromium.org
- creis@chromium.org

[Timo Reimann, 2015-08-05 21:00:29]

On 2015/08/05 at 20:57:21, commit-bot wrote:
> All required reviewers (with asterisk prefixes) have not yet approved this CL.
> 
> No L-G-T-M from a valid reviewer yet. Only full committers are accepted.
> Even if an L-G-T-M may have been provided, it was from a non-committer,
> _not_ a full super star committer.
> See http://www.chromium.org/getting-involved/become-a-committer
> Note that this has nothing to do with OWNERS files.

Moved required reviewer flag from creis to rdsmith since Charlie indicated he'd
hand reviewing over.

[Timo Reimann, 2015-08-05 21:02:33]

The CQ bit was checked by ttr314@googlemail.com to run a CQ dry run

[commit-bot: I haz the power, 2015-08-05 21:04:47]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1251243003/220001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1251243003/220001

[Charlie Reis, 2015-08-05 22:19:19]

You still need an owner's review.  Now that rdsmith has approved the changes, I
can take a look to rubber stamp the rest.

Just one question about the test setup, and I think it should be good to go.

https://codereview.chromium.org/1251243003/diff/220001/content/browser/browse...
File content/browser/browser_context.cc (right):

https://codereview.chromium.org/1251243003/diff/220001/content/browser/browse...
content/browser/browser_context.cc:97: DCHECK(context);
This one seems optional, since we'll crash on line 99 anyway if it's null.  I'm
ok if you want to keep it as documentation.

https://codereview.chromium.org/1251243003/diff/220001/content/browser/downlo...
File content/browser/download/download_manager_impl_unittest.cc (right):

https://codereview.chromium.org/1251243003/diff/220001/content/browser/downlo...
content/browser/download/download_manager_impl_unittest.cc:459:
download_urls.push_back(GURL("http://www.url4.com"));
It's generally not a good idea to set up state in the test constructor, since it
can cause one test to affect another.  Better to define download_urls_ as a
protected member and initialize it in SetUp (and clear it in TearDown) with all
the others.

[commit-bot: I haz the power, 2015-08-05 22:20:23]

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

[commit-bot: I haz the power, 2015-08-05 22:20:25]

Dry run: This issue passed the CQ dry run.

[Timo Reimann, 2015-08-05 22:32:57]

ttr314@googlemail.com changed required reviewers:
+ creis@chromium.org

[Timo Reimann, 2015-08-06 01:13:46]

Charlie, apologies for almost skipping your review. Somehow I was under the
impression that rdsmith was owner too -- my bad!

Please let me know if this is now ready to be sent into CQ, both code- and
review-wise. Thanks!

https://codereview.chromium.org/1251243003/diff/220001/content/browser/browse...
File content/browser/browser_context.cc (right):

https://codereview.chromium.org/1251243003/diff/220001/content/browser/browse...
content/browser/browser_context.cc:97: DCHECK(context);
On 2015/08/05 at 22:19:19, Charlie Reis wrote:
> This one seems optional, since we'll crash on line 99 anyway if it's null. 
I'm ok if you want to keep it as documentation.

I'm fine with removing it. Done.

https://codereview.chromium.org/1251243003/diff/220001/content/browser/downlo...
File content/browser/download/download_manager_impl_unittest.cc (right):

https://codereview.chromium.org/1251243003/diff/220001/content/browser/downlo...
content/browser/download/download_manager_impl_unittest.cc:459:
download_urls.push_back(GURL("http://www.url4.com"));
On 2015/08/05 at 22:19:19, Charlie Reis wrote:
> It's generally not a good idea to set up state in the test constructor, since
it can cause one test to affect another.  Better to define download_urls_ as a
protected member and initialize it in SetUp (and clear it in TearDown) with all
the others.

Done.

[Randy Smith (Not in Mondays), 2015-08-06 15:04:58]

Quick check: I only reviewed the *download* files.  If you're relying on my
opinion for the other files, let me know and I'll review them, but my stamp
wasn't intended to cover those files.

[Timo Reimann, 2015-08-06 15:13:11]

On 2015/08/06 at 15:04:58, rdsmith wrote:
> Quick check: I only reviewed the *download* files.  If you're relying on my
opinion for the other files, let me know and I'll review them, but my stamp
wasn't intended to cover those files.

mkwst and bauerb had a look at chrome/browser/browsing_data/*. With you having
reviewed download and Charlie covering the rest of content, we should be fine
from my understanding.

Let me know though if I missed something or you'd like to have another look. The
more, the merrier. :-)

[Charlie Reis, 2015-08-06 16:25:53]

Thanks!

On 2015/08/06 15:13:11, Timo Reimann wrote:
> On 2015/08/06 at 15:04:58, rdsmith wrote:
> > Quick check: I only reviewed the *download* files.  If you're relying on my
> opinion for the other files, let me know and I'll review them, but my stamp
> wasn't intended to cover those files.
> 
> mkwst and bauerb had a look at chrome/browser/browsing_data/*. With you having
> reviewed download and Charlie covering the rest of content, we should be fine
> from my understanding.
> 
> Let me know though if I missed something or you'd like to have another look.
The
> more, the merrier. :-)

That's correct.  I've reviewed browser_context.cc and content/public, and I'm
rubber stamping content/browser/download from rdsmith's review.  LGTM, and you
should be ready to CQ.

[Timo Reimann, 2015-08-06 17:18:06]

On 2015/08/06 at 16:25:53, creis wrote:
> Thanks!
> 
> On 2015/08/06 15:13:11, Timo Reimann wrote:
> > On 2015/08/06 at 15:04:58, rdsmith wrote:
> > > Quick check: I only reviewed the *download* files.  If you're relying on
my
> > opinion for the other files, let me know and I'll review them, but my stamp
> > wasn't intended to cover those files.
> > 
> > mkwst and bauerb had a look at chrome/browser/browsing_data/*. With you
having
> > reviewed download and Charlie covering the rest of content, we should be
fine
> > from my understanding.
> > 
> > Let me know though if I missed something or you'd like to have another look.
The
> > more, the merrier. :-)
> 
> That's correct.  I've reviewed browser_context.cc and content/public, and I'm
rubber stamping content/browser/download from rdsmith's review.  LGTM, and you
should be ready to CQ.

Great! Thanks for the support.

[Timo Reimann, 2015-08-06 17:18:13]

The CQ bit was checked by ttr314@googlemail.com

[Timo Reimann, 2015-08-06 17:18:13]

The patchset sent to the CQ was uploaded after l-g-t-m from bauerb@chromium.org,
rdsmith@chromium.org, mkwst@chromium.org
Link to the patchset: https://codereview.chromium.org/1251243003/#ps240001
(title: "Remove unneeded DCHECK; Initialize and clear download URLs along test
fixture life cycle.")

[commit-bot: I haz the power, 2015-08-06 17:19:34]

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

[commit-bot: I haz the power, 2015-08-06 18:10:40]

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

[commit-bot: I haz the power, 2015-08-06 18:10:42]

Try jobs failed on following builders:
  win_chromium_rel_ng on tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

[Timo Reimann, 2015-08-06 19:16:45]

The CQ bit was checked by ttr314@googlemail.com

[commit-bot: I haz the power, 2015-08-06 19:17:54]

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

[commit-bot: I haz the power, 2015-08-06 20:11:35]

Committed patchset #13 (id:240001)

[commit-bot: I haz the power, 2015-08-06 20:12:12]

Patchset 13 (id:??) landed as
https://crrev.com/81dc54b2ec32a33e6a7081f5537c53dc9863a153
Cr-Commit-Position: refs/heads/master@{#342187}

[battre, 2015-08-07 07:53:27]

battre@chromium.org changed reviewers:
+ battre@chromium.org

[battre, 2015-08-07 07:53:28]

Quick questions: Do we want to clear suborigins as well? I skimmed the CL and
did not notice a test that verifies that this happens or does not happen.

[Timo Reimann, 2015-08-08 12:49:08]

On 2015/08/07 at 07:53:28, battre wrote:
> Quick questions: Do we want to clear suborigins as well? I skimmed the CL and
did not notice a test that verifies that this happens or does not happen.

Assuming sub-origins and sub-domains are the same thing: I think it would
require extending Origin first (or choosing a completely different abstraction)
as the class seems to be able to match on exact hosts only.

It might be easier to focus on exact origins for now and add more flexibility
later. I am very very open though for exploring other directions as well.