feat: Add support for chromecast by judicaelandria · Pull Request #3 · silvermine/videojs-remoteplayback

judicaelandria · 2025-10-31T10:54:16Z

No description provided.

kmuncie

@judicaelandria here is a very surface level review to consider before @joshuacurtiss is back and able to give this a thorough review.

kmuncie · 2026-01-28T14:42:09Z

src/js/chromecast/ChromecastManager.ts

+    * (e.g. the Chromecast button) can respond to connect / connecting /
+    * disconnect transitions.
+    */
+   private _setupEventListeners(): void {


Do we need to cleanup these listeners at some point? Should we add a dispose() method that removes these listeners, similar to how ChromecastButton handles cleanup at ChromecastButton.ts:287-291, or is that handled somewhere else?

Looks like you aimed to solve this one, I will leave this open until @joshuacurtiss has a look

kmuncie · 2026-01-28T14:50:27Z

src/js/chromecast/ChromecastButton.ts

+   private _setupDirectAPIListeners(): void {
+      const videoElement = getVideoElement(this._player);
+
+      if (!videoElement || !checkClientChromecastSupport()) {


Would it be better to handle the !checkClientChromecastSupport down on L285 instead of here?

kmuncie · 2026-01-28T14:51:30Z

src/js/chromecast/ChromecastManager.ts

+      }
+
+      if (!this._isSetup) {
+         throw new Error('ChromecastManager not properly initialized. Call setup first.');


Issue: Error message says "Call setup first" but there's no public setup() method - initialization happens automatically in the constructor. Suggestion: Revise to: ChromecastManager initialization failed - video element with Remote Playback support not found.

kmuncie · 2026-01-28T14:54:42Z

src/styles/chromecast.scss

+
+/* Ensure button is visible when Chromecast is available */
+.vjs-chromecast-button:not(.vjs-hidden) {
+   display: block;


Is block correct here? I see alot of out rules for buttons above setting flex?

I see you removed this block entirely, was it actually not needed at all?

no, we didn't need it

kmuncie · 2026-01-28T14:56:20Z

src/js/constants/log-messages.ts

+   CHROMECAST_NOT_SUPPORTED: 'Chromecast not supported on this device',
+   CHROMECAST_SUPPORTED: 'Chromecast support detected',
+   GOOGLE_CAST_SUPPORTED: 'Google Cast API supported - full device compatibility',
+   GOOGLE_CAST_PREFERRED: 'Using Google Cast API for full device list',


These two GOOGLE_CAST... constants are apparently never used, is that intentional?

I see they were deleted.

kmuncie · 2026-01-28T14:57:37Z

src/styles/chromecast.scss

@@ -0,0 +1,49 @@
+$icon-chromecast--default: '../assets/ic_cast_white_24px.svg' !default;
+$icon-chromecast--hover: '../assets/ic_cast_connected_white_24dp.png' !default;


Whats up with the mixed file types? SVG for normal button and then hover switches to PNG? Do we need better assets for hover?

I dont see any changes here but I assume it will be considered further

fixed @kmuncie

joshuacurtiss · 2026-02-26T23:38:43Z

Hi @judicaelandria, just posting this for reference; today IRL we discussed:

Making the logic and form of the Button and Manager classes for AirPlay and Chromecast consistent with each other.
Is it possible to improve the chromecast implementation to use feature detection instead of browser detection?
Can the main RemotePlayback class track which method (for now, either AirPlay or Chromecast) is active, so that it just calls the function from that active manager? In our discussion, we were calling this a proxy pattern, but it actually is better described by the Strategy Pattern. See here: Strategy pattern example in TypeScript.

joshuacurtiss · 2026-03-13T22:59:41Z

@judicaelandria, thanks for your work. I tested with Android and Google TV and it works! Here are some questions that I'm not clear on when reviewing this code. Please note first I'm hoping you can just answer these to help me understand. Then we could decide which questions merit some code changes. Thank you!!

I noticed that the RemotePlaybackPlugin has a dispose method that calls this._chromecastManager.dispose(); but it doesn't make a similar call for AirPlayManager. Is this because the AirPlayManager class doesn't need one? Is its disposal approach different? Why? Which way is preferred?

I see _getActiveManager returns the manager that it detects is connected. (This is different than my Strategy Pattern suggestion, but it is light since isConnected isn't doing anything heavy.) Then it also returns one even if it is not connected. Why? Is this used in any situation?

When ChromecastManager sets this._remotePlayback = null in its dispose method, is this successfully disposing of the event handlers?

In ChromecastButton, is there a reason why you hardcoded the vjs-chromecast-casting-state instead of using the convention to store the classes in CSS_CLASSES?

The ic_cast_white_connected.svg is coded to be much larger than the other icons. Why?

When I run the tests, they fail. Should the Chromecast tests be failing? I also noticed that the tests set up a user agent string. Is that still relevant?

judicaelandria · 2026-03-20T16:10:49Z

In ChromecastButton, is there a reason why you hardcoded the vjs-chromecast-casting-state instead of using the convention to store the classes in CSS_CLASSES?
no reason, my bad. I moved it to the css classes
for the test, its fixed now
for _getActiveManager, I just reimplemented the RemotePlaybackPlugin using Strategy pattern like suggested so we should have that issue anymore
Airplay and Chromecast should have their dispose method now since they implements Strategy interfaces, dispose method have been added

- Implement core Chromecast functionality for the plugin

The AirPlay getVideoElement function returns an HTMLVideoElementWithAirPlay type, while Chromecast's getVideoElement returns a standard HTMLVideoElement. This type mismatch could cause runtime errors when the functions are used interchangeably.

judicaelandria marked this pull request as draft November 10, 2025 15:41

judicaelandria force-pushed the mandriamahandry/add-chromecast-support branch 4 times, most recently from dd83b05 to fe3ba58 Compare November 11, 2025 05:40

judicaelandria changed the title ~~feat: add chromecast support~~ feat: Implements Chromecast functionality for Video.js with remoteplayback API Nov 11, 2025

judicaelandria force-pushed the mandriamahandry/add-chromecast-support branch from fe3ba58 to f92a3dc Compare November 11, 2025 07:06

judicaelandria closed this Jan 20, 2026

judicaelandria reopened this Jan 20, 2026

judicaelandria force-pushed the mandriamahandry/add-chromecast-support branch 6 times, most recently from a91ed87 to 9bebbd8 Compare January 20, 2026 16:40

judicaelandria changed the title ~~feat: Implements Chromecast functionality for Video.js with remoteplayback API~~ feat: Add support for chromecast Jan 20, 2026

judicaelandria force-pushed the mandriamahandry/add-chromecast-support branch 6 times, most recently from fc8d9e3 to 7f2c9ca Compare January 27, 2026 07:27

judicaelandria marked this pull request as ready for review January 27, 2026 07:29

kmuncie reviewed Jan 28, 2026

View reviewed changes

judicaelandria force-pushed the mandriamahandry/add-chromecast-support branch 2 times, most recently from 6b3def0 to e45da59 Compare February 2, 2026 15:33

judicaelandria requested a review from kmuncie February 2, 2026 15:33

judicaelandria force-pushed the mandriamahandry/add-chromecast-support branch 2 times, most recently from b04ed24 to 324c77f Compare March 11, 2026 13:50

judicaelandria added 3 commits March 20, 2026 19:31

feat: add support for Chromecast

15710fc

- Implement core Chromecast functionality for the plugin

refactor: change videojs test id to remoteplayback-test-player

d9436f4

judicaelandria force-pushed the mandriamahandry/add-chromecast-support branch from 324c77f to 4887697 Compare March 20, 2026 16:32

		@@ -0,0 +1,49 @@
		$icon-chromecast--default: '../assets/ic_cast_white_24px.svg' !default;
		$icon-chromecast--hover: '../assets/ic_cast_connected_white_24dp.png' !default;

Conversation

judicaelandria commented Oct 31, 2025

Uh oh!

kmuncie left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

joshuacurtiss commented Feb 26, 2026

Uh oh!

joshuacurtiss commented Mar 13, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

judicaelandria commented Mar 20, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

joshuacurtiss commented Mar 13, 2026 •

edited

Loading