search

CefView / CefViewCore

A common library providing clean and easy consuming of CEF
MIT License
99 stars 52 forks source link

feat: expose onJsdialog #37

Open TrafalgarSX opened 2 months ago

TrafalgarSX commented 2 months ago

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

tishion commented 2 months ago

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