Skip to content

[NUI] Add WebView APIs for video playback notification.#7721

Open
huayongxu wants to merge 1 commit into
Samsung:DevelNUIfrom
huayongxu:da-video-webview-1
Open

[NUI] Add WebView APIs for video playback notification.#7721
huayongxu wants to merge 1 commit into
Samsung:DevelNUIfrom
huayongxu:da-video-webview-1

Conversation

@huayongxu

@huayongxu huayongxu commented Jun 26, 2026

Copy link
Copy Markdown
Contributor

Description of Change

the related patch at binder side:
https://review.tizen.org/gerrit/c/platform/core/uifw/dali-csharp-binder/+/346532

API Changes

  • ACR:

@gemini-code-assist gemini-code-assist Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Code Review

This pull request introduces support for playback video events in WebView by adding P/Invoke callback registrations and exposing corresponding C# events (Ready, Started, Finished, Stopped, and Paused). The review feedback suggests using EventArgs.Empty instead of instantiating new EventArgs() when raising these events to avoid unnecessary allocations and reduce garbage collection pressure.

Important

The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.

Comment on lines +3525 to +3548
private void OnPlaybackVideoReady()
{
playbackVideoReadyEventHandler?.Invoke(this, new EventArgs());
}

private void OnPlaybackVideoStarted()
{
playbackVideoStartedEventHandler?.Invoke(this, new EventArgs());
}

private void OnPlaybackVideoFinished()
{
playbackVideoFinishedEventHandler?.Invoke(this, new EventArgs());
}

private void OnPlaybackVideoStopped()
{
playbackVideoStoppedEventHandler?.Invoke(this, new EventArgs());
}

private void OnPlaybackVideoPaused()
{
playbackVideoPausedEventHandler?.Invoke(this, new EventArgs());
}

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

medium

Using new EventArgs() allocates a new instance of EventArgs every time the event is raised, which increases garbage collection pressure. Since EventArgs does not carry any event-specific data, it is highly recommended to use the cached EventArgs.Empty instead.

        private void OnPlaybackVideoReady()
        {
            playbackVideoReadyEventHandler?.Invoke(this, EventArgs.Empty);
        }

        private void OnPlaybackVideoStarted()
        {
            playbackVideoStartedEventHandler?.Invoke(this, EventArgs.Empty);
        }

        private void OnPlaybackVideoFinished()
        {
            playbackVideoFinishedEventHandler?.Invoke(this, EventArgs.Empty);
        }

        private void OnPlaybackVideoStopped()
        {
            playbackVideoStoppedEventHandler?.Invoke(this, EventArgs.Empty);
        }

        private void OnPlaybackVideoPaused()
        {
            playbackVideoPausedEventHandler?.Invoke(this, EventArgs.Empty);
        }

@TizenAPI-Bot

Copy link
Copy Markdown
Collaborator

Internal API Changed

Added: 5, Removed: 0, Changed: 0

Added

+ /// <since_tizen>none</since_tizen
+ [EditorBrowsable(EditorBrowsableState.Never)]
+ System.EventHandler Tizen.NUI.BaseComponents.WebView::PlaybackVideoFinished

+ /// <since_tizen>none</since_tizen
+ [EditorBrowsable(EditorBrowsableState.Never)]
+ System.EventHandler Tizen.NUI.BaseComponents.WebView::PlaybackVideoPaused

+ /// <since_tizen>none</since_tizen
+ [EditorBrowsable(EditorBrowsableState.Never)]
+ System.EventHandler Tizen.NUI.BaseComponents.WebView::PlaybackVideoReady

+ /// <since_tizen>none</since_tizen
+ [EditorBrowsable(EditorBrowsableState.Never)]
+ System.EventHandler Tizen.NUI.BaseComponents.WebView::PlaybackVideoStarted

+ /// <since_tizen>none</since_tizen
+ [EditorBrowsable(EditorBrowsableState.Never)]
+ System.EventHandler Tizen.NUI.BaseComponents.WebView::PlaybackVideoStopped

