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.

Sign up for GitHub

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

Jump to bottom

Added null checks for managedCefBrowserAdapter. #635

Merged
merged 1 commit into from
Nov 25, 2014
Merged

Added null checks for managedCefBrowserAdapter. #635

merged 1 commit into from
Nov 25, 2014

Conversation

dominikhasiwar
Copy link

Got a nullref in ChromiumWebBrowser.Load(string url) if bound address changes after dispose was called. Therefor added null checks for managedCefBrowserAdapter.

@amaitland
Copy link
Member

@DHasi Welcome to CefSharp. Thanks for the contribution! 👍

I'll comment inline where relevant.

amaitland
amaitland reviewed Nov 25, 2014
View reviewed changes
CefSharp.Wpf/ChromiumWebBrowser.cs
@@ -470,8 +468,11 @@ private void CreateOffscreenBrowserWhenActualSizeChanged()
return;
}

managedCefBrowserAdapter.CreateOffscreenBrowser(BrowserSettings ?? new BrowserSettings());
browserCreated = true;
if (managedCefBrowserAdapter != null)
Copy link
Member

Choose a reason for hiding this comment

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

I don't believe a null check is required here as browserCreated is never set to false

@amaitland
Copy link
Member

@DHasi For the sake of getting this PR merged quickly, I think it's preferable to concisely fix the bug your experiencing. As it stands there are a lot of white noise formatting changes that take the focus off the actual proposed fix. There's also a few changes that aren't required in content.

Hopefully my comments aren't off putting. Happy to discuss in further detail if required.

@amaitland amaitland added the cef3 label Nov 25, 2014
@amaitland amaitland added this to the 37.0.0 milestone Nov 25, 2014
@dominikhasiwar
Copy link
Author

Hi Alex,
thanks a lot for your feedback! This was actually my first open-source-project pull request.so I expected some “mistakes” from my side. :) I will rework my fix and – as you mentioned – focus on the actual problem I found. I’m a very big fan of this project and really looking forward to contribute.

@amaitland
Copy link
Member

I’m a very big fan of this project and really looking forward to contribute

@DHasi Fantastic thanks! Merging this now 👍

amaitland added a commit that referenced this pull request Nov 25, 2014
@amaitland
Merge pull request #635 from dhasi/wpf_cwb_nullRef
aa13f24 
Added null checks for managedCefBrowserAdapter.
@amaitland amaitland merged commit aa13f24 into cefsharp:master Nov 25, 2014
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
cef3
Projects
None yet
Milestone
37.0.0-pre01
Development

Successfully merging this pull request may close these issues.
2 participants
@dominikhasiwar @amaitland