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

Explicit implement internal interface #302

Merged
merged 5 commits into from
May 21, 2014
Merged

Explicit implement internal interface #302

merged 5 commits into from
May 21, 2014

Conversation

JanEggers
Copy link
Contributor

No description provided.

@JanEggers
Copy link
Contributor Author

any news on this one ?

@jornh
Copy link
Contributor

jornh commented May 13, 2014

Same problem with this one. Needs a merge of master so it can be auto-merged.

@JanEggers
Copy link
Contributor Author

@jornh im done also added my changes to the win form portion.

ready for review / merge

@amaitland
Copy link
Member

@jornh What do you think about making this PR next in line to be merged?

@JanEggers Any chance you can merge in master again? Apologies for having to do it twice! As stated above I'd like to get this resolved ASAP 😄

@jornh
Copy link
Contributor

jornh commented May 20, 2014

I agree - go ahead! Tightening the API up like this before getting out of beta potentially affects many people

@jornh jornh changed the title Explit implement internal interface Explicit implement internal interface May 20, 2014
@@ -6,9 +6,11 @@ public interface IWebBrowserInternal : IWebBrowser
{
IDictionary<string, object> BoundObjects { get; }

//TODO: shouldnt this be part of IWebBrowser
Copy link
Member

Choose a reason for hiding this comment

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

Once this PR is merged, I'd like to get some more API cleanup done, moving these to IWebBrowser is on that list.

Copy link
Member

Choose a reason for hiding this comment

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

Also ShowDevTools and CloseDevTools should probably be moved to IWebBrowser as well. I'm thinking in the short term we move them as it's a minor change, and update down the track when we have a clearer picture of what's happening with DevTools in general.

@JanEggers
Copy link
Contributor Author

@amaitland: its up to date

@amaitland
Copy link
Member

@JanEggers Thank you! "Merge pull request" button has returned 😄

@amaitland
Copy link
Member

I've looked over it twice now and everything looks in order 👍

@jornh Mind running a pair of eyes over this when you get a second?

@jornh
Copy link
Contributor

jornh commented May 21, 2014

Will do that after work/tonight

@amaitland - also good work on getting those other PRs refreshed etc. lets
just hope they won't collide to much. Better get this one reviewed quickly:)

On Wednesday, May 21, 2014, amaitland [email protected] wrote:

I've looked over it twice now and everything looks in order [image: 👍]

@jornh https://github.com/jornh Mind running a pair of eyes over this
when you get a second?


Reply to this email directly or view it on GitHubhttps://github.com//pull/302#issuecomment-43719767
.

@jornh
Copy link
Contributor

jornh commented May 21, 2014

Just looking at webView.o... with IntelliSense before and after ... just has less unneeded method clutter - but I found the events I wanted to hook into ... well what can I say ... other than ...? 😄

I only took a few samples though trusting you guys already did a stellar job. I'd say merge away @amaitland!

perlun added a commit that referenced this pull request May 21, 2014
@perlun perlun merged commit 5741485 into cefsharp:master May 21, 2014
@perlun
Copy link
Member

perlun commented May 21, 2014

Sorry @amaitland, couldn't help myself... 😜 😃

@JanEggers JanEggers deleted the ExplitImplementInternalInterface branch May 21, 2014 20:48
@amaitland
Copy link
Member

@jornh Thanks!
@perlun I'm just pleased that it's merged 😄

@jornh jornh added this to the 31.0.0 milestone May 27, 2014
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.

4 participants