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

Async .net side waiter for bound objects #2362

Closed
mitchcapper opened this issue Apr 14, 2018 · 14 comments
Closed

Async .net side waiter for bound objects #2362

mitchcapper opened this issue Apr 14, 2018 · 14 comments

Comments

@mitchcapper
Copy link
Contributor

mitchcapper commented Apr 14, 2018

I mentioned this briefly previously but I think it might be of value to have a .net side task that you could await for an object to be bound into existence. This can be partially accomplished with the nice ObjectBoundInJavascript event but as it doesn't fire if already bound it cannot be done as easily.

This may make it easier for people to move from the old binding to the new binding system if they only

use bound objects to exfil data from JS and not get results of methods.

It allows for something like:

helper = new Helper(browser);
browser.JavascriptObjectRepository.Register("Tester", new TestClass(), true, CefSharp.BindingOptions.DefaultBinder);
await helper.EnsureRegistered("Tester");
browser.ExecuteScriptAsync("Tester.test('hi there');");

Attached is a .net helper class that emulates the process that I could see being used. It has added complexity as we cannot tell from .net if an object is already bound (I guess could call EvaluateScriptAsync to test each window object). This could be resolved with another event handler of ObjectAlreadyBoundInJavascript called for any time bindobjectasync is called with an already bound object.

Helper.cs

Let me know thoughts or if worthwhile. This helper works for me (and actually could be simplified to just wait for BindObjectAsync rather than resolving after each bound object). Part of the current flow is to show what would be possible with the additional event handler, just one line of code with no helper class bound into the JS namespace. It could be taken a step further with no JS injection if the bind call was possible from .net code.

@amaitland
Copy link
Member

This can be partially accomplished with the nice ObjectBoundInJavascript event but as it doesn't fire if already bound it cannot be done as easily.

Adding a custom set of EventArgs for ObjectBoundInJavascript and adding some extra properties like bool AlreadyBound and bool IsCached would be pretty trivial. The question I have does it make sense to do so? Would be be confusing in any way?

I was originally tossing up between ObjectBoundInJavascript and ObjectsBoundInJavascript with the latter being called a single time with a list of all bound objects, maybe it's worth adding a second event with the different behavior?

Part of the current flow is to show what would be possible with the additional event handler, just one line of code with no helper class bound into the JS namespace.

Do you think a totally new event specially for this purpose would be preferable? If so what would it be called?

It could be taken a step further with no JS injection if the bind call was possible from .net code.

My initial though on automatically binding objects upon context creation were that it's not particularly helpful when Javascript was driving the interaction. The simplest possible option that should work currently would be implementing IRenderProcessMessageHandler and using some code similar to:

void IRenderProcessMessageHandler.OnContextCreated(IWebBrowser browserControl, IBrowser browser, IFrame frame)
{
	frame.ExecuteJavaScriptAsync("CefSharp.BindObjectAsync();");
}

The advantage of executing the code in the IRenderProcessMessageHandler is you get more control, only bind for certain frames, etc. Adding an option to the render process it's self would be an more than likely a simple bool, I was trying to avoid that as it's not very user friendly in my opinion.

Probably need a semi real work use case to make sure whatever option choose is easy enough to use.

@mitchcapper
Copy link
Contributor Author

I thought about modifying the current event, I would consider that a breaking change however as if you currently count on it to only fire on first bind it now is called more often. I am happy with that solution though not sure about others although at 63 is fairly new it may not be a big deal.

ObjectBoundInJavascript is fine I think. In JS users may call it once or over multiple lines.

I agree allowing users to control when object bind is useful, especially as if they do want autobinding they can implement something like OnContextCreated.

The main goal of all of this was just to give a way from c# to make sure something is bound. If there is a new function or existing function with args that is always called is greatly simplifies a helper class to something like the following: Helper.cs

@amaitland
Copy link
Member

I thought about modifying the current event, I would consider that a breaking change however as if you currently count on it to only fire on first bind it now is called more often.

Can modify BindObjectAsync slightly, check if the first/last param is an object and read it's properties. I did consider adding something like this before, can also add extra params like ignore cache.

would be something like CefSharp.BindObjectAsync({ NotifyIfAlreadyBound : true }, "myObject"), or could be the last param, not sure which is more intuitive.

I'd also change the event args to add extra properties, AlreadyBound, IsCached, etc.

@mitchcapper
Copy link
Contributor Author

