Mac: Add windowed rendering support

[chromiumembedded]

Mac: Add windowed rendering support

[magreenblatt]

Comment 1. originally posted by magreenblatt on 2014-01-22T16:27:08.000Z:

Thanks for the patch. Some comments:

1. line 46:

• private boolean osr_;

Perhaps name this variable something more informative like |osr_enabled_|.

2. line 339:

• // start application with argument "--osr" for offscreen-rendering

Same here, call it "--osr-enabled".

3. line 65:

• if( osr_ ) {

The style guide would have you write this as "if (osr_) {". Same in other places that use if, while, etc.

4. line 287:

• public long getWindowHandleOfCanvas(Canvas canvas) {
• long result = 0;

•  Component comp = canvas;  
  

Wrong indent.

5. line 388:

• // On Mac OS X the message loop is performed by
• // calling initialize, which will block the main process
• // until the message loop will be quit
• if (!osr) {

•  CefContext.initialize("",osr);  
  

• }

For future reference on why this works: http://stackoverflow.com/questions/5642802/termination-of-program-on-main-thread-exit

6. line 428:

+static bool g_use_osr = false;

We may want to support both windowed and osr use cases in the same application, in which case we shouldn't use a global variable.

7. line 454:

•  command\_line->AppendSwitch("enable-speech-input");  
  

•  command\_line->AppendSwitch("enable-media-stream");  
  

Instead of hard-coding arbitrary flags we should consider forwarding |args| from main().

8. line 559:

+// Determines whether the current thread is already attached to the VM,
+// and tells the caller if it needs to later DetachCurrentThread

What are the performance characteristics of attaching/detaching the same thread multiple times? Would it be better to attach each CEF thread one time and then detach before application exit?

9. line 615:

+#define BEGIN_ENV(e) \
+JNIEnv *e = NULL; \
+bool __shouldDetach = false; \
+if (GetJNIEnv(&e, &__shouldDetach) == JNI_OK && e != NULL) { \
+
+#define END_ENV(e) \
+DetachFromThread(&__shouldDetach); \

Indent the 2nd+ lines of macros 4 spaces.

10. line 703:

Remove extra whitespace. Lots of this in util_mac.mm as well.

11. line 808:

+@ end // BrowserStub

Need 2 spaces before "//".

[magreenblatt]

Original comment by Anonymous.

---

Comment 2. originally posted by [email protected] on 2014-01-24T11:07:25.000Z:

Thanks for reviewing the patch.
Regarding your questions and comments:

1.) I've changed osr_ to osr_enabled_

2.) To correspond your example application ceflient,
I've changed the param to "--off-screen-rendering-enabled"

3.-4. and 9.-11.)
Sorry for that. I've corrected (hopefully all) that style-guide glitches.
Some of them came from the SDK, some of them I've missed to fix and some
of them came from copy-pasta.

5.) I've added your URL-reference to the in-line comment

6.) That's true, but it's currently hard to realize. I think this should happen
during the refactoring/extension of JCEF I've discussed with you (see my document I've sent to you).
Therefore I've added an TODO(jcef) remark to the code.

7.) Yes, you're right. I've missed to remove that hard coded values. I've done it
now. But according your suggestion, I've added an "forward params" mechanism into
Java/JNI which I've put into the separate Issue comment 33..
(see: https://code.google.com/p/javachromiumembedded/issues/detail?id=33)

8.) I never made any performance tests according this, but we're using these code
snippets (BEGIN_ENV, END_ENV) for years within our product and we never had any
performance issues. Beside this, currently the code is only used within the Obj-C
method "createBrowser" and therefore I haven't any misgivings about that.

See attached patch-file for my changes.

[magreenblatt]

Comment 3. originally posted by magreenblatt on 2014-01-30T18:27:01.000Z:

Added in revision 21 with minor style and documentation changes.

[magreenblatt]

Comment 4. originally posted by magreenblatt on 2014-01-31T15:07:24.000Z:

Missing jni_util_mac.mm added in revision 25.

[magreenblatt]

Original changes by Anonymous.

---

• set attachment to "2014-01-23_04-added_windowed_rendering_for_mac.patch"

[magreenblatt]

• set state to "resolved"