Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: expose onJsdialog #37

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

TrafalgarSX
Copy link

@tishion
Copy link
Member

tishion commented Aug 22, 2024

hi @TrafalgarSX , thanks for your contribution, this change looks good, but I have some concerns.

1.You didn't implement all methods in CefJSDialogHandler

Please look into this definition of CefJSDialogHandler
https://github.com/chromiumembedded/cef/blob/6045/include/cef_jsdialog_handler.h.

There are 4 methods in this interface, you have implemented 2 (actually 1 because you just return false in 'CefJSDialogHandler::OnBeforeUnloadDialog'). I think this is not correct implementation, at least this is not complement implementation.

The following methods are designed to be called by the CEF initiated from inner context or JavaScript context.
OnBeforeUnloadDialog
OnResetDialogState
OnDialogClosed

you only implemented 'OnJSDialog', looks pretty cool and it will display dialog with your implementation, but what will happen if CEF calls OnResetDialogState to close all dialogs? there's is empty implementation right?

2.CefJSDialogHandler currently is only useful for linux, so I think we'd better keep the default implementation for other platforms.

please refer to the CEF test implementation:
https://github.com/chromiumembedded/cef/blob/6045/tests/cefclient/browser/dialog_handler_gtk.h

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