I would vote for first on a method that takes an open ended array of args. Any of these changes sound great.

@amaitland
Copy link
Member

Marking this as blocking as I think this should happen before 65 is released. I'll try and get it sorted next week.

amaitland added a commit that referenced this issue Apr 27, 2018
…ository.ObjectsBound event

This contains the breaking changes required for #2362
@amaitland
Copy link
Member

amaitland commented Apr 27, 2018

A rough implementation was merged into master with 8f7692a

TODO

  • Remove code duplication
  • Add more qunit tests
  • Update General Usage Guide

As it's not ready for wide spread release yet, I've merged the breaking changes into the cefsharp/65 branch with commit 801d53b

Will look at releasing a 65.0.0-pre01 set of packages very shortly.

Let me know what you think. You can see the new JS syntax at 8f7692a#diff-f99ba049add937c58daa5a3d4533da17R473

@mitchcapper
Copy link
Contributor Author

Syntax and event args seem easy to follow will certainly simplify binding resolution from .net as well.

@amaitland
Copy link
Member

Suggestions on naming welcome 😃

@mitchcapper
Copy link
Contributor Author

I assume the only items you are asking for naming feedback on are the new NotifyIfAlreadyBound / IgnoreCache options? The naming seems straightforward enough and while it is easy to use them as is I do wonder if NotifyIfAlreadyBound isn't better done as an option from the .net side rather than JS. It is a minor point however. NotifyEvenIfAlreadyBound, AlwaysGenerateEvent, AlwaysGenerateNotification, ForceNotify if you are looking for other options.

@amaitland
Copy link
Member

The naming seems straightforward enough and while it is easy to use them as is I do wonder if NotifyIfAlreadyBound isn't better done as an option from the .net side rather than JS

From JS you can change the param when you call it, from .Net if the objects are cached in the render process then it wouldn't be possible to change the notify option. The notification is triggered in the render process. Maybe it's not something that needs to be changed once set? I was thinking all new options going froward would be in JS unless there's a really strong case for them to be in .Net. I was going to look at adding a parent param, so instead of binding directly to window, you can provide an object that becomes the parent. I'm sure there will be other params for use cases that I'm not aware of yet.

NotifyEvenIfAlreadyBound, AlwaysGenerateEvent, AlwaysGenerateNotification, ForceNotify if you are looking for other options.

Probably just go with the current name unless there's a strong opinion for something else.

amaitland added a commit that referenced this issue May 9, 2018
To support this use case added IJavascriptObjectRepository.ObjectsBoundInJavascript (called a single time with names of all objects bound)

Implements the idea put forward in #2362
@amaitland
Copy link
Member

Have added EnsureObjectBoundAsync extension method, there's a new event called IJavascriptObjectRepository.ObjectsBoundInJavascript that simplifies the code greatly (hopefully the name isn't confusing, it's simple called a single time when the object bound message is received.

Example looks something like:

browser.EnsureObjectBoundAsync("bound").ContinueWith(t =>
{
  var boundObjects = t.Result;
});

Obviously you can await the Task. Still needs a bit more testing/cleanup. It doesn't check browser.CanExecuteJavascriptInMainFrame like the example provided, there's still issue #2250 that makes using that unreliable after a process switch. Let me know what you think.

@mitchcapper
Copy link
Contributor Author

ObjectsBoundInJavascript certainly simplifies it and as a user can use either shouldn't make it too difficult for users.

EnsureObjectBound helper will likely be useful for others so should make a nice addition for transitioning existing code.

amaitland added a commit that referenced this issue May 16, 2018
To support this use case added IJavascriptObjectRepository.ObjectsBoundInJavascript (called a single time with names of all objects bound)

Implements the idea put forward in #2362

# Conflicts:
#	CefSharp/Internals/JavascriptObjectRepository.cs
@amaitland
Copy link
Member

Section added to the General Usage Guide see https://github.com/cefsharp/CefSharp/wiki/General-Usage/_compare/f3ded5d15cb167569c62bf4134f3bb1b049764e9...72cdf2f2d5666db7cfac100f35494c54a0562755

I'm not totally happy with the formatting, overall the whole JSB section could use some work, there are quite a few options now. Open to suggestions for how to improve the formatting and make things clearer.

@amaitland
Copy link
Member

65.0.0 should appear on Nuget.org shortly.

The documentation still needs some work, so I've creating #2455

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants