Skip to content

VPLAY-11310: Remove legacy AAMP JS bindings - #593

Open
Vinish100 wants to merge 1 commit into
dev_sprint_25_2from
feature/VPLAY-11310
Open

VPLAY-11310: Remove legacy AAMP JS bindings#593
Vinish100 wants to merge 1 commit into
dev_sprint_25_2from
feature/VPLAY-11310

Conversation

@Vinish100

Copy link
Copy Markdown
Contributor

Reason for change: Remove un-used AAMP legacy JS bindings
Test Procedure: Make sure all apps are working as expected
Risks: None
Priority: P2

@Vinish100
Vinish100 requested a review from a team October 17, 2025 12:32
Copilot AI review requested due to automatic review settings November 10, 2025 14:54
@Vinish100
Vinish100 requested a review from a team as a code owner November 10, 2025 14:54

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Pull Request Overview

This PR refactors the JavaScript bindings codebase by removing deprecated AAMP_JSController bindings and consolidating functionality into the AAMPMediaPlayer bindings. Key changes include deletion of ~4800 lines of legacy code, removal of dynamic library loading mechanism, and integration of event creation logic.

  • Removed deprecated jsbindings.cpp (AAMP_JSController implementation)
  • Consolidated event creation into jseventlistener.cpp
  • Removed dynamic loading mechanism (dlopen/dlclose) from PlayerInstanceAAMP
  • Added global AAMP JS object for version querying

Reviewed Changes

Copilot reviewed 14 out of 15 changed files in this pull request and generated 2 comments.

Show a summary per file
File Description
jsbindings/jsbindings.cpp Deleted entire file containing legacy AAMP_JSController implementation
jsbindings/jsevent.cpp/h Deleted files; functionality moved to jseventlistener.cpp
jsbindings/jsbindings-version.h Deleted; version macro moved to jsmediaplayer.cpp
main_aamp.cpp Removed dlopen/dlclose logic and LoadJS/UnloadJS implementations; formatting fixes
jsbindings/jsmediaplayer.cpp Added global AAMP object, AAMP_getProperty_Version, and event creation
jsbindings/jseventlistener.cpp Integrated createNewAAMPJSEvent function from deleted jsevent.cpp
test/utests/tests/JsBindingTests/ Updated to use AAMPPlayer_LoadJS/UnloadJS instead of aamp_LoadJS/UnloadJS

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread jsbindings/jseventlistener.cpp
Comment thread CMakeLists.txt Outdated
@Vinish100
Vinish100 force-pushed the feature/VPLAY-11310 branch 3 times, most recently from 2a78115 to f024d14 Compare July 29, 2026 17:08
@Vinish100
Vinish100 requested a review from Copilot July 29, 2026 19:09
@Vinish100
Vinish100 force-pushed the feature/VPLAY-11310 branch from f024d14 to 296e975 Compare July 29, 2026 19:11

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 15 out of 16 changed files in this pull request and generated 2 comments.

Comments suppressed due to low confidence (1)

test/utests/fakes/FakeJSObjectRef.cpp:247

  • FakeJSObjectRef uses NULL to represent the JavaScript 'undefined' value (JSValueMakeUndefined returns NULL). JSValueIsUndefined should be consistent with that; returning false unconditionally can cause tests to exercise cleanup paths incorrectly when code checks for undefined.
bool JSValueIsUndefined(JSContextRef ctx, JSValueRef value)
{
	return false;
}

Comment thread jsbindings/jsmediaplayer.cpp
Comment thread jsbindings/jsmediaplayer.cpp
@Vinish100
Vinish100 force-pushed the feature/VPLAY-11310 branch from 296e975 to 414a53c Compare August 4, 2026 10:51
Reason for change: Remove un-used AAMP legacy JS bindings. Keep
AAMP.version JSObject for backward compatibility and for apps to
query AAMP version without creating a UVE instance
Test Procedure: Make sure all apps are working as expected
Risks: None
Priority: P2

Signed-off-by: Vinish100 <vinish.balan@gmail.com>

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 15 out of 16 changed files in this pull request and generated 1 comment.

Suppressed comments (2)

main_aamp.cpp:1690

  • PlayerInstanceAAMP::LoadJS/UnloadJS implementations were removed from main_aamp.cpp, but the methods are still declared in main_aamp.h. Any caller that still references these APIs will now hit an undefined reference at link time. Either remove the declarations (and update call sites) or provide stub implementations consistent with the new JS bindings entry points.
/**
 *  @brief Support multiple listeners for multiple event type
 */
void PlayerInstanceAAMP::AddEventListener(AAMPEventType eventType, std::shared_ptr<EventListener> eventListener)

test/utests/tests/JsBindingTests/JsBindingTests.cpp:61

  • TestJsBindings now passes a NULL context into AAMPPlayer_LoadJS/AAMPPlayer_UnloadJS. Those functions explicitly early-return on NULL, so this test no longer exercises any JS binding load/unload logic (it effectively becomes a no-op and won't catch regressions).
TEST_F(JsBindingTests, TestJsBindings)
{
	void *context = NULL;
	AAMPPlayer_LoadJS(context);

	AAMPPlayer_UnloadJS(context);
}

Comment on lines +4436 to +4441
void __attribute__ ((destructor(101))) _aamp_term()
{
LOG_TRACE("Enter");
//Clear any active js mediaplayer instances on term
ClearAAMPPlayerInstances();
}
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.

2 participants