[video_player] Add optional web options

[defuncart]

• Fixes video_player protect video from download  flutter#62192
• Fixes first part of [Video_player][Web]: Add option to show video player controls and make double click go fullscreen when controls are invisible flutter#88151 (ability to show controls)
• Fixes [video_player] Add Fullscreen for Web flutter#64397

Can be used as follows:

_controller = VideoPlayerController.asset(
  'assets/Butterfly-209.mp4',
  videoPlayerOptions: const VideoPlayerOptions(
    webOptions: VideoPlayerWebOptions(
        controls: VideoPlayerWebOptionsControls.enabled(
          allowDownload: false,
          allowFullscreen: false,
          allowPlaybackRate: false,
          allowPictureInPicture: false,
        ),
        allowContextMenu: false,
        allowRemotePlayback: false,
     ),
  ),
);

Pre-launch Checklist

• I read the Contributor Guide and followed the process outlined there for submitting PRs.
• I read the Tree Hygiene wiki page, which explains my responsibilities.
• I read and followed the relevant style guides and ran the auto-formatter. (Unlike the flutter/flutter repo, the flutter/packages repo does use dart format.)
• I signed the CLA.
• The title of the PR starts with the name of the package surrounded by square brackets, e.g. [shared_preferences]
• I listed at least one issue that this PR fixes in the description above.
• I updated pubspec.yaml with an appropriate new version according to the pub versioning philosophy, or this PR is exempt from version changes.
• I updated CHANGELOG.md to add a description of the change, following repository CHANGELOG style.
• I updated/added relevant documentation (doc comments with ///).
• I added new tests to check the change I am making, or this PR is test-exempt.
• All existing and new tests are passing.

If you need help, consider asking for advice on the #hackers-new channel on Discord.

[flutter-dashboard]

It looks like this pull request may not have tests. Please make sure to add tests before merging. If you need an exemption to this rule, contact Hixie on the #hackers channel in Chat (don't just cc him here, he won't see it! He's on Discord!).

If you are not sure if you need tests, consider this rule of thumb: the purpose of a test is to make sure someone doesn't accidentally revert the fix. Ask yourself, is there anything in your PR that you feel it is important we not accidentally revert back to how it was before your fix?

Reviewers: Read the Tree Hygiene page and make sure this patch meets those guidelines before LGTMing.

[defuncart]

@ditman @stuartmorgan The original PR was opened 3 months ago, would be nice to get some feedback on the api. I think this is a feature that can benefit flutter web users.

[stuartmorgan]

Bringing over my API note from the other PR for convenience (/cc @ditman)

I think the main question is whether we want to expose things through an options mechanism like this, or do something more direct like exposing the element for direct manipulation. How many more things like this might we want to add later? If we end up duplicating all of a large API surface that would be problematic, but if it's a fairly small set of options this (or the alternate pattern we have in some other plugins of using platform-specific option subclasses) could be reasonable.

[stuartmorgan]

@ditman Do you have an opinion on the API question above?

[stuartmorgan]

Update from triage: waiting for @ditman's input on the high-level API question.

[ditman]

I don't have a strong opinion on the API, and small nitpicks in the PR in general.

I wonder if the new configuration object warrants being added to the videoPlayer configuration, and instead expose the setWebOptions(WebOptions options) method that users can call if needed!

(We've used other web strategies, but their usage is cumbersome to programmers, and I wouldn't recommend them until we flesh them out more!)

[defuncart]

@ditman @stuartmorgan Thanks for the review.

As more options may come in the future (i.e. mute, autoplay), I'd split the control options out to their own model VideoPlayerWebOptionsControls for a clearer api with named constructors.

I agree that setWebOptions(options) should suffice for web users and it doesn't need to be a property of VideoPlayerOptions. However, the benefit of being part of VideoPlayerOptions is that when initialize is called, the optional web options are initialized. If setWebOptions(options) is a method on VideoPlayerController, then initialize will need to be invoked before setWebOptions.

[stuartmorgan]

@ditman Do you want to weigh in again on the structure we want for exposing <video> attributes?

[ditman]

@defuncart I've been sold of the following: VideoPlayerWebOptions is a stand-alone object, AND that it is contained within the VideoPlayerOptions; that sounds good to me!

@ditman Do you want to weigh in again on the structure we want for exposing

The main question is whether we want to expose things through an options mechanism like this, or do something more direct like exposing the element for direct manipulation.

In the video_player case, and because of how it uses a Controller object as a source of truth, I think @defuncart's solution is better than directly exposing the video element to users.

Exposing the element directly would enable calls to play/pause/seek/change the volume and the controller state would lose sync with the actual video playback element (at least until the controller fully reacts to events coming from the native video player object!).

---

@defuncart I think there's at least a couple of extra attributes that might be nice to have in this new configuration:

• disablepictureinpicture : Prevents the browser from suggesting a Picture-in-Picture context menu or to request Picture-in-Picture automatically in some cases.
• disableremoteplayback : used to disable the capability of remote playback in devices that are attached using wired (HDMI, DVI, etc.) and wireless technologies (Miracast, Chromecast, DLNA, AirPlay, etc.).

They seem very related to "where you can display this video" (especially with the disabling of the contextMenu).

Could you add them as well? That could also demonstrate the extensibility of the solution :)

