JSB - Forward binding result to BindObjectAsync #5091
Conversation
WalkthroughThis set of changes refactors the internal handling of JavaScript object binding and promise resolution within the CEF/CefSharp browser subprocess. The code now uses direct V8 promise creation and a unified promise object for asynchronous callbacks, replacing manual script evaluation and separate resolve/reject handlers. Several binding methods are renamed and refactored to return V8 values directly, streamlining the conversion of managed objects to native JavaScript objects. Helper methods are introduced to standardize the structure of bind results. The test suite is updated to use single quotes and includes a new test to validate the structure of the binding result object. Changes
Sequence Diagram(s)sequenceDiagram
participant JS as JavaScript
participant Handler as JavascriptAsyncMethodHandler
participant Callback as JavascriptAsyncMethodCallback
participant V8 as V8 Engine
JS->>Handler: Call async method
Handler->>V8: CreatePromise()
Handler->>Callback: Construct with promise
Handler->>JS: Return promise
Callback-->>V8: On success/failure, call ResolvePromise/RejectPromise
V8-->>JS: Promise resolved or rejected
Poem
✨ Finishing Touches
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (5)
CefSharp.BrowserSubprocess.Core/Async/JavascriptAsyncMethodCallback.cpp (2)
16-26: Use RAII to guaranteeExit()even on unexpected exceptions
_context->Exit()is placed in afinallyblock which is fine in C++/CLI, however we still repeat theEnter()/Exit()dance in two methods and rely on the developer to remember to wrap new call-sites the same way.
A small RAII helper (e.g.ContextScope ctx(_context);) that callsEnter()in the ctor andExit()in the dtor would:
- remove the duplicated
if (_context && _context->Enter()) … Exit()pattern- ensure
Exit()is invoked even if a newreturnis inserted before thefinally- make the code less error-prone for future contributors
No functional change now, but worth considering before more async callbacks are added.
31-41: Factor common logic betweenSuccessandFailBoth branches only differ by the
ResolvePromise/RejectPromisecall yet repeat context handling and null checks.
Extracting a private templated helper that accepts a lambda ([](auto p){ p->ResolvePromise(val); }) will improve readability and keep the two public methods trivially thin.CefSharp.BrowserSubprocess.Core/Async/JavascriptAsyncObjectWrapper.cpp (2)
21-23: Remove unused variableobjectName
objectNameis computed but never used after the refactor, leaving dead code and an unnecessary UTF-16→UTF-8 conversion.- auto objectName = StringUtils::ToNative(object->JavascriptName);🧰 Tools
🪛 Cppcheck (2.10-2)
[error] 21-21: syntax error
(syntaxError)
18-33: Consider early-out whenobject->Methodsis empty
ConvertToV8Valueallocates and registers bookkeeping objects even when no methods exist, leading to an empty V8 object and an unnecessary entry in_wrappedMethods.
A guard clause could skip wrapper allocation in this corner-case.if(object->Methods->Count == 0) { return javascriptObject; // nothing to bind }🧰 Tools
🪛 Cppcheck (2.10-2)
[error] 21-21: syntax error
(syntaxError)
CefSharp.Example/Resources/BindingTestAsync.js (1)
292-309: Simplify key-existence assertionsUsing
Object.getOwnPropertyDescriptorsis verbose and yields descriptor objects that are then coerced to booleans.
assert.ok('count' in bindResult)(orhasOwnProperty) makes intent clearer and works for both data & accessor properties.-const keys = Object.getOwnPropertyDescriptors(bindResult); -assert.equal(true, !!keys['count'], 'count'); +assert.ok('count' in bindResult, 'count');This also avoids false negatives if a property is inherited in a future refactor.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (14)
CefSharp.BrowserSubprocess.Core/Async/JavascriptAsyncMethodCallback.cpp(2 hunks)CefSharp.BrowserSubprocess.Core/Async/JavascriptAsyncMethodCallback.h(1 hunks)CefSharp.BrowserSubprocess.Core/Async/JavascriptAsyncMethodHandler.cpp(1 hunks)CefSharp.BrowserSubprocess.Core/Async/JavascriptAsyncObjectWrapper.cpp(2 hunks)CefSharp.BrowserSubprocess.Core/Async/JavascriptAsyncObjectWrapper.h(1 hunks)CefSharp.BrowserSubprocess.Core/BindObjectAsyncHandler.h(4 hunks)CefSharp.BrowserSubprocess.Core/CefAppUnmanagedWrapper.cpp(1 hunks)CefSharp.BrowserSubprocess.Core/CefAppUnmanagedWrapper.h(0 hunks)CefSharp.BrowserSubprocess.Core/JavascriptObjectWrapper.cpp(2 hunks)CefSharp.BrowserSubprocess.Core/JavascriptObjectWrapper.h(1 hunks)CefSharp.BrowserSubprocess.Core/JavascriptPropertyWrapper.cpp(1 hunks)CefSharp.BrowserSubprocess.Core/JavascriptRootObjectWrapper.cpp(2 hunks)CefSharp.BrowserSubprocess.Core/JavascriptRootObjectWrapper.h(1 hunks)CefSharp.Example/Resources/BindingTestAsync.js(4 hunks)
💤 Files with no reviewable changes (1)
- CefSharp.BrowserSubprocess.Core/CefAppUnmanagedWrapper.h
🧰 Additional context used
🧬 Code Graph Analysis (2)
CefSharp.BrowserSubprocess.Core/BindObjectAsyncHandler.h (1)
CefSharp.BrowserSubprocess.Core/Async/JavascriptAsyncMethodCallback.h (1)
JavascriptAsyncMethodCallback(34-37)
CefSharp.BrowserSubprocess.Core/JavascriptRootObjectWrapper.cpp (1)
CefSharp.BrowserSubprocess.Core/JavascriptRootObjectWrapper.h (1)
Bind(106-106)
🪛 Cppcheck (2.10-2)
CefSharp.BrowserSubprocess.Core/JavascriptRootObjectWrapper.cpp
[error] 19-19: syntax error
(syntaxError)
[error] 21-21: syntax error
(syntaxError)
🔇 Additional comments (16)
CefSharp.BrowserSubprocess.Core/JavascriptPropertyWrapper.cpp (1)
25-29: Improved object conversion and property setting approachThe modification properly leverages the new
ConvertToV8Valuemethod to obtain a V8 object representation first, then explicitly sets it as a property on the target object. This approach is more consistent with the broader refactoring of JavaScript object binding across the codebase.CefSharp.BrowserSubprocess.Core/JavascriptRootObjectWrapper.h (1)
106-106: Method signature enhancement to return V8 valueThe method signature change from
voidtoCefRefPtr<CefV8Value>return type aligns with the broader refactoring to standardize the conversion and binding approach. This allows the binding result to be forwarded directly, supporting the PR objective of enabling direct access to the binding result.CefSharp.BrowserSubprocess.Core/Async/JavascriptAsyncObjectWrapper.h (1)
41-41: Method renamed and improved to return V8 valueThis change is a key part of the refactoring that replaces void binding methods with methods that return V8 values. Renaming from
BindtoConvertToV8Valuemore accurately describes the method's purpose and enables the promised functionality of forwarding binding results toBindObjectAsync.CefSharp.BrowserSubprocess.Core/CefAppUnmanagedWrapper.cpp (2)
651-652: Capture and use binding resultThis change effectively captures the result from the
Bindmethod, which is now returning a V8 value. This is essential for forwarding the binding result to client code as stated in the PR objectives.
659-664: Standardized bind result creationUsing the
CreateBindResulthelper method provides a more consistent structure for bind results, enabling the new syntax shown in the PR objectives:const result = await cefSharp.bindObjectAsync('boundAsync2'). This is a cleaner approach than manually constructing the result object.CefSharp.BrowserSubprocess.Core/Async/JavascriptAsyncMethodHandler.cpp (1)
27-30: Great improvement using native promise creation instead of script evaluationThis change elegantly replaces the previous approach of manually evaluating a JavaScript script to create promises with the more direct
CefV8Value::CreatePromise()API. This is more efficient and easier to maintain since it eliminates the dependency on a string-based script.The simplified callback constructor now takes a single promise object rather than separate resolve/reject callbacks, which better aligns with how promises are conceptually meant to be used.
CefSharp.BrowserSubprocess.Core/JavascriptObjectWrapper.h (1)
59-59: Good API design change from void to return valueRenaming
BindtoConvertToV8Valueand changing the signature to return aCefRefPtr<CefV8Value>rather than modifying a passed-in parameter is a solid improvement. This follows functional programming principles by reducing side effects and making the method's purpose more explicit.CefSharp.BrowserSubprocess.Core/Async/JavascriptAsyncMethodCallback.h (2)
19-24: Clean up promise handling with unified member variableReplacing separate
_resolveand_rejectmembers with a single_promisemember simplifies the code and better represents the conceptual model of promises. The constructor signature change to accept a single promise object instead of separate callbacks is a natural extension of this refactoring.
31-31: Consistent cleanup in destructorThe destructor correctly resets the new
_promisemember instead of the previously separate callback references, ensuring proper resource cleanup.CefSharp.BrowserSubprocess.Core/JavascriptObjectWrapper.cpp (2)
15-27: Method signature change improves API designThe implementation now properly creates and returns a V8 object directly instead of modifying a passed reference. This is consistent with the header changes and makes the method's purpose clearer.
🧰 Tools
🪛 Cppcheck (2.10-2)
[error] 19-19: syntax error
(syntaxError)
43-45: Clean return of constructed objectThe method now returns the constructed JavaScript object directly, aligning with the refactored method signature. This makes the code flow more intuitive and eliminates the side effect of modifying a passed-in parameter.
CefSharp.BrowserSubprocess.Core/JavascriptRootObjectWrapper.cpp (1)
50-52: Duplicate storage of the same V8 objectThe same
v8Objis assigned to bothresultandglobal.
If the intent is to supply a lightweight “summary” object back to JS callers, sharing references is fine; otherwise consider cloning or using different property attributes.
Please make sure later mutations from user code do not inadvertently leak back into the global instance.CefSharp.BrowserSubprocess.Core/BindObjectAsyncHandler.h (4)
132-171: Improved promise handling with native CEF promise API.The code now uses
CefV8Value::CreatePromise()to create a native promise at the beginning of the frame validity check. This replaces the previous approach that likely used a JavaScript-based promise creation mechanism. The promise is now consistently passed through the code flow and properly resolved with a structured result created by the newCreateBindResultmethod.This implementation aligns with the PR objective to replace the existing
kPromiseCreatorScriptwith native promise creation and enables passing the binding result directly to the caller.
176-176: Simplified callback creation with unified promise object.The
JavascriptAsyncMethodCallbackconstructor now receives the entire promise object rather than separate resolve/reject functions. This simplifies the flow and reduces potential for errors in callback handling.
192-201: Consistent error handling with standardized result object.When objects are already bound, the code now uses the same structured response format via
CreateBindResult. This ensures a consistent interface for JavaScript consumers regardless of the binding outcome path. The single return value assignment (retval = resultPromise) at the end simplifies the code flow and makes it more maintainable.
222-241: Well-designed result structure with dual-case property support.The new
CreateBindResultmethod creates a standardized response object with both PascalCase and camelCase property names. This is an excellent practice for JavaScript interoperability, as it accommodates different coding conventions in the consuming code. The method encapsulates the creation logic, making it more maintainable and ensuring consistency across different response paths.The properties included (
Count,Message,Success, andBindResult) provide a comprehensive status report of the binding operation, which improves the developer experience.
| auto result = CefV8Value::CreateArray(objects->Count); | ||
| if (objects->Count > 0) | ||
| { |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Returning an array whose numeric indices are unset may be misleading
result is created with objects->Count but only named properties are populated.
Callers iterating over for(let i=0;i<result.length;i++) will receive undefined values.
Two alternatives:
- Use
CefV8Value::CreateObjectinstead of an array since consumers access objects by name (result.boundAsync2). - Or, additionally set the numeric indices while building the array.
- auto result = CefV8Value::CreateArray(objects->Count);
+ auto result = CefV8Value::CreateObject(nullptr, nullptr);Aligning the structure with actual usage avoids surprises.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| auto result = CefV8Value::CreateArray(objects->Count); | |
| if (objects->Count > 0) | |
| { | |
| auto result = CefV8Value::CreateObject(nullptr, nullptr); | |
| if (objects->Count > 0) | |
| { |
🧰 Tools
🪛 Cppcheck (2.10-2)
[error] 19-19: syntax error
(syntaxError)
[error] 21-21: syntax error
(syntaxError)
| @@ -2,209 +2,209 @@ QUnit.module('BindingTestAsync', (hooks) => | |||
| { | |||
| hooks.before(async () => | |||
| { | |||
| await CefSharp.BindObjectAsync("boundAsync"); | |||
| await CefSharp.BindObjectAsync('boundAsync'); | |||
There was a problem hiding this comment.
Please revert the styling change, it creates too much noise and doesn't actually provide a functional change.
Makes it very hard to review what's actually changed.
There was a problem hiding this comment.
Sorry. I did it out of habit.
|
Firstly, thanks for the
What's the use case for this? |
I don't have any special case. It's a request from colleagues. Because it seems more logical: you call bindObjectAsync and get the object you need, instead of looking for it somewhere in the window. |
I think a new method that creates a instance of the object without adding to the global scope would be preferable. |
|
@amaitland Initially, I considered adding a parameter to explicitly control whether binding to window should be used. |
This is my preference, I believe this will make for a better user experience. Much easier to document and provide examples for.
For me a new
Unless |
I'm not a fan of adding const boundAsync2 = await cefSharp.createObjectAsync('boundAsync2'); I think doing that in a separate
This is an excellent improvement and I'll cherry pick this change out of this |
Summary:
CefV8Value::CreatePromise()const result = await cefSharp.bindObjectAsync('boundAsync2'); const boundAsync2 = result.bindResult.boundAsync2;Types of changes
Checklist:
Summary by CodeRabbit
New Features
Refactor
Style
Tests