initial version of the headless download manager delegate [chromium/src : master]
David Vallet (Gerrit)
PTAL, this is ready to land
To view, visit change 590913. To unsubscribe, or for help writing mail filters, visit settings.
Eric Seckler (Gerrit)
Thanks for taking this on, David :)
Patch set 7:Code-Review +1
3 comments:
File headless/lib/browser/headless_browser_context_impl.cc:
Patch Set #7, Line 217:
DownloadManagerDelegatenit: Can we make HeadlessBrowserContextImpl::GetDownloadManagerDelegate() return a HeadlessDownloadManagerDelegate*, so that we don't have to do the casts in HeadlessBrowserImpl?
File headless/lib/browser/headless_browser_impl.cc:
Patch Set #7, Line 154:
browser_context->GetDownloadManagerDelegate();nit: doesn't look like we need this here, let's remove this line :)
File headless/lib/headless_download_browsertest.cc:
Patch Set #7, Line 359:
Check that downloading a single file works.nit: Test comment here and for DeniedDefaultDownload below need updating.
To view, visit change 590913. To unsubscribe, or for help writing mail filters, visit settings.
David Vallet (Gerrit)
Thanks, PTAL
3 comments:
Patch Set #7, Line 217:
DownloadManagerDelegatenit: Can we make HeadlessBrowserContextImpl::GetDownloadManagerDelegate() r
This is overriding a content::BrowserContext so we probably want to keep it as is.
Doing an additional method would mean adding it to public/HeadlessBrowserContext which only contains high level methods.
I created a From method from a content:DownloadManager to encapsulate the casting
File headless/lib/browser/headless_browser_impl.cc:
Patch Set #7, Line 154:
HeadlessDownloadManagerDelegate* download_dele
nit: doesn't look like we need this here, let's remove this line :)
Done
File headless/lib/headless_download_browsertest.cc:
Patch Set #7, Line 359:
Check that denying downloads works.
nit: Test comment here and for DeniedDefaultDownload below need updating.
Oops! done
To view, visit change 590913. To unsubscribe, or for help writing mail filters, visit settings.
Eric Seckler (Gerrit)
1 comment:
Patch Set #7, Line 217:
DownloadManagerDelegate
This is overriding a content::BrowserContext so we probably want to keep it
I was thinking we could just override the method with a more specialized return type in the Impl. Afaik, c++ supports covariant return types when overriding? :)
To view, visit change 590913. To unsubscribe, or for help writing mail filters, visit settings.
David Vallet (Gerrit)
@pfeldman: can you review changes to browser protocol?
1 comment:
File headless/lib/browser/headless_browser_context_impl.cc:
Patch Set #7, Line 217:
ownloadManagerDelegate*
I was thinking we could just override the method with a more specialized re
Huh, TIL
DOne
To view, visit change 590913. To unsubscribe, or for help writing mail filters, visit settings.
Pavel Feldman (Gerrit)
I think this needs to be implemented on the content level instead. I've recently moved embedder-specific alert() handling from chrome/ to content/. Downloads are no different. See what I did there: https://chromium-review.googlesource.com/c/592608
1 comment:
File third_party/WebKit/Source/core/inspector/browser_protocol.json:
Patch Set #10, Line 4947:
"name": "setDownloadBehavior",- Why is this on the browser level? Can this be page?
- Sad about seeing it for headless only. We would need it for headed.
To view, visit change 590913. To unsubscribe, or for help writing mail filters, visit settings.
Pavel Feldman (Gerrit)
Downloads are easier because we don't need to maintain backwards compatibility. WebDriver was making me preserve one for alert(). In your case, I would simply allow overriding download manager with the protocol-based version. I.e. what you did for headless would move into devtools/protocol/ and would substitute chrome's delegate.
Eric Seckler (Gerrit)
David Vallet has uploaded this change for review.
initial version of the headless download manager delegate
patch from issue 2886693002 at patchset 100001 (http://crrev.com/2886693002#ps100001)
Following fixes:
- Added Deny/Allow behavior tests, fixed setting deny by default
- Changed using TaskScheduler for FILE thread post tasks, since use of BrowserThread::FILE is deprecated
- Merged setDownloadBehavior and setDownloadDirectory into setDownloadBehaviour with downloadPath optional parameter.
- Additional cleanup
BUG=696481
Change-Id: Ib4385e478e89219062241983a6ceb523bf476d09
---
M headless/BUILD.gn
M headless/lib/DEPS
M headless/lib/browser/headless_browser_context_impl.cc
M headless/lib/browser/headless_browser_context_impl.h
M headless/lib/browser/headless_browser_impl.cc
M headless/lib/browser/headless_browser_impl.h
M headless/lib/browser/headless_devtools_manager_delegate.cc
M headless/lib/browser/headless_devtools_manager_delegate.h
A headless/lib/browser/headless_download_manager_delegate.cc
A headless/lib/browser/headless_download_manager_delegate.h
A headless/lib/headless_download_browsertest.cc
M third_party/WebKit/Source/core/inspector/browser_protocol.json
12 files changed, 788 insertions(+), 5 deletions(-)
Sami Kyöstilä (Gerrit)
I think the implementation looks great, but Pavel has a point: if there's a straightforward way to make this support non-headless too then let's try to do that.
1 comment:
File headless/lib/browser/headless_devtools_manager_delegate.cc:
Patch Set #10, Line 547:
AppendASCIILet's use FromUTF8Unsafe instead to support non-ASCII paths.
To view, visit change 590913. To unsubscribe, or for help writing mail filters, visit settings.
David Vallet (Gerrit)
Patch Set 10:
(1 comment)
I think the implementation looks great, but Pavel has a point: if there's a straightforward way to make this support non-headless too then let's try to do that.
Thanks for the suggestions!
Sorry for the silence, I just had some time to look into this.
I believe that this is should be doable for both headless/head, as Pavel suggests.
But, I don't think it can be done in Page without significantly reworking how the download manager works in Chrome. The main reason is that the download manager delegate is stored in the browser context: https://cs.chromium.org/chromium/src/content/public/browser/browser_context.h?rcl=baca1a8cf2f9382857ba94863d94e333674526b9&l=219
My suggestion then is to move the impl to content/protocol as Pavel suggests, but leave it in the Browser domain, wdyt?
Pavel Feldman (Gerrit)
Browser's fine. I just saw that most of the methods receive DownloadItem* as a parameter and those that don't are either coming from DownloadItemImpl or from WebContentsImpl directly. I.e. WC context should be trivially accessible. I might have missed something though. We can have a per-context delegate provided that all methods receive DI or WC. Up to you though, for most automation scenarios browsercontext would suffice.
David Vallet (Gerrit)
PTAL this is an early version of a working devtools Page.setDownloadBehavior command
Looking for early feedback (see TODO(dvallet)), thx!
David Vallet (Gerrit)
PTAL
I opted to have the new DevToolsDownloadManagerDelegate take a proxy manager delegate, for the following reasons:
- It allows using the GetNextId() of the chrome_download_manager_delegate, otherwise it makes it incompatible when disabling the custom download behavior or restoring a user-data-dir
- It allows gracefully falling back to the chrome_download_manager_delegate without having to add specific logic to recover it. This is doable, but it did add extra logic (e.g. taking into account how many page_handlers are enabled, keeping the previous instance)
- It allows handling some cases in which a download will start *after* the page handler is disabled
Sami Kyöstilä (Gerrit)
1 comment:
File content/browser/devtools/protocol/page_handler.cc:
I think we can reach here many times with different browser contexts. Could we instead have a global DevToolsDownloadManagerDelegate and here we create one if we haven't and attach it to the current browser context?
To view, visit change 590913. To unsubscribe, or for help writing mail filters, visit settings.
David Vallet (Gerrit)
1 comment:
File content/browser/devtools/protocol/page_handler.cc:
I think we can reach here many times with different browser contexts. […]
Done
To view, visit change 590913. To unsubscribe, or for help writing mail filters, visit settings.
Sami Kyöstilä (Gerrit)
Thanks, non-owner lgtm.
Patch set 18:Code-Review +1
Pavel Feldman (Gerrit)
Great stuff, see the large comment that explains a small change that might make things simpler on your side...
16 comments:
File content/browser/devtools/protocol/devtools_download_manager_delegate.h:
void Shutdown() override;
bool DetermineDownloadTarget(
content::DownloadItem* download,
const content::DownloadTargetCallback& callback) override;
bool ShouldOpenDownload(
content::DownloadItem* item,
const content::DownloadOpenDelayedCallback& callback) override;
void GetNextId(const content::DownloadIdCallback& callback) override;style: "// DownloadManagerDelegate overrides" comment above the following ones, no empty lines between overriden methods.
Patch Set #18, Line 52:
friend class base::RefCountedThreadSafe<DevToolsDownloadManagerDelegate>;We are not refcounted, stray?
Patch Set #18, Line 72:
base::WeakPtrFactory<DevToolsDownloadManagerDelegate> weak_ptr_factory_;Singleton should not have a weakptr factory.
File content/browser/devtools/protocol/devtools_download_manager_delegate.cc:
Patch Set #18, Line 43:
download_manager_ = download_manager;For the consistency, you should restore the proxy delegate on the previous instance.
Patch Set #18, Line 48:
proxy_download_delegate_ = proxy_delegate;So this is nullable, which means that it could be that in chrome embedder, devtools installs its delegate before chrome installs its delegate.
Patch Set #18, Line 48:
proxy_download_delegate_ = proxy_delegate;Let's try making proxy delegate always there.
In fact, DownloadManager::SetDelegate is only used for testing. How about we assign delegate_ in download_manager_impl_ constructor via taking it from the context instead of doing it here: https://cs.chromium.org/chromium/src/content/browser/browser_context.cc?type=cs&sq=package:chromium&l=210
In this case, we can assume that when devtools override comes in play, we already have an underlying embedder delegate. This would simplify your code below a great deal. We can rename DownloadManager::SetDelegate into DownloadManager::SetDelegateForTesting to make sure it is not abused.
Patch Set #18, Line 70:
return proxy_download_delegate_->DetermineDownloadTarget(download,You'll be able to DCHECK(proxy_download_delegate_)
Patch Set #18, Line 74:
if (!download_helper ||download_helper will then be here for sure, so this branch won't be needed
Patch Set #18, Line 85:
base::FilePath download_path =You could expose GenerateDefaultFilename on the download_manager and reuse it between this and the shell. Copy-paste is fine as well though.
Patch Set #18, Line 121:
if (proxy_download_delegate_)I would say it should be false in case you have the helper instance per this item.
File content/browser/devtools/protocol/page_handler.h:
Patch Set #18, Line 13:
#include <vector>stray?
File content/browser/devtools/protocol/page_handler.cc:
Patch Set #18, Line 633:
if (behavior == "allow") {style: drop {}
Patch Set #18, Line 637:
download_helper->SetDownloadPath(download_path.fromMaybe(""));is "" a good default?
download_delegate->SetDownloadManager(download_manager);
download_delegate->SetProxyDelegate(download_manager->GetDelegate());
download_manager->SetDelegate(download_delegate);Encapsulate these calls into a single
download_delegate->TakeOver(download_manager);
It knows what to do to the manager.
File content/shell/browser/shell_download_manager_delegate.cc:
Patch Set #18, Line 7:
#include <string>stray?
File third_party/WebKit/Source/core/inspector/browser_protocol.json:
Patch Set #18, Line 627:
{ "name": "downloadPath", "type": "string", "optional": true, "description": "The default path to save downloaded files to." }Juts a side note, would be more hermetic if we could return it as a stream instead of writing to file.
To view, visit change 590913. To unsubscribe, or for help writing mail filters, visit settings.
David Vallet (Gerrit)
Thanks for the review!
18 comments:
void TakeOver(content::DownloadManager* download_manager);
// DownloadManagerDelegate overrides
void Shutdown() override;
bool DetermineDownloadTarget(
content::DownloadItem* download,
const content::DownloadTargetCallback& callback) override;
bool ShouldOpenDownload(
content::DownloadItem* item,
const content::DownloadOpenDelayedCallback& callback) override;
void GetNextId(const content::DownloadIdCallback& callback) override;
style: "// DownloadManagerDelegate overrides" comment above the following ones, no empty lines betwe […]
Done
void TakeOver(content::DownloadManager* download_manager);
// DownloadManagerDelegate overrides
void Shutdown() override;
bool DetermineDownloadTarget(
content::DownloadItem* download,
const content::DownloadTargetCallback& callback) override;
bool ShouldOpenDownload(
content::DownloadItem* item,
const content::DownloadOpenDelayedCallback& callback) override;
void GetNextId(const content::DownloadIdCallback& callback) override;
style: "// DownloadManagerDelegate overrides" comment above the following ones, no empty lines betwe […]
Done
Patch Set #18, Line 52:
friend struct base::DefaultSingletonTraits<DevToolsDownloadManagerDelegate>;
We are not refcounted, stray?
Done
Patch Set #18, Line 52:
friend struct base::DefaultSingletonTraits<DevToolsDownloadManagerDelegate>;
We are not refcounted, stray?
Done
Patch Set #18, Line 72:
DISALLOW_COPY_AND_ASSIGN(DevToolsDownloadManagerDelegate);
Singleton should not have a weakptr factory.
Done
Patch Set #18, Line 72:
DISALLOW_COPY_AND_ASSIGN(DevToolsDownloadManagerDelegate);
Singleton should not have a weakptr factory.
Patch Set #18, Line 43:
download_manager_ = download_manager;For the consistency, you should restore the proxy delegate on the previous instance.
Done
Patch Set #18, Line 43:
download_manager_ = download_manager;For the consistency, you should restore the proxy delegate on the previous instance.
Done
Patch Set #18, Line 48:
proxy_download_delegate_ = download_manager->GetDelegate();So this is nullable, which means that it could be that in chrome embedder, devtools installs its del […]
For headless there's no embedded delegate, so in that case we do need to keep the logic in case proxy_delegate is not set.
Re SetDelegate: Wouldn't we need this to take over the manager when setting the DevtoolsDownloadManagerDelegate? afaik DownloadManagerImpl is only created once (per browser?) so getting it from the BrowserContext would not work.
Patch Set #18, Line 70:
return proxy_download_delegate_->DetermineDownloadTarget(item, callback);
You'll be able to DCHECK(proxy_download_delegate_)
The use cases are mainly for headless, where there is no delegate to substitute.
Patch Set #18, Line 74:
if (!download_helper ||download_helper will then be here for sure, so this branch won't be needed
Ditto here, this is the case for headless without setting DownloadBehavior, in that case we should deny by default. I've added a comment to make this more clear.
Patch Set #18, Line 85:
base::FilePath download_path =
You could expose GenerateDefaultFilename on the download_manager and reuse it between this and the s […]
Yeah there's a bit of copy/paste here which I'm not a fun of.
But I don't see a cleaner way of sharing the code though. I could have a virtual DetermineDefaultDownloadTarget added to download_manager but I think it would be strange to add it to the download_manager public interface, since it does look like something the delegate should do.Patch Set #18, Line 121:
if (!DevToolsDownloadManagerHelper::FromWebContents(item->GetWebContents()) ||
I would say it should be false in case you have the helper instance per this item.
Good point, Done
File content/browser/devtools/protocol/page_handler.h:
Patch Set #18, Line 13:
#include <vector>stray?
It's a lint error, should I ignore it?
File content/browser/devtools/protocol/page_handler.cc:
Patch Set #18, Line 637:
download_helper->SetDownloadPath(download_path.fromJust());is "" a good default?
Done. I'll just leave it empty
download_delegate->TakeOver(download_manager);
}Encapsulate these calls into a single […]
Done
File content/shell/browser/shell_download_manager_delegate.cc:
Patch Set #18, Line 7:
#include <string>stray?
ditto, this is a lint error
File third_party/WebKit/Source/core/inspector/browser_protocol.json:
Patch Set #18, Line 627:
{ "name": "downloadPath", "type": "string", "optional": true, "description": "The default path to save downloaded files to." }
Juts a side note, would be more hermetic if we could return it as a stream instead of writing to fil […]
Ack. I think it should be doable by extending the delegate to decide if it wants to receive the stream rather than saving to a file.
To view, visit change 590913. To unsubscribe, or for help writing mail filters, visit settings.
Pavel Feldman (Gerrit)
The code looks good. I think we could do better in terms of nullability of the default proxy. Let me try out a couple of things - I don't want to give suggestions that won't work. If I find nothing better, we can land as is. Sg?
9 comments:
File content/browser/devtools/protocol/devtools_download_manager_delegate.h:
Patch Set #19, Line 32:
void SetDownloadManager(content::DownloadManager* manager);You no longer need it, can be inlined into TakeOver.
Patch Set #19, Line 39:
void TakeOver(content::DownloadManager* download_manager);Just started reviewing this, but it feels like there should be an opposite one that would release the control over the download manager for symmetry. For example, when Page.disable is called.
Patch Set #19, Line 41:
// DownloadManagerDelegate overridesfriendly style nit: "." in the end of the comment.
File content/browser/devtools/protocol/devtools_download_manager_delegate.cc:
Patch Set #18, Line 48:
proxy_download_delegate_ = download_manager->GetDelegate();For headless there's no embedded delegate, so in that case we do need to keep the logic in case prox […]
Ah, that makes sense. Headless not having a delegate kill my proposal...
File content/browser/devtools/protocol/devtools_download_manager_delegate.cc:
Patch Set #19, Line 39:
void DevToolsDownloadManagerDelegate::SetDownloadManager(yeah, inline or expose as a friend to a test.
Patch Set #19, Line 89:
if (download_path.empty()) {Let's make sure download path is always there in the helper?
Patch Set #19, Line 95:
callback.Run(item->GetForcedFilePath(),Do you know when this happens? I think we should not force anything in the automated mode. I.e. download where the protocol tells it to.
Patch Set #19, Line 123:
return proxy_download_delegate_->ShouldOpenDownload(item, callback);This crashes when there is no helper attached.
Patch Set #19, Line 134:
callback.Run(next_id++);What bugs me is that we are essentially implementing a default version of the delegate for content in content. To the extent of implementing one in the browser context and assuming it is always there in this code...
To view, visit change 590913. To unsubscribe, or for help writing mail filters, visit settings.
Pavel Feldman (Gerrit)
2 comments:
File content/browser/devtools/protocol/page_handler.cc:
Patch Set #19, Line 648:
void PageHandler::OverrideDownloadDelegate() {Ok, so the only thing missing is going back from this state. We want PageHandler::Disable to delete all the helpers from the web contents and we'd also want devtools delegate to release control and redirect manager back to its original delegate.
The tricky bit is web contents vs browser, single devtools delegate is singleton, we would need to count clients.
File third_party/WebKit/Source/core/inspector/browser_protocol.json:
Patch Set #19, Line 626:
{ "name": "behavior", "type": "string", "enum": ["deny", "allow"], "description": "Whether to allow all or deny all download requests." },Let's add 'default' that would restore original browser behavior. It would not make sense to headless, but when running against real chrome, it would still make sense.
To view, visit change 590913. To unsubscribe, or for help writing mail filters, visit settings.
David Vallet (Gerrit)
SGTM, I'm happy to refine it until it feels right.
9 comments:
File content/browser/devtools/protocol/devtools_download_manager_delegate.h:
Patch Set #19, Line 32:
// |DownloadManagerDelegate| it will be stored as a proxy delegate.
You no longer need it, can be inlined into TakeOver.
Done. Thinking about this a bit, I've made TakeOver the only way to create this instance, that way we ensure that its not possible to create and assign this delegate directly.
Patch Set #19, Line 39:
// DownloadManagerDelegate overrides.Just started reviewing this, but it feels like there should be an opposite one that would release th […]
Yes, this makes sense if we decide to release control at Page.Disable(), see my comment in page_handler.cc
Patch Set #19, Line 41:
bool DetermineDownloadTarget(
friendly style nit: "." in the end of the comment.
Done
File content/browser/devtools/protocol/devtools_download_manager_delegate.cc:
Patch Set #19, Line 39:
void DevToolsDownloadManagerDelegate::TakeOver(
yeah, inline or expose as a friend to a test.
Done. We don't use it for tests, so I think is safe to remove.
Patch Set #19, Line 89:
callback.Run(empty_path,
Let's make sure download path is always there in the helper?
Sorry, I'm not sure what you mean by this. The default path can only be obtained from the BrowserContext, that's why this check is here.
Do you know when this happens? I think we should not force anything in the automated mode. I.e. […]
DOne. You're right this is probably not something we want for automated. This is when a request info has a save file path attached, which I imagine is when is setup in the user profile.
Patch Set #19, Line 123:
if (download_helper &&
This crashes when there is no helper attached.
Oops, good catch!
File content/browser/devtools/protocol/page_handler.cc:
Patch Set #19, Line 648:
nullptr;Ok, so the only thing missing is going back from this state. […]
I did try this and it worked up to a point. I found a couple of problem though:
- Sometimes Page.Disable() is called before the download initiates, so I thought it was best ensuring that if Helper data is associated we follow that setup. See code p[1] for an example
- Also, I founds sometimes Page.Disable() was not called as I expected (but maybe that was bad code on my part), again, to ensure that things are consistent I thing that relying on Helper data was better.
If you think this is correct behavior then is doable, the devtools delegate still needs to be aware of the GetNextID method of the proxied delegate though, since otherwise download ids are not in sync.
[1]
const CDP = require("chrome-remote-interface");
async function getDownload () {
const protocol = await CDP({port:9222});
try {
const {Page, Runtime} = protocol;
await Page.enable();
await protocol.send('Page.setDownloadBehavior', {behavior : "allow", downloadPath: "/tmp/download1"});
// await protocol.send('Page.bringToFront', {});
await Page.navigate({url:"http://ipv4.download.thinkbroadband.com/5MB.zip"});
} catch (err) {
console.error(err);
} finally {
protocol.close();
}
}getDownload().catch((err) => console.error(err));
File third_party/WebKit/Source/core/inspector/browser_protocol.json:
Patch Set #19, Line 626:
{ "name": "behavior", "type": "string", "enum": ["deny", "allow", "default"], "description": "Whether to allow all or deny all download requests, or use default Chrome behavior if available (otherwise deny)." },Let's add 'default' that would restore original browser behavior. […]
Done
To view, visit change 590913. To unsubscribe, or for help writing mail filters, visit settings.
Pavel Feldman (Gerrit)
2 comments:
File content/browser/devtools/protocol/devtools_download_manager_delegate.cc:
Patch Set #19, Line 89:
callback.Run(empty_path,Sorry, I'm not sure what you mean by this. […]
I'm suggesting to make download_path a required field when behavior is set to auto-accept.
File content/browser/devtools/protocol/page_handler.cc:
Patch Set #19, Line 648:
nullptr;
I did try this and it worked up to a point. I found a couple of problem though: […]
So in the snippet, await Page.navigate was processed before download started? I'd say this is intended behavior, Page.navigate is an atomic browser-level command, it never makes sense to await for it to be completed, one should rather wait for onload, network idle or download request as in your case.
To view, visit change 590913. To unsubscribe, or for help writing mail filters, visit settings.
David Vallet (Gerrit)
PTAL,
I've added the logic to Reset the DownloadDelegate.
4 comments:
File content/browser/devtools/protocol/devtools_download_manager_delegate.h:
Yes, this makes sense if we decide to release control at Page. […]
Done
File content/browser/devtools/protocol/devtools_download_manager_delegate.cc:
Patch Set #19, Line 89:
content::DownloadItem::TARGET_DISPOSITION_OVERWRITE,
I'm suggesting to make download_path a required field when behavior is set to auto-accept.
Done
Patch Set #19, Line 134:
proxy_download_delegate_->GetNextId(callback);What bugs me is that we are essentially implementing a default version of the delegate for content i […]
I think we still need to keep the proxy_delegate logic if we want to have a default option that bypasses the devtool_delegate
File content/browser/devtools/protocol/page_handler.cc:
So in the snippet, await Page. […]
I see, so a user should wait for a download to be finalized, in this case resetting in Disable should be doable
To view, visit change 590913. To unsubscribe, or for help writing mail filters, visit settings.
Pavel Feldman (Gerrit)
5 comments:
File content/browser/devtools/protocol/page_handler.h:
Patch Set #22, Line 176:
static int count_;style: don't declare in .h, use int g_count in anonymous namespace below.
File content/browser/devtools/protocol/page_handler.cc:
Patch Set #22, Line 634:
if (behavior == "allow") {We should have generated a constant for it, something like Page::SetDownloadBehaviorBehavior.
Patch Set #22, Line 636:
if (!download_path.isJust()) {drop {}
Patch Set #22, Line 641:
if (behavior == "default")ditto, use the constant.
also
The lifespan of the delegate should be while there are non-default overrides. I.e. when "default" is passed, we should go through the MaybeReset code path. Can we migrate devtools download delegate from the canonical singleton to a ref-counted one?
- Make DevToolsDownloadManager ref-counted, make it store raw pointer to its singleton value in a static field, set that static field in the contructor, null it in destructor.
- Hand-craft DevToolsDownloadManager::GetInstance() that creates instance lazily and returns ref-counter pointer to it.
- Store scoped_refptr<DevToolsDownloadManager> in page_handler for as long as you want override to exist.
That way you have ref-counting that is safe, no leaky singletons and delegate's destructor will take care of unregistering.
File third_party/WebKit/Source/core/inspector/browser_protocol.json:
Patch Set #22, Line 5075:
"name": "setDownloadBehavior",I don't think you are handling it in the Browser domain, remove this?
To view, visit change 590913. To unsubscribe, or for help writing mail filters, visit settings.
David Vallet (Gerrit)
Thx! PTAL
Pavel Feldman (Gerrit)
Get rid of the counter and this is good to go?
5 comments:
File content/browser/devtools/protocol/devtools_download_manager_delegate.cc:
Patch Set #24, Line 65:
g_devtools_manager_delegate = new DevToolsDownloadManagerDelegate();style nit: i would assign this in the constructor.
File content/browser/devtools/protocol/page_handler.cc:
Patch Set #24, Line 66:
int g_count_ = 0;You should not need it any longer.
Patch Set #24, Line 274:
MaybeResetDownloadDelegate();download_manager_delegate_ = nullptr;
Patch Set #24, Line 632:
OverrideDownloadDelegate();Inline it and only override when behavior is not 'default'?
Patch Set #24, Line 661:
if (g_count_ > 0 && --g_count_ == 0)You should not need to count any more, right?
To view, visit change 590913. To unsubscribe, or for help writing mail filters, visit settings.
David Vallet (Gerrit)
David Vallet uploaded patch set #26 to this change.
initial version of the headless download manager delegate
This cl implementes setDownloadBehavior devtools command, for both headles/non-headless
BUG=696481
Change-Id: Ib4385e478e89219062241983a6ceb523bf476d09
---
M content/browser/BUILD.gn
A content/browser/devtools/protocol/devtools_download_manager_delegate.cc
A content/browser/devtools/protocol/devtools_download_manager_delegate.h
A content/browser/devtools/protocol/devtools_download_manager_helper.cc
A content/browser/devtools/protocol/devtools_download_manager_helper.h
M content/browser/devtools/protocol/devtools_protocol_browsertest.cc
M content/browser/devtools/protocol/page_handler.cc
M content/browser/devtools/protocol/page_handler.h
M content/browser/devtools/protocol_config.json
M content/shell/browser/shell_download_manager_delegate.cc
M third_party/WebKit/Source/core/inspector/browser_protocol.json
M third_party/WebKit/Source/core/inspector/inspector_protocol_config.json
12 files changed, 922 insertions(+), 7 deletions(-)
To view, visit change 590913. To unsubscribe, or for help writing mail filters, visit settings.
David Vallet (Gerrit)
5 comments:
File content/browser/devtools/protocol/devtools_download_manager_delegate.cc:
Patch Set #24, Line 65:
download_delegate->proxy_download_delegate_ = download_manager->GetDelegate();
style nit: i would assign this in the constructor.
Done
File content/browser/devtools/protocol/page_handler.cc:
Patch Set #24, Line 66:
std::string EncodeImage(const gfx::Image& image,
You should not need it any longer.
Done
download_manager_delegate_ = nullptr;
Done
Patch Set #24, Line 632:
content::BrowserContext* browser_context = web_contents->GetBrowserContext();
Inline it and only override when behavior is not 'default'?
Done
Patch Set #24, Line 661:
static_cast<WebContentsImpl*>(WebContents::FromRenderFrameHost(host_)) :
You should not need to count any more, right?
Done
To view, visit change 590913. To unsubscribe, or for help writing mail filters, visit settings.
Pavel Feldman (Gerrit)
lgtm % nits
Patch set 26:Code-Review +1
2 comments:
File content/browser/devtools/protocol/devtools_download_manager_delegate.cc:
Patch Set #26, Line 65:
download_delegate->proxy_download_delegate_ = download_manager->GetDelegate();This already looks good, making this code run in the constructor for symmetry would be even better, but this is not critical.
File content/browser/devtools/protocol/page_handler.cc:
Patch Set #26, Line 650:
return Response::Error("downloadPath not provided");Although you return error, you are setting up an override. Please make this check above (around line 624) and bail out in case of this error.
To view, visit change 590913. To unsubscribe, or for help writing mail filters, visit settings.
David Vallet (Gerrit)
2 comments:
File content/browser/devtools/protocol/devtools_download_manager_delegate.cc:
Patch Set #26, Line 65:
if (download_delegate->download_manager_)This already looks good, making this code run in the constructor for symmetry would be even better, […]
Maybe I'm overthinking it, but in this case we allow taking over a different manager delegate. If put in the constructor this would not be possible. Also, we'd need to plumb the download manager intro the GetInstance method.
File content/browser/devtools/protocol/page_handler.cc:
Patch Set #26, Line 650:
download_helper->SetDownloadBehavior(Although you return error, you are setting up an override. […]
Done
To view, visit change 590913. To unsubscribe, or for help writing mail filters, visit settings.
David Vallet (Gerrit)
Patch set 28:Commit-Queue +2
Commit Bot (Gerrit)
CQ is trying da patch.
Note: The patchset sent to CQ was uploaded after this CL was approved.
"Fixed some nitscomments" https://chromium-review.googlesource.com/c/590913/28
Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/chromium-review.googlesource.com/590913/28
Bot data: {"action": "start", "triggered_at": "2017-08-23T01:36:05.0Z", "cq_cfg_revision": "81f1a3f0cb07446abeff7dd01ff1b8bae94ffe0d", "revision": "513c35c4e09564576e47fed18f3add7015fdee0f"}
Commit Bot (Gerrit)
Commit Bot merged this change.
initial version of the headless download manager delegate
This cl implementes setDownloadBehavior devtools command, for both headles/non-headless
BUG=696481
Change-Id: Ib4385e478e89219062241983a6ceb523bf476d09
Reviewed-on: https://chromium-review.googlesource.com/590913
Commit-Queue: David Vallet <dva...@chromium.org>
Reviewed-by: Pavel Feldman <pfel...@chromium.org>
Reviewed-by: Sami Kyöstilä <skyo...@chromium.org>
Reviewed-by: Eric Seckler <esec...@chromium.org>
Cr-Commit-Position: refs/heads/master@{#496577}
---
M content/browser/BUILD.gn
A content/browser/devtools/protocol/devtools_download_manager_delegate.cc
A content/browser/devtools/protocol/devtools_download_manager_delegate.h
A content/browser/devtools/protocol/devtools_download_manager_helper.cc
A content/browser/devtools/protocol/devtools_download_manager_helper.h
M content/browser/devtools/protocol/devtools_protocol_browsertest.cc
M content/browser/devtools/protocol/page_handler.cc
M content/browser/devtools/protocol/page_handler.h
M content/browser/devtools/protocol_config.json
M content/shell/browser/shell_download_manager_delegate.cc
M third_party/WebKit/Source/core/inspector/browser_protocol.json
M third_party/WebKit/Source/core/inspector/inspector_protocol_config.json
12 files changed, 920 insertions(+), 7 deletions(-)