@huayongxu huayongxu force-pushed the da-video-webview-1 branch from a9b4d66 to 81c4e97 Compare June 26, 2026 02:49
@huayongxu huayongxu changed the title Add WebView APIs for video playback notification. [NUI] Add WebView APIs for video playback notification. Jun 26, 2026
@TizenAPI-Bot

Copy link
Copy Markdown
Collaborator

Internal API Changed

Added: 5, Removed: 0, Changed: 0

Added

+ /// <since_tizen>none</since_tizen
+ [EditorBrowsable(EditorBrowsableState.Never)]
+ System.EventHandler Tizen.NUI.BaseComponents.WebView::PlaybackVideoFinished

+ /// <since_tizen>none</since_tizen
+ [EditorBrowsable(EditorBrowsableState.Never)]
+ System.EventHandler Tizen.NUI.BaseComponents.WebView::PlaybackVideoPaused

+ /// <since_tizen>none</since_tizen
+ [EditorBrowsable(EditorBrowsableState.Never)]
+ System.EventHandler Tizen.NUI.BaseComponents.WebView::PlaybackVideoReady

+ /// <since_tizen>none</since_tizen
+ [EditorBrowsable(EditorBrowsableState.Never)]
+ System.EventHandler Tizen.NUI.BaseComponents.WebView::PlaybackVideoStarted

+ /// <since_tizen>none</since_tizen
+ [EditorBrowsable(EditorBrowsableState.Never)]
+ System.EventHandler Tizen.NUI.BaseComponents.WebView::PlaybackVideoStopped

@huayongxu huayongxu force-pushed the da-video-webview-1 branch from 81c4e97 to 1fd5084 Compare June 26, 2026 05:22
Original-Author: yehyeon.kim@samsung.com
Co-Authored-By: Cline SR <GaussO3.4>
@TizenAPI-Bot

Copy link
Copy Markdown
Collaborator

Internal API Changed

Added: 5, Removed: 0, Changed: 0

Added

+ /// <since_tizen>none</since_tizen
+ [EditorBrowsable(EditorBrowsableState.Never)]
+ System.EventHandler Tizen.NUI.BaseComponents.WebView::PlaybackVideoFinished

+ /// <since_tizen>none</since_tizen
+ [EditorBrowsable(EditorBrowsableState.Never)]
+ System.EventHandler Tizen.NUI.BaseComponents.WebView::PlaybackVideoPaused

+ /// <since_tizen>none</since_tizen
+ [EditorBrowsable(EditorBrowsableState.Never)]
+ System.EventHandler Tizen.NUI.BaseComponents.WebView::PlaybackVideoReady

+ /// <since_tizen>none</since_tizen
+ [EditorBrowsable(EditorBrowsableState.Never)]
+ System.EventHandler Tizen.NUI.BaseComponents.WebView::PlaybackVideoStarted

+ /// <since_tizen>none</since_tizen
+ [EditorBrowsable(EditorBrowsableState.Never)]
+ System.EventHandler Tizen.NUI.BaseComponents.WebView::PlaybackVideoStopped

@JoonghyunCho

Copy link
Copy Markdown
Member

🤖 [AI Review]

Reviewed — no findings.

Scope checked:

  • 5 new playback events (PlaybackVideoReady/Started/Finished/Stopped/Paused) are all [EditorBrowsable(EditorBrowsableState.Never)], so they are exempt from the public-API doc requirement; each still carries a <summary>.
  • Delegate lifetime: each callback is stored in a private field before Marshal.GetFunctionPointerForDelegate, preventing GC of the delegate that native code invokes.
  • add/remove symmetry: native Register*Callback is called with the function pointer on first subscribe and IntPtr.Zero on last unsubscribe, gated by the null-handler check — no double-register or dangling registration.
  • new EventArgs() in the 5 new raisers is consistent with the 6 existing event raisers in this same file (convention-consistent; not flagged).
  • Interop signatures (CSharp_Dali_WebView_RegisterPlaybackVideo*Callback, (HandleRef, IntPtr)) match the sibling WebView callback bindings.

No 🔴 critical issues, no 🟡 suggestions to flag.


Automated review — final merge decision rests with human reviewers.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants