pottermm / javachromiumembedded

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

Window Rendering: CefBrowser can't be embedded into a JSplitPane or JScrollPane #104

Closed GoogleCodeExporter closed 9 years ago

GoogleCodeExporter commented 9 years ago
What steps will reproduce the problem?
(Test 1)
1. Use latest SVN-Trunk (r99)
2. Apply attached patch 2014-07-25_testcase1_using-a-split-pane.patch
3. Start tests.simple.MainFrame

(Test 2)
1. Use latest SVN-Trunk (r99)
2. Apply attached patch 2014-07-25_testcase2_using-a-scroll-pane.patch
3. Start tests.simple.MainFrame

What is the expected output? What do you see instead?
(Test1) You'll see a JSplitPane. On the left a list of URL's on the right a 
CefBrowser instances.
1. Try to resize splited parts. -> This won't work for Mac as well as Windows
2. Try to make the whole window smaller: -> On mac you'll get a gray border

(Test2) You'll see a small window with. On top of the window is a 
CefBrowserInstance, on the bottom a Text area (both within one JPane). The Pane 
is embedded into a JScrollPane.
1. On Mac the Browser window is too big and fills out the whole window. You 
aren't able to scroll
2. On Windows if you scroll, the browser content moves out of its displayable 
rect.

See attached screenshots.

What version of the product are you using? On what operating system?
Latest SVN-Trunk (r99)

Please provide any additional information below.
This problem occurs on Mac and Windows using windowed rendering mode. OSR is 
not affected.
There are several reasons for this issue.
On Mac there is a missing NSView between the native Java part and the native 
CEF browser which is responsible to clip the browser window to its bounds.
On Windows Java doesn't update the visible region (clipped rect) of the native 
browser window correctly.

==> Please see my attached patch file 
"2014-07-25_modified_canvas_to_jframe.patch" for a solution of this issue.

Most significant changes in the patch file:
-------------------------------------------
[Java changes]
(*) Changed CefBrowserWr to use a JPanel instead of a Canvas.
  This is because JPanel is a lightweight component which delivers the right displayable rect. A heavyweight one hasn't those informations.
  The displayable rect is required to update the native embedded CefBrowser window.
(*) Windows still requires a Canvas object to be able to append the native 
browser to the java window. For this reason on windows the Canvas is a child of 
JPanel.
(*) To be able to react on scroll events, it's required to update the native 
code as soon as the ancestor has moved or resized
(*) The updating process is done by calling the new method "doUpdate()". It 
computes the visible rect as well as the browser rect. Those values are passed 
to the native world

[Native changes for Windows]
(*) The native UpdateUI method calls SetWindowRgn with the patched displayable 
rect.

[Native changes for Mac]
(*) Removed jni_util_mac.mm because it was never used
(*) Instead of adding CefBrowser as child of the windows main view, a content
view (CefBrowserContentView) is set between the main view and the
CefBrowser. Why?

The CefBrowserContentView defines the viewable part of the browser view.
In most cases the content view has the same size as the browser view,
but for example if you add CefBrowser to a JScrollPane, you only want to
see the portion of the browser window you're scrolled to. In this case 
the sizes of the content view and the browser view differ.

With other words the CefBrowserContentView clips the CefBrowser to its
displayable content.

Creation of the CefBrowserContentView is done by calling AddCefBrowser and 
destroyed
by calling RemoveCefBrowser within the LifeSpanHandler.

Original issue reported on code.google.com by k...@censhare.de on 25 Jul 2014 at 9:40

Attachments:

GoogleCodeExporter commented 9 years ago
Thanks for the patch. A few comments:

1. Line 124:

