search

jramosd / javachromiumembedded

Automatically exported from code.google.com/p/javachromiumembedded
0 stars 0 forks source link

Change CefHandler to act like CefClient from CEF. #47

Closed GoogleCodeExporter closed 9 years ago

GoogleCodeExporter commented 9 years ago 
This issue is an enhancement of JCEF (patch 3 of 10) 
----------------------------------------------------
Purpose: Change CefHandler to act like CefClient from CEF.

This patch relies on the attached patch file of issue #. 
If you want to try these code changes, add the patch of issue # first and 
afterwards add the patch file attached to this issue. Otherwise you'll get some 
patch-errors.

This issue is divided into two patch files (3a and 3b).
With the first patch (3a), the interface CefHandler is renamed into 
CefClientHandler.

With the second patch (3b), the interface CefClientHandler is changed to an 
abstract class
which inherhits from CefNative (a new interface - see patch file for details).

Its previous methods are moved to the new handler interface
- CefLifeSpanHandler
and the new handlers introduced by patch 1 (see issue #)
- CefDisplayHandler
- CefRenderHandler
- CefMessageRouterHandler
- CefFocusHandler

CefClientHandler is extended by the new methods
- CTOR (forwarded to JNI)
- DTOR (forwarded to JNI)
- abstract method getDisplayHandler() (called by JNI)
- abstract method getFocusHandler() (called by JNI)
- abstract method getLifeSpanHandler() (called by JNI)
- abstract method getMessageRouterHandler() (called by JNI)
- abstract method getRenderHandler() (called by JNI)
- protected method removeDisplayHandler() (forwarded to JNI)
- protected method removeFocusHandler() (forwarded to JNI)
- protected method removeLifeSpanHandler() (forwarded to JNI)
- protected method removeMessageRouterHandler() (forwarded to JNI)
- protected method removeRenderHandler() (forwarded to JNI)

For LifeSpanHandler an adapter class is provided which methods are empty. 
- CefLifeSpanHandlerAdapter
This class exists as convenience for creating handler objects.

CefClient is extended by the methods
- addLifeSpanHandler(CefLifeSpanHandler handler)
- removeLifeSpanHandler()
to add and remove the handler implementation to the client.

Please see attached patch file for further informations.

Original issue reported on code.google.com by k...@censhare.de on 8 Mar 2014 at 7:54

Attachments:

GoogleCodeExporter commented 9 years ago 
This patch relies on the attached patch file of issue #46. 
If you want to try these code changes, add the patch of issue #46 first and 
afterwards add the patch file attached to this issue. Otherwise you'll get some 
patch-errors.

Original comment by k...@censhare.de on 8 Mar 2014 at 7:56

GoogleCodeExporter commented 9 years ago

Original comment by magreenb...@gmail.com on 12 Mar 2014 at 9:50

GoogleCodeExporter commented 9 years ago

Original comment by magreenb...@gmail.com on 12 Mar 2014 at 9:51

GoogleCodeExporter commented 9 years ago

Original comment by magreenb...@gmail.com on 12 Mar 2014 at 9:52

GoogleCodeExporter commented 9 years ago 
Initial patches added in revision 34. For the purposes of efficiency the below 
issues will be addressed in a follow-up change.

1. It looks like CefRenderHandler.onCursorChange is no longer called so we 
should probably remove it.

2. It seems like there are two distinct use cases for [Get|Set]CefForJNIObject 
--
a. Retrieving the CEF object for the |this| JNI object (like in CefBrowser_N), 
and;
b. Retrieving the CEF object for one of the other handler types (like 
CefDisplayHandler, CefLifeSpanHandler, etc. from ClientHandler).

The CefNative complexity isn't required for (a), which perhaps makes the usage 
overly complex (see #3 below). I wonder if we should split this into two 
separate native reference implementations. I.e., keep the old N_CefHandle 
method for (a) and use the new CefNative method for (b). What do you think?

3. If we continue to use CefNative for (a) is it necessary to expose the 
CefNative interface via the public CefBrowser and CefQueryCallback classes? It 
seems like CefNative should be an implementation detail of the *_N classes.

4. The org.cef import order in MainFrame.java should be alphabetical.

5. In MainFrame.onQuery don't both return false and execute callback.failure. 
Returning false will cause the failure to be sent automatically.

6. In ClientHandler::SetBrowser we're registering MessageRouterHandler (via 
AddHandler) when OnAfterCreated is called and never removing it. This could be 
a problem if we allow MessageRouterHandler to be changed or removed in the 
future.

Original comment by magreenb...@gmail.com on 28 Mar 2014 at 12:16

GoogleCodeExporter commented 9 years ago 
Hi Marshall,
@comment #5.1: CefRenderHandler.onCursorChange is called if you use OSR mode. 
So I don't think we should remove it.

@comment #5.2: You're right. The complexity of CefNatife isn't required for 2a) 
but I think using CefNative makes it more clear which classes have/need a 
native counterpart. And my suggestion is, that it makes the implementation 
somewhat easier because you don't need to keep in mind if you're using 2a) or 
2b). 

@comment #5.3: That's a design issue, you're right. I've fixed that in the 
attached patch file "2014-05-28_issue47a.patch". Now the only not *_N classes  
which inherits directly from CefNative, are CefClientHandler and the interfaces 
CefMessageRouterHandler, CefResourceHandler (see @comment #5.6) and 
CefURLRequestHandler. Beside that I've introduced CefNativeAdapter which 
contains the default implementation of CefNative. It is used in all cases where 
it is possible to directly extend it. 

@comment #5.4: You've already did it. Thanks for that.

@comment #5.5: That's fixed in the patch file 2014-05-28_issue47b.patch

@comment #5.6: That's a significant issue. For that reason I've changed the 
whole MessageRouterHandler implementation. Attached you'll find the patch file 
2014-06-03_issue47c.patch with the following changes:

(*) Introduced the Java classes CefMessageRouter and CefMessageRouterConfig
(*) Removed CefMessageRouterHandler from CefClient and added CefMessageRouter 
support instead
(*) Added a second handler example to: "Bookmarks->Binding Test 2"

What does that mean:
Now you can create _at any time_ a new instance of CefMessageRouter with an 
instance of CefMessageRouterConfig. This allows you to create additional 
JavaScript binding names like "myQuery" in the "Binding Test 2" example. You 
have to add the router to all CefClient instances which should react on the new 
query. Your CefMessageRouterHandler isn't added to CefClient anymore but to the 
CefMessageRouter instance which should use it. Now you can add multiple handler 
instances to CefMessageRouter and multiple CefMessageRouter instances to 
CefClient as well.

If you create a new CefMessageRouter, this is created at the browser process in 
the native code as well. 
After adding the CefMessageRouter instance to CefClient, the native code passes 
its configuration to all running render processes (of this client) by using 
"browser->SendProcessMessage()". The render process creates a new 
CefMessageRouterRendererSite instance on the fly and the binding is done. 
Render processes which are created afterwards receive all configurations for 
the CefMessageRouter renderer-site by "OnRenderThreadCreated".

On this way you can add and remove handlers at any time you want. See 
"Bookmarks->Binding Test 2" for a working example. 

Please Note: The attached patch files "a" to "c" rely on each other. So add "a" 
first and "b" and "c" afterwards.

Original comment by k...@censhare.de on 3 Jun 2014 at 9:36

Attachments:

GoogleCodeExporter commented 9 years ago 
@#6: Thanks, added in revision 72, revision 73 and revision 74 respectively.

Original comment by magreenb...@gmail.com on 17 Jun 2014 at 4:19