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

Update to Cef Version 59.0.0 #2114

Merged
merged 11 commits into from
Oct 7, 2017
Merged

Update to Cef Version 59.0.0 #2114

merged 11 commits into from
Oct 7, 2017

Conversation

GrabzIt
Copy link
Contributor

@GrabzIt GrabzIt commented Aug 18, 2017

Note that there is one breaking change as offscreen windows are now automatically transparent due to this cef change: https://bitbucket.org/chromiumembedded/cef/commits/3f71138d64586d774f645a6db5283b5205ea9df4

To make a offscreen window non-transparent you need to set the background colour as shown below were it is set to white:

browserSettings.BackgroundColor = 0255255255;

Note that there is one breaking change as offscreen windows are now automatically transparent due to this cef change: https://bitbucket.org/chromiumembedded/cef/commits/3f71138d64586d774f645a6db5283b5205ea9df4

To make a offscreen window non-transparent you need to set the background colour as shown below were it is set to white:

browserSettings.BackgroundColor = 0255255255;
@GrabzIt
Copy link
Contributor Author

GrabzIt commented Aug 18, 2017

This will require the cef version in appveyor to be updated to 3.3071.1649

@ipoweb
Copy link

ipoweb commented Oct 5, 2017

any news about this update ?

</ItemGroup>
<Import Project="$(VCTargetsPath)\Microsoft.Cpp.targets" />
<ImportGroup Label="ExtensionTargets" />
<Target Name="EnsureNuGetPackageBuildImports" BeforeTargets="PrepareForBuild">
Copy link
Member

Choose a reason for hiding this comment

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

The EnsureNuGetPackageBuildImports targets need to be removed from this file and the others.

See the following for background
http://blog.davidebbo.com/2014/01/the-right-way-to-restore-nuget-packages.html
https://www.xavierdecoster.com/post/2014/03/06/migrate-away-from-msbuild-based-nuget-package-restore.html

