RFC: Proposed refactorings

[perlun]

[Moved from #258]

first of all thx.

i would like to help u guys a little more and start with some refactoring.

first of i want to explit implement IRenderWebBrowser.setxxxxx methods in webview to hide the internal methods called by cef to cleanup the api of webview.

also i would like to completely remove IWpfWebBrowser as the (WebView) control is the contract(interface) in wpf and there is no need for an interface and wpf controls work with dependency property changed not inotifypropertychanged and lifetime is handled with loaded / unloaded events not dispose.
in xaml u need to specify a concrete instance anyway.
and binding the control to the viewmodel as u do in the example is bad practice as a viewmodel should not rely on a control

also i would like to replace remaining public methods with commands and move commands created in the example to the wpf lib to make them available without needing to reference sample dll or copy samplecode.

and i would like to move the code in BrowserSubProcess project to the c++ project. in IJavascriptProxy.h is stated that the interface should be internal anyway. and maybe extend the interface to a twoway so occouring exceptions can be send to the main process. i dont see any reason why the browsersubprocess should contain more code then SubprocessCefApp.Execute or something like that.

i could port the interface to use async / await task pattern that would remove the need to blocking wait for it.

you are using manual/autoReset events in various places without disposing them ( CefSubprocessWrapper) for instance that will lead to handle leaks in the long run

i had a look at the open issues and FAQ / is there any design document or things like that ?

later on im thinking about a way to map javasrcipt complex types to either existing types or dynamics.

another question:

whats the point of RtzCountdownEvent and not using System.Threading.CountdownEvent
( i dont see all class depedencies as it seems visual studio is not that good at finding them in c++ compared to c# )

please let me know if you think my ideas go in the right direction or if i misunderstood design decisions of you / your team. if you have other more important tasks for me to do first im ok with that too i am just much more experienced in c# than in c++ so i would prefere to handle tasks in the wpf libary.

cheers jan

[perlun]

@JanEggers, here are my comments:

i would like to help u guys a little more and start with some refactoring.

Sounds good. 😃 Refactorings is usually a good thing, as long as we have agreed on the scope and how things should get done.

first of i want to explit implement IRenderWebBrowser.setxxxxx methods in webview to hide the internal methods called by cef to cleanup the api of webview.

Go ahead, this sounds like a reasonable change. (please make each change a separate PR, even though this is slightly more awkward for you, but it makes review etc. a lot easier. So one PR for this one please.)

also i would like to completely remove IWpfWebBrowser as the (WebView) control is the contract(interface) in wpf and there is no need for an interface and wpf controls work with dependency property changed not inotifypropertychanged and lifetime is handled with loaded / unloaded events not dispose.

This one I am more hesitant about. But you are right - it's not really "needed" (you can just refer to the WebView straight away. I cannot at this moment remember exactly why I added it. 😃 Maybe it felt more "clean" to expose the WebView API that way, rather than just using the WebView straight away.

You can play around with it and see what you feel. Regarding the INotifyPropertyChanged, you are right that it's more common for WPF controls to use Dependency Properties (and that's also what the WebView actually uses, from what I can tell). This change (getting rid of INotifyPropertyChanged for the WebView) feels OK with me, so this you could do if you like - as a separate PR.

in xaml u need to specify a concrete instance anyway.
and binding the control to the viewmodel as u do in the example is bad practice as a viewmodel should not rely on a control

Well, now that I think it through properly, this might actually be the reason why I felt compelled to add the IWpfWebBrowser interface in the first place. Since there are certain details in CEF/CefSharp which are impossible to accomplish without executing a method on the control (like EvaluateScript) we must have some mechanism for propagating the control (or a controlled subset of it, like the IWpfWebBrowser interface in this case) to the ViewModel, so you can call some methods on it as needed. Yes, I am perfectly aware that this propagating is bad practice in WPF; you are totally right about this. But in this case, I don't really see any better way to do it, and I've done my best to try and make it be done in the cleanest fashion I could come up it with. Do you have any suggestion for how to improve it further? They are greatly welcome.

also i would like to replace remaining public methods with commands and move commands created in the example to the wpf lib to make them available without needing to reference sample dll or copy samplecode.

The remaining public methods in the IWpfWebBrowser, I take it? As long as it is stuff which could get triggered by the UI (like the Back/Forward etc. buttons) I think it makes sense, but for stuff like EvaluateScript I don't think it's the right approach to take. But please elaborate a bit further on this, it might be that I haven't understood your point properly.

and i would like to move the code in BrowserSubProcess project to the c++ project. in IJavascriptProxy.h is stated that the interface should be internal anyway. and maybe extend the interface to a twoway so occouring exceptions can be send to the main process. i dont see any reason why the browsersubprocess should contain more code then SubprocessCefApp.Execute or something like that.

The reason why it's done like that is mostly that I strongly dislike working with C++. 😃 It's such a refactoring-unfriendly PITA, so moving stuff into the (C#-based) SubProcess project makes things IMHO a bit more maintainable.

Ideally I would like to rewrite the CefSharp project also in C# but that's quite a lot of work and I'm not sure it's such a good idea. But I've refactored the CefSharp.Wpf control from C++ to C#, and the subprocess is also C#, so the smaller parts we have to keep in C++, the better (from my POV).

i could port the interface to use async / await task pattern that would remove the need to blocking wait for it.

Possibly an idea, but it means we have to rely on Microsoft.Bcl.Async since async/await isn't available in .NET 4.0 (and I don't think we should depend on 4.5 yet). But yes, it could be done - give it a try and make a separate PR out of this one if you like.

you are using manual/autoReset events in various places without disposing them ( CefSubprocessWrapper) for instance that will lead to handle leaks in the long run

That's not good. Please fix and submit a separate PR for this one also.

i had a look at the open issues and FAQ / is there any design document or things like that ?

Not yet, it's more a matter of searching the Google Group and the GitHub issue archive at the moment.

later on im thinking about a way to map javasrcipt complex types to either existing types or dynamics.

I think that's a good thing, I've also thought about something like that actually. In the first run we should implement the BindObject stuff in CefSharp3. This one is a quite tricky one, since we have stuff running in different processes, but it's clearly doable. You could start looking into these parts also if you like, but it's probably much harder than the other stuff mentioned previously (so perhaps focusing on them first is easier).

another question:

whats the point of RtzCountdownEvent and not using System.Threading.CountdownEvent
( i dont see all class depedencies as it seems visual studio is not that good at finding them in c++ compared to c# )

No idea whatsoever; that code was already present when I joined the project. You can investigate it further to see if we can get rid of it.

please let me know if you think my ideas go in the right direction or if i misunderstood design decisions of you / your team. if you have other more important tasks for me to do first im ok with that too i am just much more experienced in c# than in c++ so i would prefere to handle tasks in the wpf libary.

The same for me. I've done quite a lot of C++ historically, but hadn't really touched it from 2004 to 2013 when I came in contact with the CefSharp project... I think that's a quite general fact, that people are much more familiar with C# than C++, so this is also one very important point about why we shouldn't convert things to C++. IMHO, of course.

All in all - good points/questions from your side and it's also really good that you ask first, before doing too much work. Will be looking forward to your contributions soon! 😃

[JanEggers]

The reason why it's done like that is mostly that I strongly dislike working with C++. 😃 It's such a refactoring-unfriendly PITA, so moving stuff into the (C#-based) SubProcess project makes things IMHO a bit more maintainable.

(damn how does quoting work)

that are exactly my thoughts :) i just thought it would be a Change thats too big. if you dont mind i would start with moving as much code as possible in a base libary. In my oppinion only the c++ wrappers should remain in the c++ Project. that would speedup all the other Tasks since dependency detection of vs will work properly wich greatly helps to understand the code. Any preferences for the new Project Name?

omg there is no using keyword in c++ that is nuts

[jornh]

i had a look at the open issues and FAQ / is there any design document or things like that ?

Not yet, it's more a matter of searching the Google Group and the GitHub issue archive at the moment.

Maybe this is a good time to start building a bit of "CefSharp3 hackers guide" in the wiki - just by adding minimal bits and pieces as decisions are made from now on? (because searching Group+issues for rationales on design is a PITA)

And the easiest way to do quoting is to mark and press the r key. Aheemm now ... back to the issue I signed up to work on 😄

[perlun]

(damn how does quoting work)

Either use the approach that @jornh suggested, or prepend the text with a > character, which I did (since I wasn't aware of the excellent r key. 😃)

[perlun]

that are exactly my thoughts :) i just thought it would be a Change thats too big. if you dont mind i would start with moving as much code as possible in a base libary. In my oppinion only the c++ wrappers should remain in the c++ Project. that would speedup all the other Tasks since dependency detection of vs will work properly wich greatly helps to understand the code. Any preferences for the new Project Name?

Perhaps something like this:

1. Rename CefSharp to CefSharp.Core.
2. Add a new C#-based CefSharp-project.

However, be aware that this is by far a trivial task. 😃 Things are quite dependent on each other. Nonetheless, there are certainly stuff that could be moved out from CefSharp.Core (like most of the JavaScript/type reflection stuff, which isn't really that dependent on CEF stuff). You can give it a try and see how far you get! I think it would be a great improvement, because just as you say, having stuff in C# rather than C++ makes it much, much easier to work with the code, refactor, analyze how stuff works etc...

[perlun]

Maybe this is a good time to start building a bit of "CefSharp3 hackers guide" in the wiki - just by adding minimal bits and pieces as decisions are made from now on? (because searching Group+issues for rationales on design is a PITA)

Excellent idea. I'll see if I can find some time for it.

[JanEggers]

narf its killing me

i wanted to merge upstream and failed

with the git windows client i didnt find any merge button at all.

at work we use svn and as github supports svn client i tried it.
it merged correctly but it didnt draw the line in the network diagram so i suppose i shouldnt use that tool
can u please explain me how its done. i found a article on stackoverflow but that didnt help either

it was cmd line advice wich said
git fetch upstream
git merge upstream/master

that didnt work for me git fetch upstream didnt do anything and git merge told me there is nothing to merge but there is the missng dll that should be mergable

why have things to be that hard when doing them the first time :(

and why should i want to use a shell are we in the early 90s again

[perlun]

Hehe. 😃 Shells aren't that bad; they are powerful IMHO. But I do agree that the command-line git can be a bit picky about "who its friends are"...

I suggest one of the following clients:

1. SourceTree. I've mostly used the Mac version (since I happen to be a happy user of a MacBook Pro as my primary development machine), but the Windows version is OK also.
2. GitHub for Windows. This one is a bit more "stripped down" feature-wise compared to SourceTree. The fun part about it is that it uses CefSharp internally. 😉 But apart from that, I think it's too limited to be really usable - feel free to try it out and see if I'm wrong, but that's my perception of it.

Working with Git using SourceTree is a lot, lot easier than using the command line, especially if you need to do some of the more non-trivial operations.

[JanEggers]

source tree Looks promising ill try that one tomorrow.

i made some Progress i Setup the new Project structure with the new lib.
the Interfaces are there and some of the classes too

i created a bunch of new Interfaces for the unmanaged portions of the code that have to remain in the c++ lib. and switched the Cookie funktions in the cef class to use Tasks wich eliminates the Need to lock. i like it a lot more like that. ( only await needs .net 4.5 Tasks are in 4.0 )

i used mef and load the c++ dll and get the implementations for the new Interfaces.

is this ok ? any other idea how to inverse contol ?

i think i will commit a work in Progress tomorrow

[perlun]

i made some Progress i Setup the new Project structure with the new lib.
the Interfaces are there and some of the classes too

Cool!

i used mef and load the c++ dll and get the implementations for the new Interfaces.

I must admit I haven't really used MEF before. Castle Windsor and Unity I've used (which are both Inversion of Control containers). I don't really see what MEF would add here, but... it might be just me. 😃 If you feel like it's worthwhile, please do it like that and commit a WIP PR that we can use for discussing it further.

(The only reason I'm a bit skeptic is that I, just like you, don't think we should add interfaces just for the sake of it. Since there will only be one single implementing class, it might be a bit overkill. But then again, it might improve testability/TDD of our code, which is always a good thing for framework components like CefSharp, so maybe it isn't that bad again. MEF has the advantage of being available in .NET 4.0, so it doesn't involve any external dependency which is a huge benefit in my eyes.)

[jornh]

I like the general direction this is taking. Especially the discussion on the C++ and C# split. Looking forward to see some initial code 😄 even though I might not have a lot to add to it it since I'm still quite new to the codebase.

Regarding the design documentation mentioned above I created an initial rough high level diagram as I currently understand it at http://codepen.io/jornh/full/Iyebk If you guys think it is useful we can adjust it to fit .... It is heavily inspired - actually forked 😃 - from a diagram from another project also somewhat related to Chromium. I'm currently working on the lower "plumbing" red part of this diagram over in the cefsharp/cef-binary repo. I should probably start a WIP PR over there too hehe

Edit: fixed broken link

[JanEggers]

whooo doing great

the reason i need MEF is i want to put as much managed code as possible in the base lib as abstract classes. so i need a factory for instances of the implementation of that abstract class which is in the c++ lib.
i define the factory interface in the c# libary and implement it in c++.
to get the implementation i use a mef assemblycontainer for the c++ libary. so i get the benefit of inversion of control and the implementer is trusted with the snk you are using to sign the dll´s which makes it impossible to replace the c++ with a one that contains virus code or something like that.

source tree is great i really like that changes are marked in blocks in a file and i dont have to scroll to the changes.

im motivational challanged at the moment ( lazy ) i have problems to get the files / folders to there new places so source tree recornices it. i googled a bit and it seems i have to use the shell as moves are not jet supported :)

anyway expect a commit in 1 hour or so this cant be that hard

[JanEggers]

so its in at least for me it compiles and runs

for the moment it is a work in progress.

i would be greatly appreciate feedback i hope u like what i did

[jornh]

Great - 👍! I can take a look and try to compile ... Only, I don't see that you pushed anything to your fork on GitHub, let alone did a Pull Request https://github.com/JanEggers/CefSharp/commit/b813fb35db22bce04578fcdf067999744272d9f7 Is the last I see.

But to make you feel a bit better 😄 . I'm also a long time SVN user with very little git experience - so I can 100% relate to the pains you are going through with command line, reading on SO about branching, checkout, rebase etc.

[JanEggers]

try again i only commited but didnt push :)

[jornh]

That push helped immensely 😉

My first try as a git noob was just to download the CefSharp-CefSharp3.zip from your fork. But something has failed and my zip copy screams at me - for example see the first warning below - something that you deleted here but it is in my copy. I will continue looking at it ...

This is what VS2010 complains about when I try to do a rebuild of just the CefSharp.Core project:

Warning 1   The referenced project '..\CefSharp\CefSharp.vcxproj' does not exist.   CefSharp.Wpf.Example
Warning 2   The tag 'WebView' does not exist in XML namespace 'clr-namespace:CefSharp.Wpf;assembly=CefSharp.Wpf'. Line 168 Position 18. C:\Users\jhe\GitHub\JanEggers\CefSharp-CefSharp3\CefSharp.Wpf.Example\Views\Main\MainView.xaml  168 18  CefSharp.Wpf.Example
Error   3   error C1192: #using failed on 'C:\Users\jhe\GitHub\JanEggers\CefSharp-CefSharp3\CefSharp\bin\Debug\CefSharp.dll'    C:\Users\jhe\GitHub\JanEggers\CefSharp-CefSharp3\CefSharp.Core\Stdafx.cpp   1   1   CefSharp.Core

[JanEggers]

thx for the info

i had a look and i get the same warnings i will have a look at the projects with a texteditor and remove remaining references ( damn you cant trust the tools )

sorry that it occoured in the first place but i have about 170 warnings from libcefmanagedwrapper that debugsymbols are missing so i overread the critical warnings.

are there some settings to ignore the symbol warnings or do you have to tweak the binary dlls in some way.

anyway another question about source tree.
i tent to fall over things i dont like and fix them that leads to quite big commits. under svn ( with smartsvn as client ) i have the opportunity to split my local changes to multiple changesets so they are grouped by purpose.
Is there any option like this in source tree?

[jornh]

No problemo! That's why I offered to compile in the first place - and it's no fun to make a clean clone all the time when you do a push (that's really what CI servers are there for ;). And because I know @perlun is better qualified to review the code but has no access to his compile machine as he is on paternity leave ....

Super that you are able to reproduce. That restores confidence in the GitHub .zips as a tool. Then it's only VStudio or whatever you relied on - toolwise. 👎 BTW I referenced the wrong line in your diff above but you probably figured that out.

are there some settings to ignore the symbol warnings or do you have to tweak the binary dlls in some way.

Not sure about the source of those warnings you have. If they are about the DLL's from CEF upstream you can get symbols from http://magpcss.net/cef_downloads

anyway another question about source tree.
i tend to fall over things i dont like and fix them that leads to quite big commits.

Good thing! Clean up as you go :) Only thing is to remember to make small commits ... (unless you rely on something ~ svn changesets)

under svn ( with smartsvn as client ) i have the opportunity to split my local changes to multiple changesets so they are grouped by purpose.
Is there any option like this in source tree?

Dunno - yet! I have not played with SourceTree (only the GitHub one and git-gui that came with msysgit). I read/saw on youtube you can do the opposite called "git squashing" .. I like the name & concept ... But then maybe one option is git add -p like explained here on SO. It's just a matter of if it's available in any of the GUI clients?

Update: Downloaded ST. The homepage had me sold when it said that it supported git-flow (a workflow-style CefSharp might benefit from?). And it look like that you can revert hunks is what you were asking for. But there might be better ways to work with git?

[JanEggers]

i broke some of the Solution Configurations.

Before repairing them i would like to know why there are so many i only see use for x64 and win32.

but i think it would be better to compile all managed only dlls with anycpu in all solution configuration.

Did i miss something ?

[JanEggers]

i thought again on my concept and i think i will reverse some things.

it will be more compatible if i rename the classes in the new cef baslib to *Base and make them internal and the c++ are the ones that are used again. so i dont need a factory to create stuff.

[perlun]

Hi @JanEggers. Looked briefly at your work - impressive indeed! You seem to have come quite a bit already. Keep up the good work!

Regarding the Solution Configurations: there should only be two (Win32 and x64). If I check out my branch (CefSharp3), that's the way it is. So I think it's the new project (CefSharp.Core) that caused this.

In general, this is the way it should be: only Win32, in which all projects are compiled for Win32/x86, and x64, in which all projects are compiled for Win64/x64.

I don't think it's suitable to use AnyCPU in this case. Since the C++ libraries (libcef.dll etc) are unmanaged DLL:s, we depend on a specific architecture of them (x86 or x64), all of which works with the existing CefSharp.vcxproj file. It also has some other nice tricks, which makes it possible to use both VS2010 and VS2012 for compilation. That's accomplished using this line:

<Import Project="$(SolutionDir)\CefSharp.props" />

So this should be added to the .vcxproj, and you can then remove the extraneous solution configurations.

[JanEggers]

i have manged to create a managed taskscheduler to wrap ceftasks / cefthreads and switched the api of the subprocess to work with tasks. no more waiting and locking needed jay.

unfortunately not everything went as smooth as planned.

1. the test site is broken it is missing files. ( i didnt delete them they seem gone longer )
2. yesterday i at least got an error now i only get a blank page i dont see any exceptions and the response with errorcde 404 was read but then the cef upstreamcode just does nothing more :(
3. the async javascript execution ( upper run button ) is broken i get an accessviolation maybe stackoverflow when trying to convert the result. that makes me sad as this is one of the portions of the code i didnt touch yet. what should that button do / what should it return ?
4. internal are not internals atm. seems the probjects have to be included with as_friend but i didnt get that working jet
5. i would like to add a global assbelyinfo / add consts that are used in the all projects that way the version / publickeys etc are not spread in so many places to update.
6. i have problems with nullpointer exceptions / accessviolation on shutdown in cefrefptr so i think my cleanup code is not really that clean
7. lets get that stuff stable again i would like to enable / extend the available unittests is nunit still the testsuite of choice? why are the tests all commented out ? what about the stuff in old folder im a fan of clean code so either include the projects in tests or delete em

[perlun]

1. the test site is broken it is missing files. ( i didnt delete them they seem gone longer )

Unfortunately, you're right. I think I may have disabled it at some point.

yesterday i at least got an error now i only get a blank page i dont see any exceptions and the response with errorcde 404 was read but then the cef upstreamcode just does nothing more :(

the async javascript execution ( upper run button ) is broken i get an accessviolation maybe stackoverflow when trying to convert the result. that makes me sad as this is one of the portions of the code i didnt touch yet. what should that button do / what should it return ?

To be able to resolve these ones, I strongly recommend that you make another working copy (do not remove your existing one though! 😃) based off of the CefSharp3 branch at github.com/cefsharp/CefSharp. I.e. the current code on the "official" project page. You can then have one Visual Studio open for "your version" and another one for "our version". I think that will help you quite a lot, and it will also make it more obvious as to what stuff worked before and what not. And it will explain the question of "what should that button do", at least to a certain degree.

internal are not internals atm. seems the probjects have to be included with as_friend but i didnt get that working jet

I think I might have had some issue with that also at some point. It's no big deal if you cannot make that work; we can just leave them uncommented/undocumented.

i would like to add a global assbelyinfo / add consts that are used in the all projects that way the version / publickeys etc are not spread in so many places to update.

The publickey is only in one place:

<LinkKeyFile>..\CefSharp.snk</LinkKeyFile>

...but for the version number, I agree. The only problem though is that we have some C++ projects and some C#, which makes it a bit non-trivial. But you can give it a try if you like, however I think the other issues are far more important yet. 😉

i have problems with nullpointer exceptions / accessviolation on shutdown in cefrefptr so i think my cleanup code is not really that clean

The "extra working copy" might help you here also, so you can compare how stuff works (perhaps by single-stepping through the code) in your branch vs. our branch.

lets get that stuff stable again i would like to enable / extend the available unittests is nunit still the testsuite of choice? why are the tests all commented out ? what about the stuff in old folder im a fan of clean code so either include the projects in tests or delete em

I agree. The questions I think we need to ask ourselves is a bit like these:

1. What should be tested, and what not?
2. Do we aim for "100% code coverage" or more "testing the stuff that is likely to get broken"? I prefer the latter.
3. What test framework should be used?

You can use NUnit or perhaps even better XUnit I'd say. I used XUnit a bit last year; there is some fluid extension for it which makes it possible to write tests using a syntax like this:

    [Fact]
    public void DecodeUtf8StringRemovingBom_ReturnsStringWithoutBom()
    {
        var bytes = new byte[]
        {
            // First three bytes is the UTF8 BOM.
            239,
            187,
            191,

            Convert.ToByte('s'),
            Convert.ToByte('p'),
            Convert.ToByte('a'),
            Convert.ToByte('m')
        };

        var result = Utils.DecodeUtf8StringRemovingBom(bytes);
        result.Should().Be("spam");
    }

Regarding "what should be tested", I think we should focus on our own code. I.e. CefSharp.Core, CefSharp and CefSharp.Wpf. We don't need to unit-test Chromium or CEF; that is not the point of our project. We should expect our dependencies to work, and if they don't, I believe the unit tests for them should get added to Chromium or CEF rather than to CefSharp.

So, the unit tests should focus on our own classes, wrappers, type interrogators and so forth.

If you feel that you need tests to be able to make things work, feel free to hack around in this area also. If so, please use XUnit.

[JanEggers]

x 5. and 6. are done.
you can have a look at what i did to the assemblyinfo

x 7. the tests are switched to xunit and enabled. but they all fail. i have to wait for IWebBrowserInternal.OnInitialized calls before start testing. im thinking of routing the call to this method over the CefBrowserWrapper so it can create the subprocessproxy or at least the channelfactory....

cheers jan

[jornh]

Wow, so far it looks - really, really - great from here 😄

Here is what I have done so far and what I noticed along the way:

1. I did a pull of e63d45c
2. Minor nit: Performance1.psess …. Added to the solution but no file - just an oversight?
3. On compile it gives me one error : Cefsharp.Test is missing a Fixture.cs … I excluded it to get forward. (Apart from that a lot of warnings but I guess that's not different from before)
4. I then built and executed CefSharp.WinForms.Example successfully then CefSharp.Wpf.Example. (with VS2010 SP1 - Win32/Debug - so far... I will continue with x64 & Release and see if it barfs at me)
   • Oh boy do I love ManyBrowser mode - sweet :-)
5. In the CefSharp.Wpf.Example when I click Run at the Execute Javascript async control it throws this error:

System.ServiceModel.CommunicationObjectFaultedException was unhandled by user code
  HResult=-2146233087
  Message=The communication object, System.ServiceModel.Channels.ServiceChannel, cannot be used for communication because it is in the Faulted state.
  Source=mscorlib
  StackTrace:
    Server stack trace: 
       at System.ServiceModel.Channels.CommunicationObject.Close(TimeSpan timeout)
       at System.ServiceModel.Channels.CommunicationObject.Close()
       at System.ServiceModel.Channels.ServiceChannel.System.IDisposable.Dispose()
    Exception rethrown at [0]: 
       at System.Runtime.Remoting.Proxies.RealProxy.HandleReturnMessage(IMessage reqMsg, IMessage retMsg)
       at System.Runtime.Remoting.Proxies.RealProxy.PrivateInvoke(MessageData& msgData, Int32 type)
       at System.IDisposable.Dispose()
       at CefSharp.ManagedCefBrowserAdapterBase.<>c__DisplayClass1.<DoEvalueteScript>b__0() in C:\Users\jhe\GitHub\JanEggers\CefSharp\CefSharp\ManagedCefBrowserAdapterBase.cs:line 64
       at System.Threading.Tasks.Task`1.InnerInvoke()
       at System.Threading.Tasks.Task.Execute()
  InnerException: 
      null

Only thing I did regarding looking at the code was:

• Noticed an API change from Cef.xx to Cef.Instance.xxx but I guess it's needed due to the new support of ManyBrowser mode it just need to be documented in the ChangeLog - and CefSharp.MinimalExample needs updating after new NuGet's have been released

So @perlun what's the next step? A merge - or more reviewing/testing - or something else?
How can I help out?

[JanEggers]

i will update with the missing / too much files i still have to get used to source tree....
i think its not yet stable enough to get merged i would like to fix at least the javascript error.

i would prefere if you just merge things to https://github.com/JanEggers/CefSharp/commit/b4201d1971ea1b42381417aae8dda3200c46a285 because the c# project is far from completed. the other changes where ment to be another pullrequest but i didnt get that right.

from my perspective the classes have a lack of state handling. the most errors are handled in a fashion of if (x != null) x.DoStuff(); thats a minimalistic approach which is not optimal. what i prefer is if (not initialized) throw notinitializedexception() or if (disposed) throw disposeexception that way there are more exceptions for the api consumer to handle but they see that things are not good. at the moment if a call is not executed they dont get any exception or just a nullpointer exception so they can just assume why its not working.

from my point of view the best thing u can do is check my api changes as there are a view of them. and im not really sure if they are all welcome. i use Tasks in various places to do async stuff which was handled differently before.

the Instance on Cef class is used because i have a baseclass for that one now which uses abstract functions and i cannot use abstract on static functions.

[perlun]

I think we're not really done yet. Actually, to be honest, I feel the change is quite a bit "too big" for one single PR. I did mention in the beginning that I'd prefer if things would be submitted as a set of smaller pull-requests... 😃 It makes the review so much smoother.

There are also at least a couple of things that should be fixed:

• It doesn't build at all at the moment with VS2012 - the problem is with CefSharp.Core not including CefSharp.props which was mentioned above. Jan, please fix this.
• The Cef.Instance.xxx doesn't so cool, but I do understand that things might have to be changed because of support for ManyBrowser. I must admit I haven't really checked these parts carefully enough. But as I say, I feel that the changes are a bit "biggish" at the moment.

So what I would prefer is for example:

1. One pull request that splits CefSharp into CefSharp.Core and CefSharp, and nothing else.
2. One pull request that adds support for ManyBrowser mode.
3. One pull request that rewrites stuff to use Task etc.

I know this can feel a bit picky (which it is) but I do believe that it will make the review process easier. And it also means it's less likely that we introduce a bunch of bugs in this.

Please don't misunderstand me - I feel that your changes are really good, @JanEggers. I just want us to work in a (reasonably) professional way here and try to get a good process implemented.

I added a couple of notes also at #212. I wonder if we should try to get everything up to b4201d1 merged first (with the necessary cleanups there), and then try to structurally rebase @JanEggers changes on top of that (one small change at a time).

[JanEggers]

lol in time

[jornh]

Even in real time - I wonder if we can use github issues for chat 😄

[perlun]

Hehe, it's good that you are also thinking we should merge up to JanEggers@b4201d1 first. 😃 The way to handle multiple open pull requests is by means of git branches. You can create a new branch for this, and move your CefSharp3 branch pointer to point at b4201d1. That way, you can then more easily keep track of the different branches (which they are, conceptually).

Anyway, as said please don't take my "criticism" as anything else than "constructive criticism". I'm just trying to make sure everything gets done in a good fashion.

[perlun]

I wonder if we can use github issues for chat

Why not? Ping! 😉

[jornh]

LOL

[JanEggers]

i totally agree with you perlun just tell me what i have to do on the changes in b4201d1 to get them in and i will get that done.

and im using vs 2013 to compile so i wonder why 2012 should not work ?

[jornh]

Great - I actually feel the same as you both expressed about the size of things (waaay over the level where I could contribute anything - I just didn't want to be the picky guy this time around 😉). Also after my first comment today I tried it out with x64/xxx and Win32/Release and it failed to compile (I can dig out details if you want). I can go back and try out b4201d1 if you like or I can head over to my Native NuGet task.

Still it feels like a great "sketch" to work from!

[perlun]

Still it feels like a great "sketch" to work from!

Absolutely, it's a good "basis for discussion" if you like. I'll see if I can dig out some "quality time" 😃 within the next few days to try and get b4201d1 sorted.

[jornh]

Ah yes, "basis for ..." Is probably a more diplomatic term - even though a sketch can be pretty. And Jan you certainly did sprint yourself into the charts https://github.com/JanEggers/CefSharp/graphs/contributors

[JanEggers]

so fixed builds on all combinations im able to test atm seems i was a bit naive.

i try to split things up in different branches now.

i like the fact git calls merging cherry picking ^^ i have to use the clue brick some more to get in my head that git != svn

edit: i managed to create a private branch containing all my changes so i can seperate them and a new branch for the core libary and one for the pending pr and i will do another one for the refactoring im planning on the WPF WebView with Commands and stuff

but i cant reverse merge / rollback the cefsharp3 branch to the pr :(

[JanEggers]

i need another advice on xunit.

im using xunit.net to run the tests from vs2013 the problem is it does not set the working directory to the tests output dir and when the subprocess tries to load asseblies it looks somewhere in the vs directory.

what testrunner do you use perlun?

[perlun]

edit: i managed to create a private branch containing all my changes so i can seperate them and a new branch for the core libary and one for the pending pr and i will do another one for the refactoring im planning on the WPF WebView with Commands and stuff

Sounds great! That should make things much more maintainable I think.

but i cant reverse merge / rollback the cefsharp3 branch to the pr :(

Don't worry, I can choose to only merge the latest "known good" changeset. If you want to rollback, you can use the right-click and "Reset CefSharp3 to this commit" option (presuming you have checked out the CefSharp3 branch first). That should make it possible to move your local branch pointer for CefSharp3, which you can then push (possibly by having to use the "force" option which isn't easy to find in the GUI, but available if you enable it in the SourceTree preferences).

However, don't try this before you have some other branch for your latest changesets; otherwise, they will be very hard to find. 😃 They are still there, but SourceTree will hide them from you...

[perlun]

im using xunit.net to run the tests from vs2013 the problem is it does not set the working directory to the tests output dir and when the subprocess tries to load asseblies it looks somewhere in the vs directory.

what testrunner do you use perlun?

To be honest, I don't do test-based development much at all... but I've used ReSharper and NCrunch.net a bit.

But I think we first need to ask ourselves the question: what do we need/want to test? The subprocess etc. stuff makes things a bit complex. I think perhaps testing individual classes/methods is a better approach (and in that case, our own classes and their methods, in as much separation as is possible - i.e. best without even launching up the CEF stuff).

Do you see my point? Of course we can also make tests which rely on CEF/Subprocess stuff, but it feels more like integration testing than trivial unit-testing.

[JanEggers]

i got the branch where it should be but when i try to push i get

! [rejected] CefSharp3 -> CefSharp3 (non-fast-forward)

[perlun]

That's because you need to force-push to get that done. Try looking in the SourceTree preferences for an option that enables support for that.

[JanEggers]

i see u already merged it can i now delete the branch i created for the pr ?

[perlun]

Yes you can! There should be a button for you in the PR that lets you delete the branch.

[perlun]

(signing off now for tonight btw)

[JanEggers]

@perlun you may close this issue from my pov