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

Added parameter for lowercasing JS functions (or not) #923

Merged
merged 7 commits into from
Apr 10, 2015

Conversation

exinferis
Copy link
Contributor

Used to give the dev the possibility to decide wether JS methods/objects/parameters should be automatically made first letter lowercase or not.

@exinferis
Copy link
Contributor Author

This is the suggested fix for #922

@@ -661,9 +661,9 @@ namespace CefSharp
}
}

void RegisterJsObject(String^ name, Object^ object)
void RegisterJsObject(String^ name, Object^ object, bool lowerCaseJavascriptNames)
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

Comments inline, needs a bit of refactoring and cleanup. Nothing major though.

@amaitland amaitland added this to the 39.0.0 milestone Apr 2, 2015
@amaitland
Copy link
Member

Thinking about this a little more, might be worth just using an optional param instead of overloading, keeps the lines of code down as they're mostly just wrapper methods until they get passed for analysis.

Also I think IWebBrowser probably needs to be updated to include the new signature for whatever we go with.

@exinferis
Copy link
Contributor Author

Shall I change my pull request to fulfill the coding guidelines etc, or would you suggest to implement the alternative solution using optional parameters?

@amaitland
Copy link
Member

I think optional params makes sense. What do you think?

@exinferis
Copy link
Contributor Author

I do not know the internals of the project well enough to really be sure, but I also think that an optional parameter makes sense.
Why and where needs the IWebBrowserto be updated in that case?

@amaitland
Copy link
Member

It's only the external methods that require optional params, specifically those on the ChromiumWebBrowser instances, all other methods just need the value passed.

The IWebBrowser.RegisterJsObject method will need to be updated including xml docs.
https://github.com/cefsharp/CefSharp/blob/master/CefSharp/IWebBrowser.cs#L89

@amaitland amaitland modified the milestones: 39.0.1, 39.0.0 Apr 7, 2015
amaitland added a commit that referenced this pull request Apr 10, 2015
Added parameter for lowercasing JS functions (or not)
@amaitland amaitland merged commit d752528 into cefsharp:master Apr 10, 2015
@perlun
Copy link
Member

perlun commented Apr 13, 2015

Coming in a bit late here, but just a suggestion: lowercaseMethodNames isn't really a proper name, since it's not about "lower-casing" per se, but rather "javascript-casing". So "javascriptCaseMethodNames" or something to the like would make more sense, IMHO. What do you think @amaitland?

@perlun
Copy link
Member

perlun commented Apr 13, 2015

(just so we keep the public API clean)

@amaitland
Copy link
Member

So "javascriptCaseMethodNames" or something to the like would make more sense,

Hadn't really though about it 😄 As it applies to both properties as well, it should probably have a more generic name. I'm all for improving variable naming as a general thing 👍

@perlun Do you have time to submit a PR?

@amaitland
Copy link
Member

(Or even just apply the changes directly to master)

@exinferis
Copy link
Contributor Author

That means that you go under the assumption, that all js functions are per se lowercase (so javascriptCaseMethodNames refers to lowercase) which isn't always the case - this is a widespread behaviour, but not necessary nor wanted in many cases. I find javascriptCaseMethodNames misleading - it is about lowercasing, since "javascript-casing" does not really exist as such.

@amaitland
Copy link
Member

Technically it's probably camel-case that we're referring to, to me that's a fairly universal term. So something like camelCaseJavascriptNames is probably quite universal. Thoughts?

@exinferis
Copy link
Contributor Author

Sounds good.

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