build.ps1 Outdated
@@ -1,11 +1,11 @@
param(
[ValidateSet("vs2013", "vs2015", "nupkg-only", "gitlink")]
[Parameter(Position = 0)]
[string] $Target = "vs2013",
[string] $Target = "vs2015",
Copy link
Member

Choose a reason for hiding this comment

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

Are you deliberately upgrading the project to VC++ 2015? The documentation would need to be updated if this is the path your planning on taking, See #1983 the previous discussion on the topic.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sorry, no I wasn't I just compiled it in 2015. I will fix the target.

@@ -78,7 +77,7 @@ void ManagedCefBrowserAdapter::CreateBrowser(BrowserSettings^ browserSettings, R
CefString addressNative = StringUtils::ToNative(address);

CefBrowserHost::CreateBrowser(window, _clientAdapter.get(), addressNative,
*browserSettings->_browserSettings, requestContext);
*browserSettings->_browserSettings, *requestContext);
Copy link
Member

Choose a reason for hiding this comment

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

Was there an actual compiler error or were you just going by the Intellisense warning that there was a problem? The intellisense warning is safe to ignore.

As it's a ref counted pointer, I'd suggest changing this to static_cast<CefRefPtr<CefRequestContext>>(requestContext) instead. Passing in as a pointer should be safe in this instance, in general you shouldn't pass a ref counted pointer around as a normal pointer.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Will do I will need to read up on the difference.

Copy link
Member

Choose a reason for hiding this comment

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

@amaitland
Copy link
Member

This will require the cef version in appveyor to be updated to 3.3071.1649

@GrabzIt If you tag a release on the cef-binary project following the same structure as the others appveyor will build and upload packages to Myget.

Few passing comments made inline, best of luck 👍

@amaitland
Copy link
Member

any news about this update ?

Further releases are blocked on #1203

At this rate it's looking like you'll have to wait for Chromium to re-add support for the Process Per Tab process model.

@GrabzIt
Copy link
Contributor Author

GrabzIt commented Oct 6, 2017

Hi @amaitland I will make the changes. However someone raised the possibility of #1562 or fixing the blocking issue. What do you think?

@amaitland
Copy link
Member

However someone raised the possibility of #1562 or fixing the blocking issue. What do you think?

I find it very unlikely, you should investigate for yourself.

@GrabzIt
Copy link
Contributor Author

GrabzIt commented Oct 6, 2017

I ended up moving to the async javascript objects which doesn't have the issue. The code has been in production for a month so far with no problems.

@GrabzIt
Copy link
Contributor Author

GrabzIt commented Oct 6, 2017

Thanks for all the background information. I have made the requested changes.

@amaitland
Copy link
Member

I ended up moving to the async javascript objects which doesn't have the issue. The code has been in production for a month so far with no problems.

The problem described in #1203 affects both sync and async versions. They share the same code for large parts.

@@ -20,7 +20,7 @@ void CefBrowserHostWrapper::DragTargetDragEnter(IDragData^ dragData, MouseEvent

auto dragDataWrapper = static_cast<CefDragDataWrapper^>(dragData);
dragDataWrapper->ResetFileContents(); // Recommended by documentation to reset before calling DragEnter
_browserHost->DragTargetDragEnter(dragDataWrapper, GetCefMouseEvent(mouseEvent), (CefBrowserHost::DragOperationsMask) allowedOperations);
_browserHost->DragTargetDragEnter(*dragDataWrapper, GetCefMouseEvent(mouseEvent), (CefBrowserHost::DragOperationsMask) allowedOperations);
Copy link
Member

Choose a reason for hiding this comment

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

This pointer reference should also be changed.

@GrabzIt
Copy link
Contributor Author

GrabzIt commented Oct 6, 2017

The problem described in #1203 affects both sync and async versions. They share the same code for large parts.
It may be because my browser instances are so short lived that I am getting away with it.

I have updated the pointer in the mouse drag code as requested.

@amaitland
Copy link
Member

It may be because my browser instances are so short lived that I am getting away with it.

More like your navigating to websites within the same domain???

@GrabzIt
Copy link
Contributor Author

GrabzIt commented Oct 6, 2017

More like your navigating to websites within the same domain???

Actually I had exactly that problem! On my US servers calls to www.bbc.co.uk would go to www.bbc.com and break. It took two days of working on the issue, but I eventually read this case #1782 where he said he had a similar issue with losing registered objects and switched to async and that fixed his issue.

So I converted all of my C# JavaScript calls to use the async methods and made all of my JavaScript async as well (using let and async keywords etc) and it worked. I haven't had anymore of those problems.

@amaitland
Copy link
Member

Actually I had exactly that problem!

And your using a version greater than 57? In versions greater if the browser process switches to a new sub process, the bound objects are never registered in the new process.. The sync and async versions use the same code to transmit the objects, unless ClientAdapter::OnRenderViewReady is now being called for the creation of every process then it's not possible for one to work and the other to be broken. It maybe possible with a newer Chromium version the process switching is less aggressive and your domains aren't spawning a new process.

if (objectRepository->HasBoundObjects)

@GrabzIt
Copy link
Contributor Author

GrabzIt commented Oct 6, 2017

Oh ok, maybe that is what it is. I am now using version 60, but the fix also worked on version 59.

@amaitland
Copy link
Member

Oh ok, maybe that is what it is. I am now using version 60, but the fix also worked on version 59.

Tried your new branch and still breaks as expected, even using bbc.co.uk and then navigating to bbc.com the render process switches and the bound objects are no longer available. Unclear what your doing differently,

Anyways, I've spent way more time than I was planning on this, so that's it from me for now. I'll check back in a few months (probably when I have time off over Christmas). Best of luck getting a release out 👍

@GrabzIt
Copy link
Contributor Author

GrabzIt commented Oct 6, 2017

Thanks for all your help! You are a legend!

I am not sure why it is working on my version. One possibility is that I am setting the --renderer-process-limit to 1. So maybe that is related.

I haven't made any more changes to the cefsharp code to handle it though. I will keep an eye on the #1203 case and release when it is fixed. You are right it is better to be safe than breaking everyone's projects!

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.

3 participants