+  private Timer delayedUpdate_ = new Timer(100, new ActionListener() {

Is there some event that we could use as a trigger instead of using a 
hard-coded delay?

2. Line 729:

+-(void) setFrame:(NSRect)frameRect {
+  // Instead of using the passed frame, get the visible rect from java because
+  // the passed frame rect doesn't contain the clipped view part. Otherwise
+  // we'll overlay some parts of the Java UI.
+  if (cefBrowser.get()) {
+    CefRefPtr<CefRenderHandler> renderer =
+        cefBrowser->GetHost()->GetClient()->GetRenderHandler();
+
+    CefRect rect;
+    renderer->GetViewRect(cefBrowser, rect);
+    util_mac::TranslateRect(self, rect);
+    frameRect = (NSRect){{rect.x, rect.y}, {rect.width, rect.height}};
+  }
+  [super setFrame:frameRect];
+}

I think the way we're re-using CefRenderHandler for both windowed and 
windowless rendering is making the code more difficult to understand. It might 
be better to create a new Java interface that CefClient can implement just for 
the native windowed implementation and leave CefRenderHandler just for 
windowless rendering. It's not necessary to change it for this issue, but 
something to think about for the future.

3. Line 745:

+-(void) addCefBrowser:(CefRefPtr<CefBrowser>)browser {
+  cefBrowser = browser;
+  // Register for the start and end events of "liveResize" to avoid
+  // Java paint updates while someone is resizing the main window (e.g. by
+  // pulling with the mouse cursor) 
+  [[NSNotificationCenter defaultCenter] addObserver:self
+      selector:@selector(windowWillStartLiveResize:)
+      name:NSWindowWillStartLiveResizeNotification
+      object:[self window]];
+  [[NSNotificationCenter defaultCenter]
+      addObserver:self
+      selector:@selector(windowDidEndLiveResize:)
+      name:NSWindowDidEndLiveResizeNotification
+      object:[self window]];
+}
+
+-(void) removeCefBrowser {
+  cefBrowser = NULL;
+  [self removeFromSuperview];
+}

Should removeCefBrowser also remove the NSNotificationCenter observer?

4. Line 801:

+CefWindowHandle GetBrowserContentView(CefWindowHandle cefWindow, CefRect& 
orig) {

This function should probably be called CreateBrowserContentView instead.

5. Line 852:

+    NSString *contentStr = [[NSString alloc] 
initWithFormat:@"{{%d,%d},{%d,%d}",
+        contentRect.x, contentRect.y, contentRect.width, contentRect.height];
+    NSString *browserStr = [[NSString alloc] 
initWithFormat:@"{{%d,%d},{%d,%d}",
+        browserRect.x, browserRect.y, browserRect.width, browserRect.height];

Can we use the NSStringFromRect function here?

6. There are a number of lines in util_mac.mm that are longer than 80 
characters. Please wrap at 80 characters.

Original comment by magreenb...@gmail.com on 29 Jul 2014 at 10:31

GoogleCodeExporter commented 9 years ago
Hi Marshall,
thanks for your review.

@comment #1:
1) I didn't found an event which we could use to trigger the delayed updated. 
The other listeners (eg. ComponentListener) which can be added to component_ 
aren't  suitable for this issue.

2) I think that's a good idea. If you submit this patch to the svn, could you 
be so kind to create a new issue in the tracker which addresses this kind of 
improvement?

3) Yes you're right. "add" and "remove" are build asymmetric. I've adde the 
remove of the observer from the NSNotificationCenter to the method.

4) Renamed GetBrowserContentView to CreateBrowserContentView

5) NSStringFromRect requires a NSRect as input, not a CefRect. Changing the 
parameter type within the function name itself doesn't work because NSRect is 
an ObjC type but the function is called from the C++ code.
Another solution would be to use a type-cast. Something like that:
     NSString * contentStr = NSStringFromRect( (NSRect){{contentRect.x, contentRect.y}, {contentRect.width, contentRect.height}});
But rather than typecasting I would prefer to use the existing "initWithFormat" 
structure.

6) Fixed it. All lines are less or equal to 80 chars now.

Please see attached patch file for my changes.

Regards,
Kai

Original comment by k...@censhare.de on 30 Jul 2014 at 5:31

Attachments:

GoogleCodeExporter commented 9 years ago
@#2: Thanks, added in revision 100. Also filed issue #111 for the 
CefRenderHandler changes.

Original comment by magreenb...@gmail.com on 20 Aug 2014 at 1:46

GoogleCodeExporter commented 9 years ago
That's one delicate patch that must have required some significant work...

In our case, our application can have overlapping panes (fast views, similar to 
the Eclipse fast views). I had to disable this fix because of defect 120 (which 
contains a test case showing the pb). We can pursue conversation there.

Original comment by christop...@gmail.com on 22 Sep 2014 at 5:20