RDKEMW-13657: Plugin Feature fails on Xione-UK#124
Conversation
There was a problem hiding this comment.
Pull request overview
This pull request addresses a plugin compatibility issue for Xione-UK by modifying how the JavaScriptCore library is loaded and how module paths are resolved. The changes introduce environment-based conditional logic for library loading and add fallback handling for root directory execution.
Changes:
- Modified JavaScriptCore library loading to be conditional based on the ETHAN_LOGGING_PIPE environment variable
- Fixed the logging fallback mechanism in NativeJSLogger to properly handle cases when Ethan logging is compiled but not enabled at runtime
- Added special handling for module path resolution when running from the root directory
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 10 comments.
| File | Description |
|---|---|
| src/jsc/JavaScriptContext.cpp | Conditionally loads/unloads libJavaScriptCore.so based on ETHAN_LOGGING_PIPE environment variable |
| src/NativeJSLogger.cpp | Fixed control flow to properly fall back to printf logging when USE_ETHANLOG is defined but not enabled |
| src/JavaScriptContextBase.cpp | Added hardcoded fallback path for module resolution when PWD is "/" and removed early return for better logging |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
293166c to
c71b5da
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 4 out of 4 changed files in this pull request and generated 3 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
c71b5da to
c367b1d
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 4 out of 4 changed files in this pull request and generated 3 comments.
Comments suppressed due to low confidence (2)
src/NativeJSRenderer.cpp:368
- This change stops injecting
window.location.hrefwhenenableJSDOMis enabled (it now only happens forenableMiniJSDOM). If full JSDOM apps rely onwindow.locationbeing set to the app URL (previous behavior), they will now see an empty/default location. Consider usingenableJSDOM || enableMiniJSDOMhere, or otherwise setting location consistently for both modes.
if(context->getModuleSettings().enableMiniJSDOM)
{
std::stringstream window;
window<<"window.location = {\"href\":\"" << url << "\"};";
NativeJSLogger::log(INFO, "Adding the window location: %s to js file\n", window.str().c_str());
context->runScript(window.str().c_str(),true, url, nullptr, true);
}
src/NativeJSRenderer.cpp:387
- Same as above for local apps:
window.locationis now only set forenableMiniJSDOM, notenableJSDOM, which can regress behavior for JSDOM mode. Consider setting location for both DOM modes or documenting/handling the difference explicitly.
if(context->getModuleSettings().enableMiniJSDOM)
{
std::stringstream window;
window<<"window.location = {\"href\":\"file:/" << url << "\"};";
NativeJSLogger::log(INFO, "Adding the window location: %s to js file\n", window.str().c_str());
context->runScript(window.str().c_str(),true, url, nullptr, true);
}
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Reason for change: Resolving the plugin issue Test Procedure: build should be successful Risk: low Priority: P2
c367b1d to
c28551f
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 4 out of 4 changed files in this pull request and generated 6 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Reason for change: Resolving the plugin issue
Test Procedure: build should be successful
Risk: low
Priority: P2