Non osr mode - reparent window when necessary (when AWT/Swing deletes Canvas peer) - possible Windows fix

[GoogleCodeExporter]

What steps will reproduce the problem?
1. Run MainFrame.java in non osr mode. Edit file so browser is moved to a 
different panel or frame at runtime.

What is the expected output? What do you see instead?
Browser stops drawing. When its Canvas got removed from its hierarchy, AWT 
destroyed the Canvas native window handle and the child CEF window as well.

What version of the product are you using? On what operating system?
Latest jcef r31. Windows 7.

Please provide any additional information below.
When embedding the browser in an existing Swing app, the browser will commonly 
be moved to different hierarchies - e.g. when embedded inside a JTabbedPane, 
reordering tabs is implemented by removing the tab then adding it back. So a 
native widget has to deal with the possibility of its native parent window 
being destroyed by AWT and recreated later.

The workaround is to detect when the native Canvas peer is about to be 
destroyed (Component.removeNotify on our Canvas or our immediate Container 
parent) and recreated (Component.addNotify on our Canvas or its immediate 
Container parent). When that happens, park our CEF child window to an invisible 
window.

Here's what I use for Windows. If you have other thoughts on how to support 
this, feedback welcome :-)

CefBrowser_N.cpp

JNIEXPORT void JNICALL Java_org_cef_CefBrowser_1N_N_1SetParent
  (JNIEnv *env, jobject obj, jobject parent) {
  CefRefPtr<CefBrowser> browser = GetCefFromJNIObject<CefBrowser>(env, obj);
  if(!browser.get())
    return;
  HWND parentHwnd = GetHwndOfCanvas(parent, env);
  HWND hwnd = browser->GetHost()->GetWindowHandle();

  SetParent(hwnd, parentHwnd);
}

CefBrowser_N.java
  public final native void N_SetParent(Canvas parent);

ChromiumBrowser.java // that's my class that wraps up the CEF client in a 
consumable AWT widget
public class ChromiumBrowser extends Container {
...
static Frame reparentingHiddenFrame;
static Canvas reparentingHiddenCanvas;
...
cefClient = new CefClient(clientDelegate, false, osr);
Canvas canvas = cefClient.getCanvas();
setLayout(new BorderLayout());
add(canvas, BorderLayout.CENTER);
...
// Create a single hidden realized frame to serve as a placeholder
// for our CEF child windows when AWT destroys their parent peer
reparentingHiddenFrame = new Frame();
reparentingHiddenFrame.setSize(0,0);
reparentingHiddenCanvas = new Canvas();
reparentingHiddenFrame.add(reparentingHiddenCanvas);
reparentingHiddenFrame.addNotify();

    public void addNotify() {
        super.addNotify();
        /**
         * New native canvas peer is alive now.
         */

        /**
         * off-screen-rendering mode simply draws to whatever current window we use. Nothing to do here.
         */
        if (osr)
            return;

        /**
         * Reparent cef browser window to new parent canvas peer. Do nothing if cef browser
         * has not been created or has been destroyed.
         */
        CefBrowser cefBrowser = cefClient.getBrowser();
        if (cefBrowser == null)
            return;
        setParent(cefClient.getCanvas());
    }

    public void removeNotify() {
        /** 
         * Current native canvas peer is about to be destroyed
         */

        /**
         * off-screen-rendering mode simply draws to whatever current window we use. Nothing to do here.
         */
        if (osr)
            return;

        /**
         * Reparent cef browser window to a temporary parent canvas peer. Do nothing if cef browser
         * has not been created or has been destroyed.
         */
        CefBrowser cefBrowser = cefClient.getBrowser();
        if (cefBrowser == null)
            return;

        /**
         * Park the CEF Window to a different window that is not going to be destroyed
         */
        setParent(reparentingHiddenCanvas);
        super.removeNotify();
    }