[defuncart]

@ditman Added allowPictureInPicture and allowRemotePlayback options. As picture-in-picture is displayed in controls list, I've added the option to VideoPlayerWebOptionsControls. If you are happy with the API, then I can add tests etc.

[ditman]

If you are happy with the API, then I can add tests etc.

I'm happy with the API @defuncart, thanks! The only unaddressed concern is using dynamic as a return value of the _onContextMenu function definition, which should be more precise (at the very least it should be: Object?)

[stuartmorgan]

Update from triage: the blocking PR is in the queue to land now.

[flutter-dashboard]

It looks like this pull request may not have tests. Please make sure to add tests before merging. If you need an exemption to this rule, contact Hixie or stuartmorgan on the #hackers channel in Chat (don't just cc them here, they won't see it! Use Discord!).

If you are not sure if you need tests, consider this rule of thumb: the purpose of a test is to make sure someone doesn't accidentally revert the fix. Ask yourself, is there anything in your PR that you feel it is important we not accidentally revert back to how it was before your fix?

Reviewers: Read the Tree Hygiene page and make sure this patch meets those guidelines before LGTMing.

[ditman]

Only the changes to the core package are remaining now, right?

[stuartmorgan]

As the bot has highlighted, now that this has been reduced to just the app-facing package it's surfaced that this is missing tests. We should have a test of the options being correctly passed through this layer.

[stuartmorgan]

@defuncart Are you still planning on updating this to add tests, per the comment above?

[defuncart]

@stuartmorgan @ditman What type of tests are expected here? How can kIsWeb be mocked?

[stuartmorgan]

What type of tests are expected here?

See my earlier comment. To put it another way: a test that would fail if the code you are adding in this PR were accidentally removed or broken by someone later.

How can kIsWeb be mocked?

You shouldn't need to mock it; we run Dart unit tests in both web and vm mode, so a test that is skipped on non-web will still run in CI.

[defuncart]

What type of tests are expected here?

See my earlier comment. To put it another way: a test that would fail if the code you are adding in this PR were accidentally removed or broken by someone later.

Thanks for the info, due to holidays, I won't be able to look into this for the next month. @wreppun if you'd like, feel free to take over adding the 🧪.

[hellohuanlin]

I can only see 1 file change in this PR. Is this ready for review?

[stuartmorgan]

Converting to a Draft; please feel free to mark this as ready for review once the tests have been added.

[Thioby]

any chance to merge this? This literally makes this lib unusable.

[stuartmorgan]

@Thioby We cannot merge an uncompleted draft PR; anyone interested in accelerating the process of getting to a landable PR is welcome to contribute tests as discussed above.

[goderbauer]

(triage): @defuncart Do you still have plans to come back to this PR and address the feedback given above?

[goderbauer]

(triage): I am going to close this one since there hasn't been any follow-up. If you find the time to address the feedback given above please reopen this. Thank you.

[ditman]

Assigning to myself to land this. Needs tests.