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

Add OnPluginCrashed to RequestHandler #372

Merged
merged 3 commits into from Jun 5, 2014

Conversation

amaitland
Copy link
Member

Implement IRequestHandler to be notified when a plugin crashes.

@jornh jornh added this to the 31.0.0 milestone May 30, 2014
@amaitland
Copy link
Member Author

@jornh @JanEggers Any thoughts on how to test this?

@jornh
Copy link
Contributor

jornh commented Jun 2, 2014

I guess one way is to have flash installed, navigate to a page with flash content, manually kill flash in task manager.

Don't know if there are other cases to cover ...

@jornh
Copy link
Contributor

jornh commented Jun 2, 2014

How about adding an extra commit here with a minimal demo of this to the IRequestHandler example s somewhat similar to the OnBeforePlugin one? Just so that we have a place to add a breakpoint for testing ...

{

}

bool IRequestHandler.OnBeforeResourceLoad(IWebBrowser browser, IRequestResponse requestResponse)
Copy link
Contributor

Choose a reason for hiding this comment

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

Duh - never mind that last comment below on this PR. It's here already 😄

Copy link
Member

Choose a reason for hiding this comment

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

Yes. But it would be good to replace that empty line with a comment there saying "here you could do fun stuff when a plugin crashes" (or perhaps something more useful...). I hate empty lines. 😄

Copy link
Member Author

Choose a reason for hiding this comment

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

I think we need something here, a comment, a MessageBox, something else....I'm just unclear on what adds the most value. Perhaps keep it simple and just add a comment 😄

@amaitland
Copy link
Member Author

I found that the handler is triggered when I kill BrowserSubProcess.exe when a flash video is playing, so I think it basically works. Other than displaying a message to users, I've got no idea what else you can do on plugin crash.

Happy to add extra comments if you have any suggestions. I've added you as a contributor to my CefSharp repository, so anything trivial that you feel like fixing up, your more than welcome, I'm not precious about it in any way shape or form!

@perlun
Copy link
Member

perlun commented Jun 2, 2014

I've added you as a contributor to my CefSharp repository, so anything trivial that you feel like fixing up, your more than welcome,

Could you add me also? It's so convenient to be able to quick-edit PR:s straight in the GitHub web UI (which requires this access to be set up).

@amaitland
Copy link
Member Author

Done

@amaitland
Copy link
Member Author

For reference I've added you too @JanEggers, figured as we don't have a WIP/DEV branch (perhaps that's something to consider after we get the first stable CefSharp3 out the door?) then it's probably easier when doing code reviews to just fix trivial things up as we go.

@amaitland
Copy link
Member Author

If I add a comment as per @jornh's recommendation does anyone have an objection to merging this one?

@jornh
Copy link
Contributor

jornh commented Jun 4, 2014

I didn't get around to testing it but just committed the comment. Merge away! Sleep time (1.24 here now)

@jornh
Copy link
Contributor

jornh commented Jun 5, 2014

Merging now @amaitland - thanks!
Tested by killing the appropriate BrowserSubProcess.exe and the handler is called. And there was much rejoicing

jornh added a commit that referenced this pull request Jun 5, 2014
@jornh jornh merged commit eed4d94 into cefsharp:master Jun 5, 2014
@amaitland
Copy link
Member Author

Thanks @jornh! Much appreciated!

@amaitland amaitland deleted the feature/add-onplugincrashed branch June 5, 2014 21:38
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.

None yet

3 participants