search

ataranto / CefSharp

.Net binding for the Chromium Embedded Framework
Other
62 stars 34 forks source link

Added support for exposing CLR properties to Javascript #55

Closed perlun closed 11 years ago

oconnor663 commented 11 years ago

Yay!

ataranto commented 11 years ago

this is a super noisy diff. did you change a bunch of indentation? mix spaces/tabs?

perlun commented 11 years ago

Ah, sorry about that @ataranto... I realize there is some noise in there yes (for example in commit https://github.com/perlun/CefSharp/commit/ca972559c9963f6612f28a4829fe33a58785257c).

It seems like the VS2008-based code base is written with spaces (like I normally do in other projects as well). I guess VS2012 has its default C++ indentation set to tabs, and I have reformatted some files... My apologies. I will change to spaces and reindent the files in question (TypeUtil.cpp, BindingHandler.cpp and possibly some others) to make the diff less noisy, so that you can see the real changes more easily...

perlun commented 11 years ago

Well, it got slightly better. Still, some of the files were arguably incorrectly indented before (like WebView.h); it had only one level of indentation rather than two (since it was in the CefSharp.Wpf namespace). I agree this could have been done intentionally to avoid excessive indentation, but it's still a slightly bad idea since it makes it harder to reformat the file (which I feel something you should be able to do without too much fuss).

I suggest you look at the changes with whitespace modifications disabled for the time being (not sure if that can be done here on Github, but a standalone git GUI should be able to let you do it).

Anyway, to make things a little bit easier for you I'll summarize my changes here briefly:

If there's anything you feel should be done differently in all of this, please let me know and we'll hopefully be able to come to an agreement. I'm all open for ideas; I don't do any other C++ programming these days (but very much C#). So this is a quite interesting mix of a hobby & work for me at the moment, refreshing some ancient skills of mine... :)

ataranto commented 11 years ago

I don't really have a preference wrt formatting, tabs spaces, etc. I have tried to use the same formatting that was used in the project when I inherited maintainership. I wouldn't be averse to changing the formatting of every file, but I'd prefer to do that, or other things such as changing the Stdafx.h casing as separate commits/PRs.

perlun commented 11 years ago

I get your point... It was a separate commit actually, on my own fork, but it was all squashed into the same PR. Sorry about that. I do think you have a bit of understanding of that workflow though (you fix "other stuff" that you notice in the meanwhile, while working on the "real feature" you're implementing). To me, that's how I work every day at work, but that's a slightly different thing since we don't do pull requests etc...

So, all in all, once more sorry about the noise. I will try to be more "to the point" in my future pull requests (yes, there's definitely more coming :) so that it gets easier for you to review the modifications I've done.

perlun commented 11 years ago

@ataranto: It might be worthwhile to look at my other PR first (https://github.com/ataranto/CefSharp/pull/53). If we can manage to get that incorporated, it should make this PR (#55) be slightly less noisy.

ataranto commented 11 years ago

Agree, but I haven't started using VS2012 yet and VS2012 can't build chromium, so I'm not sure if we want to make it the default for CefSharp.

perlun commented 11 years ago

Okay, see your points. (But as I mentioned elsewhere, VS2012 can build chromium - https://groups.google.com/a/chromium.org/forum/?fromgroups=#!topic/chromium-dev/ftnAUIPBP9Y, read the last message in that thread)

If you aren't using VS2012 yet I can understand you're not so keen on pulling that stuff right in. ;) I don't see so much point in using VS2010 though (except for the Chromium point which might be moot), since the lack of C++ intellisense is quite bad and it doesn't feel that future proof anyway.

Please, let me know what you want me to do - I really want to cooperate with you on getting this stuff in. I could backport the properties change to VS2008 but it means a bit of work so I don't want to do it unless it's really necessary. :-)

ataranto commented 11 years ago

I agree that VS2008 support has to go, it's probably prevented a lot of people from contributing to CefSharp. VS2010 is still the "official" supported version to build chromium on windows.

perlun commented 11 years ago

I did some more changes to this, so that it's now rebased on top of @mwisnicki's excellent work on the vs2012 branch. Will add another feature shortly, PR coming... :)

Btw, do you happen to have any idea of if/when you will be able to review this stuff?

ataranto commented 11 years ago

no timeframe. i'm not actively working on CefSharp at this time.

perlun commented 11 years ago

No worries. Would you mind if we make an intermediate CefSharp release in the meanwhile? I know I have some stuff I want to bring out, and there may be other people having pending PR:s that could get included also.

perlun commented 11 years ago

Closing now since moved into master @ cefsharp/CefSharp.