Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Auto-close blank popups when download is complete #1033

Merged
merged 3 commits into from
May 20, 2015
Merged

Auto-close blank popups when download is complete #1033

merged 3 commits into from
May 20, 2015

Conversation

jamespearce2006
Copy link
Contributor

Fix for issue #1031

Automatically close blank popups that appear when downloads are started in a separate window, once the download is done (complete, failed or cancelled).

Test the fix here:
https://jsfiddle.net/yLk7d3z8/

@@ -38,6 +38,12 @@ namespace CefSharp
void DownloadAdapter::OnDownloadUpdated(CefRefPtr<CefBrowser> browser, CefRefPtr<CefDownloadItem> download_item,
CefRefPtr<CefDownloadItemCallback> callback)
{

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Remove extra blank line.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done - I added the line to force AppVeyor to rebuild, but forgot to remove it.

@amaitland
Copy link
Member

If we merge this into master, I'd probably remove it in 2272 and give the user control to close their own browsers since we've introduced a wrapper. (I don't think we expose the close method yet, we should though).

@jankurianski
Copy link
Member

Ah, you're talking about the working being done in #1030? So the IDownloadHandler method could turn into:

bool OnDownloadUpdated(CefSharpBrowserWrapper browser, DownloadItem downloadItem)

And then users can do the browser.Close() if they chose? I think 👍

@amaitland
Copy link
Member

And then users can do the browser.Close() if they chose?

Yep, that's the one.

@jamespearce2006
Copy link
Contributor Author

@amaitland I agree, 2272 gives the user more control over such things. Do you think it'd be worth keeping this behavior as default, though, if the user doesn't implement the appropriate handlers?

@amaitland
Copy link
Member

Do you think it'd be worth keeping this behavior as default, though, if the user doesn't implement the appropriate handlers?

For downloads you have to implement IDownloadHandler or they're silently ignored. The default makes sense, I just think it's a little difficult to implement conditionally.

@amaitland
Copy link
Member

I have toyed with the idea of implementing a generic set of handlers that provide some sensible defaults, make all the methods virtual allowing the user to override only the pieces they require. Best of both worlds in some ways, if you require the flexibility you can implement all the low lying stuff yourself, if you only need to implement one method on IRequestHandler then it safes a lot of effort (specially in the fact that you have to return false to implement the default behavior in a lot of cases). Just one of those low priority ideas that I'd never gotten around too.

@amaitland amaitland added this to the 39.0.2 milestone May 20, 2015
amaitland added a commit that referenced this pull request May 20, 2015
Auto-close blank popups when download is complete
@amaitland amaitland merged commit 29f8021 into cefsharp:master May 20, 2015
@fabionardelli1982
Copy link

I checked the issue with the version 89.0.17 and the blank popup is not closed at the end of the Download of file. Then the issue is still reproducible

@amaitland
Copy link
Member

To give users full control this change was reverted as there was no way to disable the default behaviour.

The relevant APIs for closing the popup were added years ago so you can easily close the popup in your IDownloadHandler implementation.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants