CefViewCore icon indicating copy to clipboard operation
CefViewCore copied to clipboard

feat: expose onJsdialog

Open TrafalgarSX opened this issue 1 year ago • 1 comments

https://github.com/CefView/QCefView/issues/191

TrafalgarSX avatar Aug 18 '24 13:08 TrafalgarSX

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

tishion avatar Aug 22 '24 06:08 tishion