    void setParent(Canvas parent) {
        CefBrowser cefBrowser = cefClient.getBrowser();
        if (cefBrowser == null) {
            throw new RuntimeException();
        }
        CefBrowser_N cefBrowserN = (CefBrowser_N)cefBrowser;
        cefBrowserN.N_SetParent(parent);        
    }

I'm sharing this approach in case you find this helpful. This is for Windows 
only, not Mac obviously.

With your CEF knowledge, do you know of particular issues of reparenting the 
CefBrowser->GetHost()->GetWindowHandle() handle to a different window? It 
seemed fine in my testing with CEF 3.1750.

Original issue reported on code.google.com by christop...@gmail.com on 14 Mar 2014 at 5:14

[GoogleCodeExporter]

Your approach seems reasonable. Please submit your changes as a patch file 
against the current trunk revision.

Original comment by magreenb...@gmail.com on 17 Jun 2014 at 6:21

• Changed state: Accepted

[GoogleCodeExporter]

Will have to look how to break some of the code out of my wrapper and back into 
CefBrowser and CefClient.

Note I haven't investigated yet the Mac equivalent. Maybe similar approach with 
a Canvas and shifting CALayer around will work.

Original comment by christop...@gmail.com on 18 Jun 2014 at 12:42

[GoogleCodeExporter]

About to give it a try. I'll probably add a "reparent" test action to the 
MainFrame sample, to show how the current instance can be moved to a different 
parent / frame. Though a better example is a multi-tab app, since this also 
test the Z-order part.

Original comment by christop...@gmail.com on 25 Jun 2014 at 3:59

[GoogleCodeExporter]

Here's the patch, over latest r92.

To try it out:
Recompile native lib, recompile java.
Start Detailed MainFrame > Tests > Reparent.
A new frame appears with a button "Reparent <". Click on button. Magically, the 
current browser moves into that frame. Click on the button again - the browser 
moves back and forth between the 2 frames.

You can bring a third frame (Tests > Reparent) and make it move between these 2 
frames as long as you bring it back into the original frame first.

I've done minimal changes to the sample.

Note 1. In CefBrowserWr.java, removeNotify for the Canvas was previously 
calling parentWindowWillClose();
Of course with reparenting, we don't want to automatically close the browser in 
such case. We move it to a hidden frame until further notice - addNotify. The 
developer has to call cefBrowser.close() if they know they really don't need it 
anymore - e otherwise these will keep accumulate in the hidden frame until the 
final CefApp.shutdown closes them all.
E.g. in my case, I use it in multi-tabbing scenarios, and I know when the tab 
is meant to be closed I explicitly call cefBrowser.close() along with any other 
cleanup for that tab. But when the tab is reordered, we let the reparenting do 
the work (the actual canvas peer gets destroyed by AWT when tabs are moved 
around).

Note 2. During exit, frames get disposed and the removeNotify of Canvas'es get 
invoked. Once CefApp.shutdown is started, there's no need for 
Canvas.removeNotify in CefBrowserWr to do further reparenting. I found it 
useful to have a global variable  to not mess up with removeNotify if we have 
started CefApp.shutdown. I did not expose this variable in this patch.

It'd be nice if similar approach can be done for the Mac with CALayer's.

HTH let me know what you think @magreen

Original comment by christop...@gmail.com on 25 Jun 2014 at 8:18

Attachments:

• win_reparent.diff

[GoogleCodeExporter]

screenshot of modified Detailed app

Original comment by christop...@gmail.com on 25 Jun 2014 at 8:25

Attachments:

• reparent.png

[GoogleCodeExporter]

Hi Christoph,
thanks for that patch. Some comments:

1) Please correct your indentations:
  * Use 2 spaces instead of tabs
  * Remove extra whitespaces (e.g. CefBrowserWr.java between addNotify() and removeNotify()  
  * Add one space between "if" and "(" (e.g. CefApp.java, Hunk @@ -255,6 +268,11 @@ and others)
  * Avoid importing Java classes which aren't required (e.g. you're importing CefApp or CefClient in MenuBar.java but you never use them)
  * MenuBar,java: Hunk @@ -339,11 +349,56 @@:
        -        dlg.setVisible(true);^M
        +        dlg.setVisible(true);      ^M
     -> Remove white spaces after setVisible(true); then this line isn't changed

