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

Rename IResourceHandler to IResourceHandlerFactory. #857

Merged
merged 4 commits into from
Mar 5, 2015

Conversation

rassilon
Copy link
Contributor

@rassilon rassilon commented Mar 3, 2015

Start the process of cleaning up IResouceHandler by renaming it something sensible.

Thoughts?

If you think the name is ok, I'll push a rename of the files.

Bill

{
void RegisterHandler(string url, ResourceHandler handler);

void UnregisterHandler(string url);

/// <summary>
/// Called before a resource is loaded. To specify a handler for the resource return a ResourceHandler object
/// Called before a resource is loaded. To specify a handler for the resource return a ResourceHandlerFactory object
Copy link
Member

Choose a reason for hiding this comment

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

Little bit to aggressive with the rename? I think this should still be ResourceHandler

@amaitland
Copy link
Member

I'm happy with IResourceHandlerFactory 👍 Naming was confusing before.

I think the rename may have been a little too aggressive, at least a couple of instances that I think should stay as ResourceHandler.

Other than that, rename away.

@@ -54,11 +54,11 @@ public interface IWebBrowser : IDisposable
void Load(string url);

/// <summary>
/// Registers and loads a <see cref="ResourceHandler"/> that represents the HTML content.
/// Registers and loads a <see cref="ResourceHandlerFactory"/> that represents the HTML content.
Copy link
Member

Choose a reason for hiding this comment

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

Possibly this should stay the same as well? (And the next few instances).

Feel free to improve the comments as you see fit.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes they should. Fixed.

@amaitland
Copy link
Member

@jankurianski Is renaming going to cause you any problems?

@amaitland
Copy link
Member

Will wait for feedback from @jankurianski before merging this one as he's likely to be affected.

Were you planning any other changes? I know you'd mentioned switching to a ConcurrentDictionary

@rassilon
Copy link
Contributor Author

rassilon commented Mar 4, 2015

Yeah, I do want to switch this to a ConcurrentDictionary. I figured the names might be a blocker, so I figured do this bit first, and then do the next.

After ConcurrentDictionary is probably renaming of the Scheme ResoureHandlerWrapper class to prep for creating the ability to do ResourceHandler's via async.

Either via a known ResourceHandler subtype i.e. AsyncResourceHandler and the C++ caller of ResourceHandlers can do the right thing based on being an AsyncResourceHandler child or not.
Or, by changing the IResourceHandlerFactory interface GetResourceHandler method to return an IResourceHandler instead.

Bill

@amaitland
Copy link
Member

@rassilon Are you interested in joining Team CefSharp? (Long as @jornh gives his blessing of course)

@rassilon
Copy link
Contributor Author

rassilon commented Mar 5, 2015

Sure. I think the AsyncResourceHandler might be the simplest way to go forward in order to minimize backward compat problems.

Bill

@amaitland
Copy link
Member

I think the AsyncResourceHandler might be the simplest way to go forward in order to minimize backward compat problems.

Sounds like the best of both worlds 👍

@jankurianski
Copy link
Member

@amaitland My apologies for the delay - I am not affected by this.

I also think @rassilon would be a great addition to the team!

@amaitland
Copy link
Member

@amaitland My apologies for the delay - I am not affected by this.

No problem 👍 Thanks for confirming.

amaitland added a commit that referenced this pull request Mar 5, 2015
Rename IResourceHandler to IResourceHandlerFactory.
@amaitland amaitland merged commit 6fca20b into cefsharp:master Mar 5, 2015
@amaitland
Copy link
Member

@rassilon Are you interested in joining Team CefSharp? (Long as @jornh gives his blessing of course)

@jornh Any comments on my idea?

@jornh
Copy link
Contributor

jornh commented Mar 12, 2015

Sorry Alex, must have overlooked this one 😊 I'm very much in favour of adding @rassilon to the team as well.

Bill, welcome aboard! From what I've seen you have already contributed significantly to CefSharp in a very short span of time... Very much appreciated 👍

@amaitland
Copy link
Member

Sorry Alex, must have overlooked this one 😊

No problem at all 😄

@amaitland amaitland added this to the 39.0.0 milestone Mar 26, 2015
@rassilon rassilon deleted the CleanupResourceHandler branch April 22, 2015 23:35
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