Replaced kPromiseCreatorScript with native promise #5094
Conversation
|
Warning Rate limit exceeded@amaitland has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 6 minutes and 38 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (2)
WalkthroughThis update refactors the handling of JavaScript Promises in the asynchronous method and object binding logic. The previous approach, which involved evaluating JavaScript code to create promises and managing separate resolve and reject function pointers, is replaced by direct use of the V8 API to create and resolve/reject Promise objects. Helper methods are introduced for constructing result objects, and redundant script evaluation and error handling are removed. Several class interfaces are updated to reflect these changes, including constructor signatures and static member declarations. A new asynchronous test validates the structure of the result object fields. Changes
Sequence Diagram(s)sequenceDiagram
participant JS as JavaScript
participant Handler as AsyncMethodHandler
participant Callback as JavascriptAsyncMethodCallback
participant V8 as V8 Engine
JS->>Handler: Call async method
Handler->>V8: CreatePromise()
V8-->>Handler: Promise object
Handler->>Callback: Construct with promise object
Handler->>Callback: Register callback
Handler->>V8: Return promise to JS
alt On success
Callback->>V8: ResolvePromise(result)
else On failure
Callback->>V8: RejectPromise(exception)
end
Poem
✨ Finishing Touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 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: 2
🧹 Nitpick comments (2)
CefSharp.BrowserSubprocess.Core/BindObjectAsyncHandler.h (2)
174-176: Minor: keep a local ref to prevent premature GC of the promise
promiseis passed toJavascriptAsyncMethodCallback, butretvalis also set to it.
While CEF keeps the JavaScript-side alive, keeping an extraCefRefPtrin scope (auto promise = …;) until afterSaveMethodCallbackcan avoid edge-case ref-count churn:-CefRefPtr<CefV8Value> promise = CefV8Value::CreatePromise(); +auto promise = CefV8Value::CreatePromise(); // keeps ref alive for this scopeNot critical, yet improves clarity.
214-223: Nit: makeCreateResultObjectinlineand acceptconst String^Tiny style points:
- Marking the helper as
inlineavoids ODR issues when included in multiple translation units.- The managed string is not modified – pass it as
const String^to express intent.-static CefRefPtr<CefV8Value> CreateResultObject(int count, String^ message, bool isSuccess) +inline static CefRefPtr<CefV8Value> CreateResultObject(int count, const String^ message, bool isSuccess)Purely cosmetic – feel free to ignore if this file is included only once.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
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/BindObjectAsyncHandler.h(3 hunks)CefSharp.BrowserSubprocess.Core/CefAppUnmanagedWrapper.cpp(1 hunks)CefSharp.BrowserSubprocess.Core/CefAppUnmanagedWrapper.h(0 hunks)
💤 Files with no reviewable changes (1)
- CefSharp.BrowserSubprocess.Core/CefAppUnmanagedWrapper.h
🔇 Additional comments (9)
CefSharp.BrowserSubprocess.Core/Async/JavascriptAsyncMethodCallback.cpp (2)
16-16: Great simplification of promise resolution logic.Updating the code to check for
_promise.get()instead of_resolve.get()and using the promise's nativeResolvePromise()method aligns with the PR's goal of using native promise implementation.Also applies to: 20-20
31-31: Improved promise rejection handling.Similar to the success case, the code now uses the native
RejectPromise()method directly on the promise object rather than executing a separate reject function. This makes the code more concise and maintainable.Also applies to: 35-35
CefSharp.BrowserSubprocess.Core/CefAppUnmanagedWrapper.cpp (2)
659-660: Good refactoring with helper method.Using
BindObjectAsyncHandler::CreateResultObjectcentralizes the creation of result objects, improving code maintainability and consistency. This replaces manual construction of result objects that was previously done using script evaluation.
663-664: Consistent use of the helper method.The helper is also correctly used for the case when no objects are bound, ensuring consistent behavior and structure for the response objects regardless of binding success.
CefSharp.BrowserSubprocess.Core/Async/JavascriptAsyncMethodCallback.h (3)
19-19: Simplified member variables.Replacing separate
_resolveand_rejectmembers with a single_promisemember reduces state management complexity and better represents the unified nature of a promise object.
22-24: Updated constructor for clarity.The constructor now correctly accepts a single promise object rather than separate resolve and reject functions, reflecting the simplified internal state management.
31-31: Consistent cleanup in destructor.The destructor now resets the
_promisemember, maintaining proper cleanup of resources.CefSharp.BrowserSubprocess.Core/Async/JavascriptAsyncMethodHandler.cpp (2)
27-29: Excellent use of native promise API.Using
CefV8Value::CreatePromise()directly instead of evaluating a JavaScript script is more efficient and eliminates potential evaluation errors. This is the core of the PR's improvement - replacing script-based promise creation with the native API.
30-30: Updated callback creation.The callback is now correctly created with the promise object, matching the updated constructor signature in
JavascriptAsyncMethodCallback. This completes the refactoring of the promise handling flow.
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
CefSharp.BrowserSubprocess.Core/BindObjectAsyncHandler.h(3 hunks)CefSharp.Example/Resources/BindingTestAsync.js(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- CefSharp.BrowserSubprocess.Core/BindObjectAsyncHandler.h
|
✅ Build CefSharp 135.0.170-CI5231 completed (commit 38f787b242 by @amaitland) |
| @@ -228,23 +191,13 @@ namespace CefSharp | |||
| else | |||
| { | |||
| //Objects already bound or ignore cache | |||
| CefRefPtr<CefV8Value> promiseResolve; | |||
| CefRefPtr<CefV8Exception> promiseException; | |||
| auto promiseResolve = CefV8Value::CreatePromise(); | |||
There was a problem hiding this comment.
You create a promise and assign it to the result three times. You can create it earlier and assign it to retval, and resolve it later. It will work.
And we can replace exception = "BindObjectAsyncHandler::Execute - Browser wrapper null, unable to bind objects"; with promiseResolve->RejectPromise("BindObjectAsyncHandler::Execute - Browser wrapper null, unable to bind objects");.
Summary:
CefV8Value::CreatePromise()How Has This Been Tested?
Types of changes
Checklist:
Summary by CodeRabbit