2) @Note1:  parentWindowWillClose() isn't used any more if you'll update to CEF 
version 3.1916.1749 (issue #94)

3) Why are you using one global JFrame and per CefBrowser one additional 
Canvas? So in case of reparenting you'll get: "JFrame -> Canvas -> Canvas"? 
Wouldn't it be enough to set a new parent to the first canvas and avoid calling 
"super.removeNotify()"?

4) I don't think CefApp is the right place for JFrame. This breaks the 
architecture of JCEF because CefApp is more the glue to CEF (start/stop/message 
loop/...) instead of an UI thing. And beside that one CefApp keeps 1..n 
CefClients where one CefClient keeps 1…n CefBrowser instances. So you will 
get one JFrame might keep many CefBrowser instances from many CefClient 
instances.
For me it would make more sense to keep one JFrame per CefBrowser or at lease 
one per CefClient. So you don't mix up CefBrowser instances from different 
CefClient instances.

5) Please make void N_SetParent(Canvas parent) private and create a protected 
wrapper method "setParent" within CefBrowser_N. Use try{…} 
catch(UnsatisfiedLinkError ule) { …} like the other protected methods within 
CefBrowser_N.

Regards, Kai

Original comment by k...@censhare.de on 7 Jul 2014 at 5:55

[GoogleCodeExporter]

Hi Kai:

Thanks for the feedback. Will work on it.

1) ok
2) ok. I did not introduce a new call to parentWindowWillClose(), it was there 
before. I just avoid calling it on platforms where we support reparenting. 
Unless CEF version 3.1916.1749 brings some hidden free behavior, things should 
be fine without it.
3) There certainly is more than one way to do this reparenting dance. Your 
suggestion is also interesting.

With my patch, this is what a complete typical reparenting cycle look like:
stage 1: SunAwtFrame1 > SunAwtCanvas1 > CefBrowserWindow1
The browser handle is under a Canvas under the Application's frame.

User initiates an action that drags the Canvas/browser to a different area 
(reorder tabs for example. This takes us to the transient stage 2.

stage 2: SunAwtFrame2(Hidden) > SunAwtCanvas2 > CefBrowserWindow1
In stage 2 we have temporarily parked CefBrowserWindow1 under a hidden frame. 
Its former canvas parent died its natural death. We gave it a new temporary 
canvas2 parent.

The application now created the drop destination (e.g. a new tab or new editor) 
and readds the CefBrowser to this new destination. This takes us to the last 
stage.

stage 3: SunAwtFrame1 > SunAwtCanvas3 > CefBrowserWindow1

The rational for this approach is to limit messing up with AWT since we are not 
AWT committers. We don't fight the natural handling of Canvas - we don't 
reparent the Canvas peer itself. We only reparent the CefBrowserWindow1 itself, 
which is a handle created by CEF and not AWT. So this gives us more chances not 
to be broken by future Java releases.
CefBrowserWindow1 is always parented to a Canvas. In case it ever has some 
assumption its parent is a Canvas, then that assumption is still true through 
the 3 stages of reparenting.

4) I understand the argument. The best solution over time will be the one that 
ports well to the other platforms and doesn't have side effect for the host 
application. It's nice to limit the number of hidden top windows. These top 
windows show up under MSFT Spy++, there's always a risk they can trigger some 
focus bug or break some internal AWT assumption. That's why at the moment I got 
good results with a single global hidden JFrame. I'd love to see reparenting 
done on the Mac as well to be more confident about what the ideal solution 
should be like. 

Note in theory, we could skip creating a hidden parent JFrame/Canvas all 
together and just create a transient parent window in native code. But we've 
been through the pain of making CEF work with JFrame/Canvas so it's nice to 
keep the reparenting through that known working path.

These are just thoughts, I'd be happy to have others evolve this patch and make 
it work on Mac as well...

5) ok

Original comment by christop...@gmail.com on 7 Jul 2014 at 2:52