Closed GoogleCodeExporter closed 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
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:
@#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
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
Original issue reported on code.google.com by
k...@censhare.de
on 25 Jul 2014 at 9:40Attachments: