Break CefHandler into separate Handler interfaces

[GoogleCodeExporter]

The CefHandler interface is getting too large. This is bad for a number of 
reasons:

1. More work for clients to implement unused callbacks.
2. When callbacks change all clients must make modifications even if they don't 
use the changed callback.
3. Performance may be negatively affected by executing unused callbacks.

For these reasons I propose breaking CefHandler into multiple separate handler 
interfaces which provide sets of related callbacks. A new CefClient interface 
will be introduced that supplies the handler implementations via Get*Handler() 
methods. The new proposed handler interfaces can be summarized as follows (the 
attached patch shows the full changes to cef.h):

CefLifeSpanHandler
  OnBeforeCreated
  OnAfterCreated
  OnBeforeClose

CefLoadHandler
  OnLoadStart
  OnLoadEnd
  OnLoadError

CefRequestHandler
  OnBeforeBrowse
  OnBeforeResourceLoad
  OnProtocolExecution
  OnDownloadResponse
  OnAuthenticationRequest

CefDisplayHandler
  OnNavStateChange
  OnAddressChange
  OnTitleChange
  OnTooltip
  OnStatusMessage
  OnConsoleMessage

CefFocusHandler
  OnTakeFocus
  OnSetFocus

CefKeyboardHandler
  OnKeyEvent

CefMenuHandler
  OnBeforeMenu
  GetMenuLabel
  OnMenuAction

CefPrintHandler
  GetPrintOptions
  GetPrintHeaderFooter

CefFindHandler
  OnFindResult

CefJSDialogHandler
  OnJSAlert
  OnJSConfirm
  OnJSPrompt

CefJSBindingHandler
  OnJSBinding

CefRenderHandler
  GetRect
  GetScreenPoint
  OnPopupChange
  OnPaintOnCursorChange

Comments on the proposed organization and naming are welcome.

Original issue reported on code.google.com by magreenb...@gmail.com on 16 Apr 2011 at 2:47

• Blocking: #220, #220

Attachments:

• cef_client_r218.patch

[GoogleCodeExporter]

For CefRenderHandler OnPaint and OnCursorChange are separate methods (typo 
above). Also, break OnPopupChange into separate OnPopupShow(bool show) and 
OnPopupSize methods.

Original comment by magreenb...@gmail.com on 16 Apr 2011 at 12:39

[GoogleCodeExporter]

It would be great it OnPopupShow() would also include the initial location of 
the popup rather than having to process both an OnPopupShow() *and* a 
OnPopupSize().

Original comment by dreijer...@gmail.com on 16 Apr 2011 at 5:09

[GoogleCodeExporter]

Original comment by magreenb...@gmail.com on 18 Apr 2011 at 2:12

[GoogleCodeExporter]

Original comment by magreenb...@gmail.com on 18 Apr 2011 at 2:18

[GoogleCodeExporter]

Theme support (issue #220) will introduce a CefThemeHandler interface and 
Notification support (issue #173) will introduce a CefNotificationHandler 
interface.

Original comment by magreenb...@gmail.com on 18 Apr 2011 at 2:21

[GoogleCodeExporter]

The attached patch includes the updated interface for CefRenderHandler, which 
now looks like this:

CefRenderHandler
  GetViewRect
  GetScreenRect
  GetScreenPoint
  OnPopupShow
  OnPopupSize
  OnPaint
  OnCursorChange

In response to comment #2:
The popup size/location information is not available at the time that the show 
notification is sent, so size/location will need to remain a separate 
notification.

Original comment by magreenb...@gmail.com on 18 Apr 2011 at 11:48

Attachments:

• cef_client_r219.patch

[GoogleCodeExporter]

this looks really useful, any idea when it will be committed?

Original comment by anthony....@gmail.com on 19 Apr 2011 at 6:30

[GoogleCodeExporter]

> this looks really useful, any idea when it will be committed?

I'm going to give people a few more days to provide feedback on the new API 
before I finalize it.

Original comment by magreenb...@gmail.com on 19 Apr 2011 at 6:36

[GoogleCodeExporter]

I'm considering providing empty virtual method implementations for handler API 
methods instead of using pure virtual methods. These are the important points:

1. Client applications would only need to implement the methods that are being 
used.

2. Compiler errors would no longer be generated by default in client 
applications when API method signatures change. In order to continue generating 
compiler errors client applications will need to use an OVERRIDE attribute as 
defined below. Note that OVERRIDE only works with MSVC and clang compilers.

// Annotate a virtual method indicating it must be overriding a virtual
// method in the parent class.
// Use like:
//   virtual void foo() OVERRIDE;
#if defined(COMPILER_MSVC)
#define OVERRIDE override
#elif defined(__clang__)
#define OVERRIDE override
#else
#define OVERRIDE
#endif

Original comment by magreenb...@gmail.com on 19 Apr 2011 at 7:00

[GoogleCodeExporter]

[deleted comment]

[GoogleCodeExporter]

What happens to CefHandler::HandleBeforeCreated & CefBrowser::CreateBrowser - 
specifically the handler parameter.
Can CreateBrowser replace all of these handlers?  (If it takes a struct of all 
the handlers that would work.)
[Edit:  I looked back at my code and I was setting the handler in 
CreateBrowser, not replacing it in HandleBeforeCreated.]

(Currently on the window I use for HTML notifications I use a subclass of 
CefHandler to replace these: 
    HandleBeforeCreated
    HandleAfterCreated;
    HandleBeforeWindowClose

    HandleBeforeBrowse - different rules for notifications.
    HandleLoadEnd - I load template then it handles each notification.  At LoadEnd, I can start loading the queued notifications.

    HandleNotification...  Notifications can not open other notifications, so these all return RV_HANDLED.

    So with the new api it looks like I would need to override:
      CefLifeSpanHandler
      CefLoadHandler
      CefRequestHandler
      CefNotificationHandler

Original comment by Fe3...@gmail.com on 26 Apr 2011 at 9:02

[GoogleCodeExporter]

> What happens to CefHandler::HandleBeforeCreated & CefBrowser::CreateBrowser - 
> specifically the handler parameter.
> Can CreateBrowser replace all of these handlers?  (If it takes a struct of 
all the 
> handlers that would work.)

I think that we can eliminate HandleBeforeCreated in the case where we use 
CreateBrowser and CreateBrowserSync. Currently, HandleBeforeCreated is also 
called on the current CefHandler instance for new popup windows. The |handler| 
parameter to HandleBeforeCreated allows the user to assign a different 
CefHandler for that popup window.

So instead of OnBeforeCreated we could have a single 
CefLifeSpanHandler::OnBeforePopup callback that is used just for popup windows:

virtual bool OnBeforePopup(CefRefPtr<CefBrowser> parentBrowser,
                           const CefString& url,
                           const CefPopupFeatures& popupFeatures,
                           CefWindowInfo& windowInfo,
                           CefRefPtr<CefClient>& client,
                           CefBrowserSettings& settings) =0;

Original comment by magreenb...@gmail.com on 11 May 2011 at 2:27

[GoogleCodeExporter]

Not sure I entirely follow your last comment.
  HandleBeforeCreated or CefLifeSpanHandler::OnBeforeCreated - I still need that in my app. I am framing the CefBrowser in a parent window, so that is where I create the parent, and set the CefBrowser as a child.  (This lets me frame everything even Developer Tools!)  
I don't need to override the handler in ...BeforeCreated.

  But, I currently I have 2 different handlers.  One for browsers, and popups, and a different one, (A child class with a few overrides.) for notifications.  (The notification display in my app is more specialized than the patch I sent.  In my app individual popup windows was not wanted.)

  My big question - can I replace all the handlers in the new CreateBrowser call?

Original comment by Fe3...@gmail.com on 12 May 2011 at 2:44

[GoogleCodeExporter]

> I am framing the CefBrowser in a parent window, so that is where I create the 
> parent, and set the CefBrowser as a child.

For non-popup windows CefBrowserSettings is the only parameter passed to 
HandleBeforeCreated that isn't simply forwarded from the CreateBrowser call. 
You can set CefWindowInfo.m_hWndParent before calling CreateBrowser, for 
example. We should probably remove the CefBrowserSettings option from 
CefInitialize and pass it to CreateBrowser instead. Popup windows would then 
start with the same CefBrowserSettings as the parent window.

> My big question - can I replace all the handlers in the new CreateBrowser 
call?

All handlers will be retrieved via Get*Handler() methods of the new CefClient 
class. So you would implement CefClient to provide whatever handlers you need 
and then pass the CefClient instance to CreateBrowser. Does that answer your 
question?

Original comment by magreenb...@gmail.com on 17 May 2011 at 2:56

[GoogleCodeExporter]

The attached patch includes the changes discussed in comments #12 and #14. I've 
also removed the |popup| parameter from CreateBrowser so that IsPopup() will 
now only be true for browser windows created by other browser windows (for 
which OnBeforePopup will be called).

Original comment by magreenb...@gmail.com on 17 May 2011 at 4:06

Attachments:

• cef_client_r233.patch

[GoogleCodeExporter]

Committed as revision 235.

- Replace CefHandler with a new CefClient interface and separate handler 
interfaces.
- Add support for virtual inheritance to allow multiple CefBase parented 
interfaces to be implemented in the same class.
- Replace CefThreadSafeBase with IMPLEMENT_* macros to support virtual 
inheritance and to only provide locking implementations when needed.
- Move the CefBrowserSettings parameter from CefInitialize to CreateBrowser.
- Add a new cef_build.h header that provides platform-specific and OS_* defines.
- Introduce the use of OVERRIDE to generate compiler errors on Windows if a 
child virtual method declaration doesn't match the parent declaration.
- Use NDEBUG instead of _DEBUG because _DEBUG is not defined on Mac. (from 
issue #240).

Original comment by magreenb...@gmail.com on 20 May 2011 at 2:45

• Changed state: Fixed

[GoogleCodeExporter]

Original comment by magreenb...@gmail.com on 20 May 2011 at 3:17

[GoogleCodeExporter]

[deleted comment]

[GoogleCodeExporter]

We are working on updating to this patch.  The removal of the old 
HandleBeforeCreated is less of an issue than I thought, because even Developer 
Tools is treated as a popup.  (I thought it wasn't a popup.)

So we just need to change how we handle the initial CreateBrowser.  (Yes, that 
is basically what you said in Comment 14.)

Original comment by Fe3...@gmail.com on 27 May 2011 at 12:56

[GoogleCodeExporter]

Issue 249 has been merged into this issue.

Original comment by magreenb...@gmail.com on 27 May 2011 at 1:07

[GoogleCodeExporter]

Original comment by magreenb...@gmail.com on 11 Oct 2013 at 1:51

• Now blocking: #220, #220