Skip to content

Conversation

santoshmgh
Copy link
Contributor

@santoshmgh santoshmgh commented Dec 21, 2021

Summary:

  • I have added property interceptor for Javascript Binding

Changes:

  • I have modified the TryGetProperty and TrySetProperty in JavascriptObjectRepository
  • I have PropertyInterceptor as the BindingOptions
  • Added IPropertyInterceptor to CefSharp.ModelBinding
  • Added new Unit Test cases for Property Interceptor to CefSharp.Test.JavascriptBinding.JavaScriptObjectRepositoryFacts
    How Has This Been Tested?
  • I have tested it using CefSharp.Wpf.Example

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)
  • Updated documentation

Checklist:

  • Tested the code(if applicable)
  • Commented my code
  • Changed the documentation(if applicable)
  • New files have a license disclaimer
  • The formatting is consistent with the project (project supports .editorconfig)

@AppVeyorBot
Copy link

Build CefSharp 97.0.80-CI4345 failed (commit 2d6bd9d484 by @)

@santoshmgh
Copy link
Contributor Author

@amaitland Kindly let me know if I am missing anything for the failing test case.

@amaitland
Copy link
Member

The failing test is #3867

Property interception is not available in .Net Core and will need to be excluded with an if def see https://github.com/cefsharp/CefSharp/blob/master/CefSharp/Internals/JavascriptObjectRepository.cs#L197 for example.

@AppVeyorBot
Copy link

@AppVeyorBot
Copy link

@santoshmgh
Copy link
Contributor Author

@amaitland Are we good to merge these changes?

@amaitland
Copy link
Member

Property interception is not available in .Net Core and will need to be excluded with an if def see https://github.com/cefsharp/CefSharp/blob/master/CefSharp/Internals/JavascriptObjectRepository.cs#L197 for example.

This will need to be addressed before this PR can be considered for merge.

A unit test should also be added. Testing the JavaScript object repository functionality directly should be sufficient. Add test to https://github.com/cefsharp/CefSharp/blob/master/CefSharp.Test/JavascriptBinding/JavaScriptObjectRepositoryFacts.cs

@santoshmgh
Copy link
Contributor Author

santoshmgh commented Jan 5, 2022

@amaitland
Thanks for sharing the above pointer.
If my understanding is correct Property interception execution in TryGetProperty and TrySetProperty should be excluded for NETCOREAPP. Please confirm.
Example: : TryGetProperty

#if NETCOREAPP
                result = property.GetValue(obj.Value);
#else
                if (obj.PropertyInterceptor == null)
                {
                    result = property.GetValue(obj.Value);
                }
                else
                {
                    result = obj.PropertyInterceptor.InterceptGet(() => property.GetValue(obj.Value), property.ManagedName);
                }
#endif

Additional Query
Should I need to take care of the same at any other place apart from TryGetProperty and TrySetProperty API?

@amaitland
Copy link
Member

Example: : TryGetProperty

To simplify things the whole TryGetProperty method can be excluded from the netcore implementation.

Should I need to take care of the same at any other place apart from TryGetProperty and TrySetProperty API?

Yes. You will need to compile the netcore solution and resolve all the errors.

Copy link
Member

@amaitland amaitland left a comment

Choose a reason for hiding this comment

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

Comments inline

@amaitland
Copy link
Member

@santoshmgh Please confirm if you are planning on implementing the required changes or if the PR should be closed. Thanks.

@santoshmgh
Copy link
Contributor Author

@amaitland I will be incorporating the above comments..

@santoshmgh
Copy link
Contributor Author

@amaitland Kindly let me know if I need to incorporate any other comments.

@AppVeyorBot
Copy link

@AppVeyorBot
Copy link

@santoshmgh
Copy link
Contributor Author

@amaitland Kindly let me know how can I run the test case from CefSharp.Test.dll locally. So that I can fix the failing test case.

@amaitland
Copy link
Member

Added new QUnit Test cases PropertyInterceptorLogger to CefSharp.Example.ModelBinding

I don't see a qunit test. Can you re-add it please.

Kindly let me know how can I run the test case from CefSharp.Test.dll

Tests can be run from within Visual Studio, nothing special required.

@AppVeyorBot
Copy link

@santoshmgh
Copy link
Contributor Author

@amaitland Added the unit test cases to CefSharp.Test.JavascriptBinding.JavaScriptObjectRepositoryFacts

@AppVeyorBot
Copy link

@amaitland
Copy link
Member

Added the unit test cases to CefSharp.Test.JavascriptBinding.JavaScriptObjectRepositoryFacts

Please add a separate test case rather than modifying the existing one.

@AppVeyorBot
Copy link

@santoshmgh
Copy link
Contributor Author

@amaitland I have added a separate test case for property Interceptor. Please let me know if you want to update the test name to something else.

@amaitland amaitland added this to the 98.1.x milestone Jan 30, 2022
@amaitland
Copy link
Member

Thanks. I'm not too worried about test naming, they can always be improved later without impacting anyone.

@AppVeyorBot
Copy link

@santoshmgh
Copy link
Contributor Author

@amaitland Please confirm if changes for property interceptor will be available as part of cefsharp 98 release.

@amaitland
Copy link
Member

@santoshmgh If you look at the Milestone you'll see this has already been added to 98.1.x.

@amaitland amaitland changed the title Core - added support for property interceptor Sync Javascript Binding - Add support for property interception Feb 14, 2022
@amaitland amaitland merged commit 9e02368 into cefsharp:master Feb 14, 2022
amaitland pushed a commit that referenced this pull request Feb 14, 2022
* Core - added support for property interceptor

* Incorporated the review comments

* Incorporated the review comments 2

* Review comments exclude trygetproperty and trysetproperty for .netcore app

* Added the unit test cases for trygetproperty and trysetproperty

* Fixed .net core app build failure

* Added the separate test case for property interceptor

Co-authored-by: sghanti <[email protected]>
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