initial version of the headless download manager delegate

initial version of the headless download manager delegate

BUG=696481

[Oleg Sushkov, 2017-05-17 05:11:18]

sushkov@chromium.org changed reviewers:
+ pfeldman@chromium.org, skyostil@chromium.org

[Oleg Sushkov, 2017-05-17 05:11:19]

I've implemented a basic headless download manager and corresponding dev-tools
commands that allow a devtools client to control the download behaviour. Later I
intend to add some devtools events triggered on downloads that would allow more
fine-grained control.

[Eric Seckler, 2017-05-17 09:44:44]

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

[Eric Seckler, 2017-05-17 09:44:45]

https://codereview.chromium.org/2886693002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/inspector/browser_protocol.json (right):

https://codereview.chromium.org/2886693002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/inspector/browser_protocol.json:4683: { "name":
"behavior", "type": "string", "enum": ["deny", "allow"], "description": "Whether
to allow all or deny all download requests." }
drive-by (trying to verify that this aligns with future plans): I'm guessing we
would later add an "ask" behavior (for per-download decisions via DevTools
events) and an additional parameter to switch between downloading to a file or
to a devtools stream?

[Sami, 2017-05-17 13:20:41]

Thanks! This looks like a good starting point for downloads.

https://codereview.chromium.org/2886693002/diff/20001/headless/lib/browser/he...
File headless/lib/browser/headless_devtools_manager_delegate.cc (right):

https://codereview.chromium.org/2886693002/diff/20001/headless/lib/browser/he...
headless/lib/browser/headless_devtools_manager_delegate.cc:406:
download_delegate->SetDownloadBehavior(behavior);
Do we also need to apply this to contexts that get created after this command
has run?

https://codereview.chromium.org/2886693002/diff/20001/headless/lib/browser/he...
headless/lib/browser/headless_devtools_manager_delegate.cc:428:
download_delegate->SetDownloadDirectory(path);
Same question here.

https://codereview.chromium.org/2886693002/diff/20001/headless/lib/browser/he...
File headless/lib/browser/headless_download_manager_delegate.cc (right):

https://codereview.chromium.org/2886693002/diff/20001/headless/lib/browser/he...
headless/lib/browser/headless_download_manager_delegate.cc:17: :
download_manager_(NULL),
nullptr (below too)

https://codereview.chromium.org/2886693002/diff/20001/headless/lib/browser/he...
headless/lib/browser/headless_download_manager_delegate.cc:90: ;
nit: extra ;

https://codereview.chromium.org/2886693002/diff/20001/headless/lib/browser/he...
headless/lib/browser/headless_download_manager_delegate.cc:126: // Testing exit.
?

https://codereview.chromium.org/2886693002/diff/20001/headless/lib/browser/he...
File headless/lib/browser/headless_download_manager_delegate.h (right):

https://codereview.chromium.org/2886693002/diff/20001/headless/lib/browser/he...
headless/lib/browser/headless_download_manager_delegate.h:49: void
SetDownloadBehaviorForTesting(
Did you forget to add the test sources since this doesn't seem to be called
anywhere?

https://codereview.chromium.org/2886693002/diff/20001/headless/lib/browser/he...
headless/lib/browser/headless_download_manager_delegate.h:52: void
SetDownloadBehavior(const std::string& behavior);
It feels like these should take the enum and a base::FilePath respectively for
type safety.

[Oleg Sushkov, 2017-05-19 04:38:23]

On 2017/05/17 at 09:44:45, eseckler wrote:
>
https://codereview.chromium.org/2886693002/diff/20001/third_party/WebKit/Sour...
> File third_party/WebKit/Source/core/inspector/browser_protocol.json (right):
> 
>
https://codereview.chromium.org/2886693002/diff/20001/third_party/WebKit/Sour...
> third_party/WebKit/Source/core/inspector/browser_protocol.json:4683: { "name":
"behavior", "type": "string", "enum": ["deny", "allow"], "description": "Whether
to allow all or deny all download requests." }
> drive-by (trying to verify that this aligns with future plans): I'm guessing
we would later add an "ask" behavior (for per-download decisions via DevTools
events) and an additional parameter to switch between downloading to a file or
to a devtools stream?

yep, exactly correct.

[Oleg Sushkov, 2017-05-19 06:34:22]

On 2017/05/17 at 13:20:41, skyostil wrote:
> Thanks! This looks like a good starting point for downloads.
> 
>
https://codereview.chromium.org/2886693002/diff/20001/headless/lib/browser/he...
> File headless/lib/browser/headless_devtools_manager_delegate.cc (right):
> 
>
https://codereview.chromium.org/2886693002/diff/20001/headless/lib/browser/he...
> headless/lib/browser/headless_devtools_manager_delegate.cc:406:
download_delegate->SetDownloadBehavior(behavior);
> Do we also need to apply this to contexts that get created after this command
has run?

Possibly, what would be a clean way of achieving that?

> 
>
https://codereview.chromium.org/2886693002/diff/20001/headless/lib/browser/he...
> headless/lib/browser/headless_devtools_manager_delegate.cc:428:
download_delegate->SetDownloadDirectory(path);
> Same question here.
> 
>
https://codereview.chromium.org/2886693002/diff/20001/headless/lib/browser/he...
> File headless/lib/browser/headless_download_manager_delegate.cc (right):
> 
>
https://codereview.chromium.org/2886693002/diff/20001/headless/lib/browser/he...
> headless/lib/browser/headless_download_manager_delegate.cc:17: :
download_manager_(NULL),
> nullptr (below too)
> 
>
https://codereview.chromium.org/2886693002/diff/20001/headless/lib/browser/he...
> headless/lib/browser/headless_download_manager_delegate.cc:90: ;
> nit: extra ;
> 
>
https://codereview.chromium.org/2886693002/diff/20001/headless/lib/browser/he...
> headless/lib/browser/headless_download_manager_delegate.cc:126: // Testing
exit.
> ?
> 
>
https://codereview.chromium.org/2886693002/diff/20001/headless/lib/browser/he...
> File headless/lib/browser/headless_download_manager_delegate.h (right):
> 
>
https://codereview.chromium.org/2886693002/diff/20001/headless/lib/browser/he...
> headless/lib/browser/headless_download_manager_delegate.h:49: void
SetDownloadBehaviorForTesting(
> Did you forget to add the test sources since this doesn't seem to be called
anywhere?
> 

This is a left over from a copy-paste, deleted now.

>
https://codereview.chromium.org/2886693002/diff/20001/headless/lib/browser/he...
> headless/lib/browser/headless_download_manager_delegate.h:52: void
SetDownloadBehavior(const std::string& behavior);
> It feels like these should take the enum and a base::FilePath respectively for
type safety.

done

[Sami, 2017-05-19 13:57:19]

On 2017/05/19 06:34:22, Oleg Sushkov wrote:
> This is a left over from a copy-paste, deleted now.

Ah, got it. I think this could still use some kind of test coverage :)

[Sami, 2017-05-19 13:57:26]

https://codereview.chromium.org/2886693002/diff/20001/headless/lib/browser/he...
File headless/lib/browser/headless_devtools_manager_delegate.cc (right):

https://codereview.chromium.org/2886693002/diff/20001/headless/lib/browser/he...
headless/lib/browser/headless_devtools_manager_delegate.cc:406:
download_delegate->SetDownloadBehavior(behavior);
On 2017/05/17 13:20:40, Sami wrote:
> Do we also need to apply this to contexts that get created after this command
> has run?

We could store the setting somewhere in |browser_| and have it propagate the
value to newly created contexts as soon as they create a new download manager
delegate.

https://codereview.chromium.org/2886693002/diff/60001/headless/lib/browser/he...
File headless/lib/browser/headless_download_manager_delegate.h (right):

https://codereview.chromium.org/2886693002/diff/60001/headless/lib/browser/he...
headless/lib/browser/headless_download_manager_delegate.h:49: void
SetDownloadBehavior(const DownloadBehavior& behavior);
nit: Pass by value

[Oleg Sushkov, 2017-06-05 06:06:11]

On 2017/05/19 at 13:57:26, skyostil wrote:
>
https://codereview.chromium.org/2886693002/diff/20001/headless/lib/browser/he...
> File headless/lib/browser/headless_devtools_manager_delegate.cc (right):
> 
>
https://codereview.chromium.org/2886693002/diff/20001/headless/lib/browser/he...
> headless/lib/browser/headless_devtools_manager_delegate.cc:406:
download_delegate->SetDownloadBehavior(behavior);
> On 2017/05/17 13:20:40, Sami wrote:
> > Do we also need to apply this to contexts that get created after this
command
> > has run?
> 
> We could store the setting somewhere in |browser_| and have it propagate the
value to newly created contexts as soon as they create a new download manager
delegate.
> 
>
https://codereview.chromium.org/2886693002/diff/60001/headless/lib/browser/he...
> File headless/lib/browser/headless_download_manager_delegate.h (right):
> 
>
https://codereview.chromium.org/2886693002/diff/60001/headless/lib/browser/he...
> headless/lib/browser/headless_download_manager_delegate.h:49: void
SetDownloadBehavior(const DownloadBehavior& behavior);
> nit: Pass by value

I've added some tests. PTAL. I might not be able to get this CL in before I
leave on Wednesday, but I'll try to make corrections tomorrow and maybe hand it
off in the 99% ready state to someone.

[pfeldman, 2017-06-05 16:07:31]

https://codereview.chromium.org/2886693002/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/inspector/browser_protocol.json (right):

https://codereview.chromium.org/2886693002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/inspector/browser_protocol.json:4742: "name":
"setDownloadBehavior",
You could make directory an optional parameter of this one to save on a protocol
method.

https://codereview.chromium.org/2886693002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/inspector/browser_protocol.json:4750:
"description": "Set the directory path where downloaded fles will be saved
(headless chrome only).",
fles -> files

[Sami, 2017-06-05 16:50:32]

Thanks, this looks great! A few small nitpicks.

https://codereview.chromium.org/2886693002/diff/80001/headless/lib/browser/he...
File headless/lib/browser/headless_browser_impl.cc (right):

https://codereview.chromium.org/2886693002/diff/80001/headless/lib/browser/he...
headless/lib/browser/headless_browser_impl.cc:229:
download_delegate->SetDownloadDirectory(base::FilePath(path));
nit: base::FilePath here seems unnecessary

https://codereview.chromium.org/2886693002/diff/80001/headless/lib/browser/he...
File headless/lib/browser/headless_download_manager_delegate.cc (right):

https://codereview.chromium.org/2886693002/diff/80001/headless/lib/browser/he...
headless/lib/browser/headless_download_manager_delegate.cc:57: std::cout <<
"wooo " << default_download_path_.value() << std::endl;
Might want to drop this :)

https://codereview.chromium.org/2886693002/diff/80001/headless/lib/browser/he...
headless/lib/browser/headless_download_manager_delegate.cc:127: // Testing exit.
I'm not sure what this comment means. Could you turn it into a full sentence?

https://codereview.chromium.org/2886693002/diff/80001/headless/lib/browser/he...
headless/lib/browser/headless_download_manager_delegate.cc:133: return;
nit: return not needed

https://codereview.chromium.org/2886693002/diff/80001/headless/lib/headless_b...
File headless/lib/headless_browser_browsertest.cc (right):

https://codereview.chromium.org/2886693002/diff/80001/headless/lib/headless_b...
headless/lib/headless_browser_browsertest.cc:10: #include
"base/files/scoped_temp_dir.h"
These include changes seem unrelated -- the file is otherwise unchanged?

https://codereview.chromium.org/2886693002/diff/80001/headless/lib/headless_d...
File headless/lib/headless_download_browsertest.cc (right):

https://codereview.chromium.org/2886693002/diff/80001/headless/lib/headless_d...
headless/lib/headless_download_browsertest.cc:1: // Copyright (c) 2012 The
Chromium Authors. All rights reserved.
Year += 5 and remove the (c)

https://codereview.chromium.org/2886693002/diff/80001/headless/lib/headless_d...
headless/lib/headless_download_browsertest.cc:6: // in a pure content context. 
Over time tests should be migrated here.
Not sure what "pure content context" means here? That should be the case for all
headless things.

https://codereview.chromium.org/2886693002/diff/80001/headless/lib/headless_d...
headless/lib/headless_download_browsertest.cc:37: #include
"net/dns/mock_host_resolver.h"
Unused?

https://codereview.chromium.org/2886693002/diff/80001/headless/lib/headless_d...
headless/lib/headless_download_browsertest.cc:60: static DownloadManagerImpl*
DownloadManagerForBrowser(
nit: no use for |static| since this is an anonymous namespace

https://codereview.chromium.org/2886693002/diff/80001/headless/lib/headless_d...
headless/lib/headless_download_browsertest.cc:145: : manager_(manager),
item_(NULL) {
nit: NULL is so C++98, let's go with nullptr

https://codereview.chromium.org/2886693002/diff/80001/headless/lib/headless_d...
headless/lib/headless_download_browsertest.cc:401: 
Could we have a test that checks the ALLOW/DENY behaviors (and makes sure the
default is deny)?

[Oleg Sushkov, 2017-06-06 03:36:55]

Thanks for the suggestions/corrections. I've addressed most of them below and
made the necessary changes. The two remaining are the request by pfeldman to
reduce it down to a single dev-tools command with two parameters, and Sami's
request for some additional tests. I'll leave that to whoever picks up this CL
(one of jzfeng, irisu, or dvallet probably).

https://codereview.chromium.org/2886693002/diff/80001/headless/lib/browser/he...
File headless/lib/browser/headless_browser_impl.cc (right):

https://codereview.chromium.org/2886693002/diff/80001/headless/lib/browser/he...
headless/lib/browser/headless_browser_impl.cc:229:
download_delegate->SetDownloadDirectory(base::FilePath(path));
On 2017/06/05 at 16:50:31, Sami wrote:
> nit: base::FilePath here seems unnecessary

done

https://codereview.chromium.org/2886693002/diff/80001/headless/lib/browser/he...
File headless/lib/browser/headless_download_manager_delegate.cc (right):

https://codereview.chromium.org/2886693002/diff/80001/headless/lib/browser/he...
headless/lib/browser/headless_download_manager_delegate.cc:57: std::cout <<
"wooo " << default_download_path_.value() << std::endl;
On 2017/06/05 at 16:50:31, Sami wrote:
> Might want to drop this :)

oops, done.

https://codereview.chromium.org/2886693002/diff/80001/headless/lib/browser/he...
headless/lib/browser/headless_download_manager_delegate.cc:127: // Testing exit.
On 2017/06/05 at 16:50:31, Sami wrote:
> I'm not sure what this comment means. Could you turn it into a full sentence?

hmm, looks spurious, deleted.

https://codereview.chromium.org/2886693002/diff/80001/headless/lib/browser/he...
headless/lib/browser/headless_download_manager_delegate.cc:133: return;
On 2017/06/05 at 16:50:31, Sami wrote:
> nit: return not needed

done

https://codereview.chromium.org/2886693002/diff/80001/headless/lib/headless_b...
File headless/lib/headless_browser_browsertest.cc (right):

https://codereview.chromium.org/2886693002/diff/80001/headless/lib/headless_b...
headless/lib/headless_browser_browsertest.cc:10: #include
"base/files/scoped_temp_dir.h"
On 2017/06/05 at 16:50:31, Sami wrote:
> These include changes seem unrelated -- the file is otherwise unchanged?

The origins of this file is I took download_browsertest.cc as the base and
altered it to fix the needs for headless download manager testing. These
#includes are needed in that file and I just removed them here since they arent.

https://codereview.chromium.org/2886693002/diff/80001/headless/lib/headless_d...
File headless/lib/headless_download_browsertest.cc (right):

https://codereview.chromium.org/2886693002/diff/80001/headless/lib/headless_d...
headless/lib/headless_download_browsertest.cc:1: // Copyright (c) 2012 The
Chromium Authors. All rights reserved.
On 2017/06/05 at 16:50:31, Sami wrote:
> Year += 5 and remove the (c)

done

https://codereview.chromium.org/2886693002/diff/80001/headless/lib/headless_d...
headless/lib/headless_download_browsertest.cc:6: // in a pure content context. 
Over time tests should be migrated here.
On 2017/06/05 at 16:50:32, Sami wrote:
> Not sure what "pure content context" means here? That should be the case for
all headless things.

deleted, disregard this is from the originating file

https://codereview.chromium.org/2886693002/diff/80001/headless/lib/headless_d...
headless/lib/headless_download_browsertest.cc:37: #include
"net/dns/mock_host_resolver.h"
On 2017/06/05 at 16:50:31, Sami wrote:
> Unused?

its used: host_resolver()->AddRule(kOriginOne, real_host);

https://codereview.chromium.org/2886693002/diff/80001/headless/lib/headless_d...
headless/lib/headless_download_browsertest.cc:60: static DownloadManagerImpl*
DownloadManagerForBrowser(
On 2017/06/05 at 16:50:32, Sami wrote:
> nit: no use for |static| since this is an anonymous namespace

done

https://codereview.chromium.org/2886693002/diff/80001/headless/lib/headless_d...
headless/lib/headless_download_browsertest.cc:145: : manager_(manager),
item_(NULL) {
On 2017/06/05 at 16:50:31, Sami wrote:
> nit: NULL is so C++98, let's go with nullptr

done, wart from originating file.

https://codereview.chromium.org/2886693002/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/inspector/browser_protocol.json (right):

https://codereview.chromium.org/2886693002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/inspector/browser_protocol.json:4750:
"description": "Set the directory path where downloaded fles will be saved
(headless chrome only).",
On 2017/06/05 at 16:07:30, pfeldman wrote:
> fles -> files

done

[Sami, 2017-06-06 16:52:04]

Great, thanks! Awaiting the next revision from the new owner :)  (Not sure if
you can reassign code reviews so they might need to start a new one.)