search

chromiumembedded / cef

Chromium Embedded Framework (CEF). A simple framework for embedding Chromium-based browsers in other applications.
https://bitbucket.org/chromiumembedded/cef/
Other
3.41k stars 468 forks source link

CEF3: Add off-screen rendering support #518

Closed magreenblatt closed 12 years ago

magreenblatt commented 12 years ago

Original report by me.

Original issue 518 created by magreenblatt on 2012-02-13T19:15:26.000Z:

Add off-screen rendering support. See http://magpcss.org/ceforum/viewtopic.php?f=10&t=645 for CEF3 information.

magreenblatt commented 11 years ago

Original comment by Anonymous.

Comment 52. originally posted by google@dav1d.de on 2012-12-10T22:49:06.000Z:

@ comment comment 49.: that's unfortunate

If anyone can point me into the right direction to add off-screen rendering for linux, I could probably help. Any hints appreciated.

magreenblatt commented 11 years ago

Original comment by Anonymous.

Comment 53. originally posted by Zhaojun.Meng on 2012-12-11T16:58:38.000Z:

Add a patch to support transparent background when off screen rendering.
CEF1 support transparent background when osr, because CEF1 called:
webviewhost_->webview()->setIsTransparent(true);
When using content, it's done by calling:
RenderWidgetHostImpl::SetBackground(), with a transparent bitmap.

Currently osr implement use StretchDIBits extracted from:
content/browser/renderer_host/backing_store_win.cc.
Which loss alpha channel(filled with value 0).

To apply alpha, I extract code from:
/content/browser/renderer_host/backing_store_aura.cc
to use SkBitmap and SkCanvas to implement backing store.
And it seems works not only on Windows.

magreenblatt commented 11 years ago

Original comment by Anonymous.

Comment 54. originally posted by rosca@adobe.com on 2012-12-11T17:54:03.000Z:

@ comment comment 53.: thanks a lot, I used the same source

I'm attaching the patch with following changes:
1) Alpha issue
-- backing store using skia instead of GDI and keeping alpha values
-- transparent background is set up when calling SetTransparentPainting on CefWindowInfo
-- Should work for all platforms
2) Select popups
-- Added a new argument for Invalidate, telling browser to invalidate the select popup or the main view
-- cefclient demo for select popups
-- the hook for select popup creation is done through a patch in chromium content.

To be done:
-- contribute patch content_popups.patch to http://code.google.com/p/chromium/issues/detail?id=155761
-- unit tests for alpha blending and select popups

magreenblatt commented 11 years ago

Original comment by Anonymous.

Comment 55. originally posted by rosca@adobe.com on 2012-12-17T22:57:23.000Z:

osr5.patch contains osr4.path and
-- unit test for transparent painting
-- unit tests for select popups
-- mouse event methods will take modifiers as an argument

magreenblatt commented 11 years ago

Original comment by Anonymous.

Comment 56. originally posted by a.dionisi@shinyweb.it on 2012-12-18T23:55:39.000Z:

Thanksssss!! :D

magreenblatt commented 11 years ago

Original comment by Anonymous.

Comment 57. originally posted by acron1 on 2013-01-04T09:52:25.000Z:

A release with OSR would be really, really awesome.. :) Pretty please?

magreenblatt commented 11 years ago

Comment 58. originally posted by magreenblatt on 2013-01-04T11:01:36.000Z:

@ commentscomment 53.-55: Thanks for the patches. Some comments related to osr5.patch, which overall looks quite good:

  1. http://code.google.com/p/chromiumembedded/issues/attachmentText?id=518&aid=5180055000&name=osr5.patch&token=emfylsLoUGsKNuMGw9Oax1Pa42Y%3A1357293242187comment 46.6

    ///
    // Send a key event to the browser.

    • // This method is only used when window rendering is disabled.
      ///
      /*--cef()--*/
      virtual void SendKeyEvent(const CefKeyEvent& event) =0;

The SendKeyEvent, SendMouseClickEvent, SendMouseMoveEvent and SendMouseWheelEvent might also be used with a windowed browser for automated testing purposes. It would be nice to continue to support these methods for that use case. Perhaps either keep the PlatformTranslate* methods in CefBrowserHostImpl or move them to a new shared location instead of moving them to RenderWidgetHostViewOSR. RenderWidgetHostViewOSR could then perform the globalX/globalY transformation after calling the PlatformTranslate* method. Also, the PlatformTranslate* methods are still platform-specific so the implementations should be in a platform-specific source file.

  1. http://code.google.com/p/chromiumembedded/issues/attachmentText?id=518&aid=5180055000&name=osr5.patch&token=emfylsLoUGsKNuMGw9Oax1Pa42Y%3A1357293242187comment 73.2

Is it now a requirement that GetRootScreenRect always be implemented? If so, please update the GetRootScreenRect comments in cef_render_handler.h.

  1. http://code.google.com/p/chromiumembedded/issues/attachmentText?id=518&aid=5180055000&name=osr5.patch&token=emfylsLoUGsKNuMGw9Oax1Pa42Y%3A1357293242187comment 89.6

Consider moving this code to a helper instead of duplicating it in SendMouseClickEvent, SendMouseMoveEvent and SendMouseWheelEvent.

  1. There seems to a problem computing the bounds for select popups. Currently, if you scroll so that the popup is at the bottom of the page it will show the popup below the control and clipped. Instead, it should show the popup above the control.

  2. We should add a new command-line flag to enable transparent off-screen rendering in cefclient for testing purposes.

magreenblatt commented 11 years ago

Original comment by Anonymous.

Comment 59. originally posted by iohanson on 2013-01-07T16:53:15.000Z:

@ commentcomment 56.: Thanks for comments.

I have attached osr6.patch with addressed comments. This patch also contains the fix for http://code.google.com/p/chromiumembedded/issues/detail?id=826

magreenblatt commented 11 years ago

Original comment by Anonymous.

Comment 60. originally posted by tuolin on 2013-01-08T07:47:14.000Z:

@ commentcomment 59. The content_popups.patch file seems to be missing from the patch.

magreenblatt commented 11 years ago

Original comment by Anonymous.

Comment 61. originally posted by iohanson on 2013-01-08T08:11:56.000Z:

@ commentcomment 60. Thanks, attached again

magreenblatt commented 11 years ago

Original comment by Anonymous.

Comment 62. originally posted by rosca@adobe.com on 2013-01-10T22:30:14.000Z:

The chromium bug regarding popup widget creation (http://code.google.com/p/chromium/issues/detail?id=155761) has been fixed. The changes got submitted with revision 176092 (bb) (http://src.chromium.org/viewvc/chrome?view=rev&revision=176092).

magreenblatt commented 11 years ago

Comment 63. originally posted by magreenblatt on 2013-01-11T18:17:18.000Z:

issue #826 has been merged into this issue.

magreenblatt commented 11 years ago

Comment 64. originally posted by magreenblatt on 2013-01-11T23:02:28.000Z:

@ commentcomment 61.: Thanks, I've committed your patch as revision 979 with the following changes:

A. Added a fix for comment 1. below.

B. Brought over the transparency.html test from CEF1 and added a "Transparency" item to the Tests menu.

C. Fixed the cefclient rotation effect to work even when the underlying web content is not updating.

D. Various minor style and documentation changes.

A few comments:

  1. http://code.google.com/p/chromiumembedded/issues/attachmentText?id=518&aid=5180059000&name=osr6.patch&token=uKS0CFEmpP4rHnjRV3Pdo3lOYlo%3A1357928243887comment 30.06

We should implicitly disable accelerated compositing in CefBrowserHost::CreateBrowser and CreateBrowserSync instead of requiring the client to explicitly disable it whenever off-screen rendering is used.

  1. It's probably not appropriate to always use monitor information as the basis for screen size with off-screen rendering. CEF1 provides a CefRenderHandler::GetScreenRect() callback that lets you specify the screen size. So, for example, the popup placement problem could be solved by having GetScreenRect() and GetViewRect() return the same value. Implementing this for CEF3 would likely require changes to Chromium since screen information is retrieved by RenderWidgetHostImpl::GetWebScreenInfo() via RenderWidgetHostView::GetNativeViewId().
magreenblatt commented 11 years ago

Original comment by Anonymous.

Comment 65. originally posted by olaru@adobe.com on 2013-01-17T17:17:33.000Z:

Hi,

I am attaching a first patch for OSR on Mac for feeback. I will work on adapting the unit tests, but thought I could receive some feedback in the meantime.

This patch has:

  1. Mac basic OSR (no popups): mouse and keyboard events, rendering
  2. the test view adapted from cef1
  3. HiDPI aware backing store, adapted mostly from the aura backing store, including screen switching.
  4. unit tests compile, do not crash, but do not pass either
  5. added CefBrowser::NotifyScreenInfoChanged and CefRenderHandler::GetScreenInfo along with a new type CefScreenInfo

To be done:

  1. The patch does not address popups; I am thinking of addressing popups in a future, separate patch.
  2. Make sure the patch builds on Windows
  3. Address an issue with transparency - there are some strange artifacts showing on screen in the transparency htmls.
  4. There is some code commented out in the v8 unit tests, because it did not compile on Mac - this should be fixed properly
  5. There are some changes in url_request_context_proxy.cc to address a crash when running OSR unit tests, and there might be a cleaner way to fix it.

Thanks

magreenblatt commented 11 years ago

Original comment by Anonymous.

Comment 66. originally posted by nvineeth on 2013-01-24T09:08:32.000Z:

@ commentcomment 65.:
In browser_host_impl_mac.mm I think the following change should be done in PlatformGetWindowHandle:
CefWindowHandle CefBrowserHostImpl::PlatformGetWindowHandle() {
return IsWindowRenderingDisabled() ?
window_info_.parent_view:
window_info_.view;
}

magreenblatt commented 11 years ago

Original comment by Anonymous.

Comment 67. originally posted by dreijerbit on 2013-01-24T17:52:25.000Z:

Has anybody tried using Flash with the latest OSR support on Windows? I'm unable to get the Flash plugin to render anything (e.g YouTube).

magreenblatt commented 11 years ago

Original comment by Anonymous.

Comment 68. originally posted by olaru@adobe.com on 2013-01-24T18:12:04.000Z:

@ 67
I did some quick testing before, and:

YouTube uses wmode window, IIRC.

magreenblatt commented 11 years ago

Original comment by Anonymous.

Comment 69. originally posted by dreijerbit on 2013-01-24T18:41:23.000Z:

comment 68.: Aha, that would explain it! Does anybody with knowledge of the Content API know how to fix this? So far, everything else has been working great (ordinary rendering, popups, etc.), so proper Flash support is about the only thing left for me in order to use CEF3 in a production build.

magreenblatt commented 11 years ago

Comment 70. originally posted by magreenblatt on 2013-01-24T18:44:15.000Z:

@ commentcomment 69.: We fixed plugin rendering in CEF1 by forcing Flash and Silverlight plugins to use windowless mode. Here's the related issue: http://code.google.com/p/chromiumembedded/issues/detail?id=214

magreenblatt commented 11 years ago

Comment 71. originally posted by magreenblatt on 2013-01-24T19:40:11.000Z:

@ commentcomment 65.:

  1. There is some code commented out in the v8 unit tests, because it did not compile
    on Mac - this should be fixed properly

This should be fixed in the current trunk.

  1. There are some changes in url_request_context_proxy.cc to address a crash when running
    OSR unit tests, and there might be a cleaner way to fix it.

Please file a separate issue for this.

And, some comments related to your patch:

  1. You can leave out the auto-generated files. That makes it easier to see the important changes.

  2. http://code.google.com/p/chromiumembedded/issues/attachmentText?id=518&aid=5180065000&name=osr\_mac1.patch&token=LkRxNVilsLCsjFYP0KU5\_M1KVec%3A1359053177756comment 18.2

+typedef struct _cef_screen_info_t {

Does the user need the ability to customize all of the values in this structure or can we leave some of them out? What happens if the user specifies strange values, or if they return false from GetScreenInfo()? Maybe returning false should cause it to use reasonable default values?

  1. http://code.google.com/p/chromiumembedded/issues/attachmentText?id=518&aid=5180065000&name=osr\_mac1.patch&token=LkRxNVilsLCsjFYP0KU5\_M1KVec%3A1359053177756comment 22.7

The variable is now called |parent_view|.

  1. http://code.google.com/p/chromiumembedded/issues/attachmentText?id=518&aid=5180065000&name=osr\_mac1.patch&token=LkRxNVilsLCsjFYP0KU5\_M1KVec%3A1359053177756comment 71.4

void CefBrowserHostImpl::PlatformTranslateKeyEvent(

Too much indentation. Should be 4 spaces.

  1. http://code.google.com/p/chromiumembedded/issues/attachmentText?id=518&aid=5180065000&name=osr\_mac1.patch&token=LkRxNVilsLCsjFYP0KU5\_M1KVec%3A1359053177756comment 88.2

Check if GetClient() or GetRenderHandler() return NULL.

  1. http://code.google.com/p/chromiumembedded/issues/attachmentText?id=518&aid=5180065000&name=osr\_mac1.patch&token=LkRxNVilsLCsjFYP0KU5\_M1KVec%3A1359053177756comment 88.6

|view| may be NULL.

  1. http://code.google.com/p/chromiumembedded/issues/attachmentText?id=518&aid=5180065000&name=osr\_mac1.patch&token=LkRxNVilsLCsjFYP0KU5\_M1KVec%3A1359053177756comment 95.4

Typo.

  1. http://code.google.com/p/chromiumembedded/issues/attachmentText?id=518&aid=5180065000&name=osr\_mac1.patch&token=LkRxNVilsLCsjFYP0KU5\_M1KVec%3A1359053177756comment 95.4

I guess that's just the way [NSMenu popUpContextMenu] works?

It's eventually returned via content::WebContentsDelegate::HandleContextMenu. The comment on that method says: "Returns true if the context menu operation was handled by the delegate."

Yes, if a parent view is provided (it may be NULL).

  1. http://code.google.com/p/chromiumembedded/issues/attachmentText?id=518&aid=5180065000&name=osr\_mac1.patch&token=LkRxNVilsLCsjFYP0KU5\_M1KVec%3A1359053177756comment 10.70

+#if defined(OS_MACOSX)
+// Called just before GetBackingStore blocks for an updated frame.
+void CefRenderWidgetHostViewOSR::AboutToWaitForBackingStoreMsg() {

These DLOG(INFO)s are probably just for your development, but they should be removed from the final version of the patch. Also, no need to duplicate comments from the parent class.

  1. http://code.google.com/p/chromiumembedded/issues/attachmentText?id=518&aid=5180065000&name=osr\_mac1.patch&token=LkRxNVilsLCsjFYP0KU5\_M1KVec%3A1359053177756comment 11.96

Don't duplicate comments from the parent class.

  1. http://code.google.com/p/chromiumembedded/issues/attachmentText?id=518&aid=5180065000&name=osr\_mac1.patch&token=LkRxNVilsLCsjFYP0KU5\_M1KVec%3A1359053177756comment 12.91

Something wrong with the indentation here.

  1. http://code.google.com/p/chromiumembedded/issues/attachmentText?id=518&aid=5180065000&name=osr\_mac1.patch&token=LkRxNVilsLCsjFYP0KU5\_M1KVec%3A1359053177756comment 13.06

And here.

  1. http://code.google.com/p/chromiumembedded/issues/attachmentText?id=518&aid=5180065000&name=osr\_mac1.patch&token=LkRxNVilsLCsjFYP0KU5\_M1KVec%3A1359053177756comment 18.85

+bool ClientOSRHandler::GetScreenPoint(CefRefPtr browser,

And here.

  1. http://code.google.com/p/chromiumembedded/issues/attachmentText?id=518&aid=5180065000&name=osr\_mac1.patch&token=LkRxNVilsLCsjFYP0KU5\_M1KVec%3A1359053177756comment 19.05

+CefRect convertRect(const NSRect& target, const NSRect& frame) {

Put global functions in an anonymous namespace or make them static.

  1. http://code.google.com/p/chromiumembedded/issues/attachmentText?id=518&aid=5180065000&name=osr\_mac1.patch&token=LkRxNVilsLCsjFYP0KU5\_M1KVec%3A1359053177756comment 19.81

Open bracket goes on the previous line.

  1. http://code.google.com/p/chromiumembedded/issues/attachmentText?id=518&aid=5180065000&name=osr\_mac1.patch&token=LkRxNVilsLCsjFYP0KU5\_M1KVec%3A1359053177756comment 24.53

Will you fix this in your updated patch?

  1. http://code.google.com/p/chromiumembedded/issues/attachmentText?id=518&aid=5180065000&name=osr\_mac1.patch&token=LkRxNVilsLCsjFYP0KU5\_M1KVec%3A1359053177756comment 25.88

+#if defined(OS_MACOSX)

Don't indent preprocessor directives.

  1. http://code.google.com/p/chromiumembedded/issues/attachmentText?id=518&aid=5180065000&name=osr\_mac1.patch&token=LkRxNVilsLCsjFYP0KU5\_M1KVec%3A1359053177756comment 26.11

+#else
+#error
+#endif

Add a message to the #error directive (like "Unsupported platform").

  1. http://code.google.com/p/chromiumembedded/issues/attachmentText?id=518&aid=5180065000&name=osr\_mac1.patch&token=LkRxNVilsLCsjFYP0KU5\_M1KVec%3A1359053177756comment 26.48

Better to use 1U instead of (size_t)1. We'll eventually make this change in other files as well.

  1. http://code.google.com/p/chromiumembedded/issues/attachmentText?id=518&aid=5180065000&name=osr\_mac1.patch&token=LkRxNVilsLCsjFYP0KU5\_M1KVec%3A1359053177756comment 26.60

Use 0x7f7f0000U instead of static_cast.

  1. http://code.google.com/p/chromiumembedded/issues/attachmentText?id=518&aid=5180065000&name=osr\_mac1.patch&token=LkRxNVilsLCsjFYP0KU5\_M1KVec%3A1359053177756comment 28.08

Don't indent preprocessor directives.

magreenblatt commented 11 years ago

Original comment by Anonymous.

Comment 72. originally posted by nvineeth on 2013-01-26T08:13:46.000Z:

@ commentcomment 65.:
http://code.google.com/p/chromiumembedded/issues/attachmentText?id=518&aid=5180065000&name=osr\_mac1.patch&token=XXgRI-PFlIFqDbEEWdNfv9PC4YQ%3A1359177777718comment 20.85

Shouldn't the |mouseUp| parameter be true, since the button is Up? Also changing this to true does not select the text when double clicked. On Windows, when this param is true, the double clicked test area is selected. Similarly pls check for other buttons as well.

magreenblatt commented 11 years ago

Original comment by Anonymous.

Comment 73. originally posted by nvineeth on 2013-01-30T09:22:25.000Z:

@ Commentcomment 72.
In r980 win also the param should be false for selection to happen on dbl click . My observation for Windows is not correct above.

magreenblatt commented 11 years ago

Original comment by Anonymous.

Comment 74. originally posted by olaru@adobe.com on 2013-02-06T17:59:53.000Z:

@ 71 Thanks for the comments Marshall. What is not mentioned below should be addressed in the patch.

  1. There are some changes in url_request_context_proxy.cc to address a crash when running
    OSR unit tests, and there might be a cleaner way to fix it.

Please file a separate issue for this.

I have added a ContextMenu test to the OSR unit tests, because I have not been able to reproduce the issue any other way. I assume to reproduce this would require some messing around with bot the UI and the API - show context menu, the destroy the browser. I will add the assert stack trace to the bug.

I can remove the workaround code from url_request_context_proxy.cc, but the unit tests will crash on the ContextMenu test.

On Windows, it does not crash, but leaves the menu on screen, and prevents the unit tests from exiting. Clicking on ViewSource will crash the unit tests.

  1. http://code.google.com/p/chromiumembedded/issues/attachmentText?id=518&aid=5180065000&name=osr\_mac1.patch&token=LkRxNVilsLCsjFYP0KU5\_M1KVec%3A1359053177756comment 18.2

+typedef struct _cef_screen_info_t {

Does the user need the ability to customize all of the values in this structure or can we leave some of them out?

It's worth noting that right now, the GetScreenInfo function is only called from content on Posix systems. It would be desirable for it to be called on Windows as well, but this will require a small patch to Chromium in RenderWidgetHostImpl::GetWebScreenInfo.

If you're asking whether we can make the structure simpler, I think so, yes. The content code seems to only access the device scale factor, and the rest is passed into WebKit. I haven't seen visible effects from modifying the other values, but I might be missing something, and not testing the appropriate use cases.

The defaults will be used if the client does not provide the information.

What happens if the user specifies strange values, or if they return false from GetScreenInfo()? Maybe returning false should cause it to use reasonable default values?

Defaults are used when returning false. I'm not sure about strange values though.

  1. http://code.google.com/p/chromiumembedded/issues/attachmentText?id=518&aid=5180065000&name=osr\_mac1.patch&token=LkRxNVilsLCsjFYP0KU5\_M1KVec%3A1359053177756comment 88.2
  • GetClient()->GetRenderHandler()->GetScreenPoint(GetBrowser(),

Check if GetClient() or GetRenderHandler() return NULL.

When window rendering is disabled, CreateBrowser checks that the client and render handler are not NULL, so the checks are not needed. I've also removed similar checks from the other implemented functions.

  1. http://code.google.com/p/chromiumembedded/issues/attachmentText?id=518&aid=5180065000&name=osr\_mac1.patch&token=LkRxNVilsLCsjFYP0KU5\_M1KVec%3A1359053177756comment 24.53
  • // FIXME: This logic fails if the user presses both Shift keys at once, for
  • // example: we treat releasing one of them as keyDown.

Will you fix this in your updated patch?

The osrtest files were initially taken from the CEF1 OSR test implementation, along with this code and comment. The original code (and comment) seems to be taken from WebKit, where it is duplicated in a number of files (third_party/WebKit/Source/WebKit/chromium/src/mac/WebInputEventFactory.mm). It is not fixed in any of them, and it probably involves holding a state of all modifier keys, which may not be worth the effort for a test application.

There is still an issue with transparency in cefclient, but it seems to happen only on HiDPI. There are artifacts when resizing the client window, as well as in some other use cases. When moving to a non-hidpi screen, the artifacts do not appear. I am investigating this issue.

Some notes about the unit tests:

magreenblatt commented 11 years ago

Original comment by Anonymous.

Comment 75. originally posted by olaru@adobe.com on 2013-02-12T12:13:13.000Z:

I've added a chromium bug and patch for the transparency issue:
https://code.google.com/p/chromium/issues/detail?id=175698
https://codereview.chromium.org/12249002/

I will create a Chromium patch in the mean time.

magreenblatt commented 11 years ago

Original comment by Anonymous.

Comment 76. originally posted by olaru@adobe.com on 2013-02-19T11:37:37.000Z:

Added issue https://code.google.com/p/chromiumembedded/issues/detail?id=887 for the crash.

magreenblatt commented 11 years ago

Original comment by Anonymous.

Comment 77. originally posted by olaru@adobe.com on 2013-02-21T16:39:57.000Z:

Adding a new version of OSR on Mac, with some fixes. This patch includes.

  1. A chromium patch to fix the transparency painting issue. This should be removed if the patch makes it into chromium.
  2. Fix cefclient navigation buttons, which were broken in the previous patch by the overriding of the cefclient window delegate with osr view.
  3. Renamed osrtest_mac.h/mm to cefclient_osr_widget_mac.h/mm to match the windows naming.
  4. A Chromium patch to allow overriding GetScreenInfo in content on Windows, just like on Mac. I have not implemented the callback in the client widget yet. I will add this as a Chromium patch in the near future.
magreenblatt commented 11 years ago

Original comment by Anonymous.

Comment 78. originally posted by olaru@adobe.com on 2013-02-21T18:03:12.000Z:

It appears that the GetScreenInfo Win changes in content.patch have already made their way into Chromium, before I could push for them myself.

magreenblatt commented 11 years ago

Original comment by Anonymous.

Comment 79. originally posted by olaru@adobe.com on 2013-02-22T11:55:33.000Z:

It appears that my editor automatically trimmed the whitespace from the previous patch, and it does not apply correctly. This new version should fix things.

magreenblatt commented 11 years ago

Comment 81. originally posted by magreenblatt on 2013-02-22T18:02:26.000Z:

@ commentcomment 78.: I'll be updating CEF to a newer Chromium version shortly.

@ commentcomment 79.: Thanks for the updated patch. Overall it looks good. Some comments:

  1. When creating patch files please use `git diff --no-prefix` and don't include generated files (*_capi.h, *_cpptoc.cc, etc).

  2. http://code.google.com/p/chromiumembedded/issues/attachmentText?id=518&aid=5180079000&name=osr\_mac2.1.patch\_fix&token=JLPGuWjmkmz\_2syRDBJ2YmPRO\_4%3A1361551776148comment 11.86

This syntax may fail with some compiler versions. Use assignment to the NSPoint members instead.

  1. http://code.google.com/p/chromiumembedded/issues/attachmentText?id=518&aid=5180079000&name=osr\_mac2.1.patch\_fix&token=JLPGuWjmkmz\_2syRDBJ2YmPRO\_4%3A1361551776148comment 20.14

Use lower-case names ("osrtest") here and in other places for consistency with existing resource URLs.

  1. http://code.google.com/p/chromiumembedded/issues/attachmentText?id=518&aid=5180079000&name=osr\_mac2.1.patch\_fix&token=JLPGuWjmkmz\_2syRDBJ2YmPRO\_4%3A1361551776148comment 23.4

Consider calling the argument something else (like |enabled|) so that we don't need to use |this| for the assignment.

And various style issues:

  1. http://code.google.com/p/chromiumembedded/issues/attachmentText?id=518&aid=5180079000&name=osr\_mac2.1.patch\_fix&token=JLPGuWjmkmz\_2syRDBJ2YmPRO\_4%3A1361551776148comment 26.9

We use American spelling (color vs colour) for consistency.

  1. http://code.google.com/p/chromiumembedded/issues/attachmentText?id=518&aid=5180079000&name=osr\_mac2.1.patch\_fix&token=JLPGuWjmkmz\_2syRDBJ2YmPRO\_4%3A1361551776148comment 33.3

+///
+// Class representing the virtual screen information for the offscreen
+// rendering screen
+///

Use proper punctuation (periods).

  1. http://code.google.com/p/chromiumembedded/issues/attachmentText?id=518&aid=5180079000&name=osr\_mac2.1.patch\_fix&token=JLPGuWjmkmz\_2syRDBJ2YmPRO\_4%3A1361551776148comment 12.33

And here.

  1. http://code.google.com/p/chromiumembedded/issues/attachmentText?id=518&aid=5180079000&name=osr\_mac2.1.patch\_fix&token=JLPGuWjmkmz\_2syRDBJ2YmPRO\_4%3A1361551776148comment 37.6

Wrong indentation. There's a lot of this in ClientOSRHandler as well.

  1. http://code.google.com/p/chromiumembedded/issues/attachmentText?id=518&aid=5180079000&name=osr\_mac2.1.patch\_fix&token=JLPGuWjmkmz\_2syRDBJ2YmPRO\_4%3A1361551776148comment 11.14

+//#include "cefclient/client_popup_handler.h"

Delete this line.

  1. http://code.google.com/p/chromiumembedded/issues/attachmentText?id=518&aid=5180079000&name=osr\_mac2.1.patch\_fix&token=JLPGuWjmkmz\_2syRDBJ2YmPRO\_4%3A1361551776148comment 12.58

Delete this line.

  1. http://code.google.com/p/chromiumembedded/issues/attachmentText?id=518&aid=5180079000&name=osr\_mac2.1.patch\_fix&token=JLPGuWjmkmz\_2syRDBJ2YmPRO\_4%3A1361551776148comment 24.79

Delete this line.

  1. http://code.google.com/p/chromiumembedded/issues/attachmentText?id=518&aid=5180079000&name=osr\_mac2.1.patch\_fix&token=JLPGuWjmkmz\_2syRDBJ2YmPRO\_4%3A1361551776148comment 24.69

Open bracket should be on the same line.

magreenblatt commented 11 years ago

Original comment by Anonymous.

Comment 82. originally posted by olaru@adobe.com on 2013-02-25T22:36:42.000Z:

Thank you for the feedback Marshall.

I realized I mixed up the commanl lines when posting the osr_mac2.1 patches, and instead of libcef_dll, I excluded the libcef folder (completely). I also trickled in some of the popups patch code, which is now removed. This also explains 1.

The new patch is based on CEF r1107, and addresses the comments, but the libcef folder still needs reviewing.

2,3 - done

    • I've changed it to be camel case notation, to be consistent with its cef_win.h counterpart.

And various style issues:

  1. I changed this, but fwiw it was copy pasted from WebKit.

2 - Done

3 and 6 are part of the popups implementation, so they no longer appear in the current patch. I will address them there.

  1. @ Done, and done. I hope I caught all indentation issues in ClientOSRHandler. Most were likely copy paste errors. Unfortunately, check_style.py and cpplint do not seem to support mm files.

5, 7, 8 - Done

magreenblatt commented 11 years ago

Original comment by Anonymous.

Comment 83. originally posted by dreijerbit on 2013-02-28T23:24:36.000Z:

Has anybody else notified a graphical artifact when scrolling the browser by clicking the middle mouse button, moving the mouse, and then clicking the middle mouse button again? I'm seeing the "move cursor" stay on the screen after scrolling has finished.

magreenblatt commented 11 years ago

Comment 84. originally posted by magreenblatt on 2013-02-28T23:28:23.000Z:

@ commentcomment 83.: This appears to be a bug in Chromium 25 and not specific to off-screen rendering. I'm experiencing it on some but not all websites with Chrome 25.0.1364.97 as well.

magreenblatt commented 11 years ago

Comment 85. originally posted by magreenblatt on 2013-03-05T16:57:36.000Z:

issue #901 has been merged into this issue.

magreenblatt commented 11 years ago

Comment 86. originally posted by magreenblatt on 2013-03-05T17:13:31.000Z:

@ commentcomment 82.: Overall looks good. Are there any other changes you'd like to include before this gets committed? Also, please update your patch to the current CEF trunk revision which uses Chromium revision 184577 (bb). Thanks.

magreenblatt commented 11 years ago

Comment 87. originally posted by magreenblatt on 2013-03-05T17:17:11.000Z:

magreenblatt commented 11 years ago

Original comment by Anonymous.

Comment 88. originally posted by olaru@adobe.com on 2013-03-07T17:45:33.000Z:

Attaching the osr_mac patch rebased on top of r1131. This patch should have nothing more to add, except for rebasing to the new revision.

I am also attaching a popups patch, wich applies on top of the code with the base osr_mac patch already applied. This is rather small, and does not modify a lot in terms of functionality:

The popups patch does not introduce major changes, but still needs to go through review, although it should be easier since the patch is smaller. If you think this can go in at the same time, all the better. If not, please go ahead withouth the second patch.

magreenblatt commented 11 years ago

Comment 89. originally posted by magreenblatt on 2013-03-15T16:08:47.000Z:

@ commentcomment 88.: Thanks for the updated patches. Some comments about osr_mac_popups.patch:

  1. https://code.google.com/p/chromiumembedded/issues/attachmentText?id=518&aid=5180088001&name=osr\_mac\_popups.patch&token=J9X1L4DQD6VRytN5CylWGcVhT44%3A1363361521194comment 72.

Perhaps this check should fail if any of the width/height dimensions are 0?

  1. https://code.google.com/p/chromiumembedded/issues/attachmentText?id=518&aid=5180088001&name=osr\_mac\_popups.patch&token=J9X1L4DQD6VRytN5CylWGcVhT44%3A1363361521194comment 13.4

    bool rotating_;

    • bool _wasLastMouseDownOnView;

Follow the style of the existing variable names.

  1. https://code.google.com/p/chromiumembedded/issues/attachmentText?id=518&aid=5180088001&name=osr\_mac\_popups.patch&token=J9X1L4DQD6VRytN5CylWGcVhT44%3A1363361521194comment 11.6

+===================================================================
+--- render_thread_impl.cc (revision 184577 (bb))
++++ render_thread_impl.cc (working copy)
+@ @ -299,7 +299,7 @ @

What effect does this have on windowed mode usage? Perhaps we should instead toggle this value somewhere in the CEF code only after checking that we're using off-screen rendering?

  1. https://code.google.com/p/chromiumembedded/issues/attachmentText?id=518&aid=5180088001&name=osr\_mac\_popups.patch&token=J9X1L4DQD6VRytN5CylWGcVhT44%3A1363361521194comment 21.1

Use a class variable instead so it doesn't break if someone creates multiple ClientOSRHandler instances in the future.

magreenblatt commented 11 years ago

Original comment by Anonymous.

Comment 90. originally posted by askthegimp on 2013-03-16T22:39:34.000Z:

Hi, here's a chromium patch that enables flash and silverlight rendering in offscreen mode (only tested on windows). The patch is almost identical to http://code.google.com/p/chromiumembedded/issues/detail?id=527. It's a quick and dirty hack, just for a start. Actually it does not check wether we are rendering offscreen or not, so perhaps a better idea would be to do the work in cef3 where RenderViewImpl::createPlugin is invoked and where we have visibility to the internal cef settings/flags.

magreenblatt commented 11 years ago

Original comment by Anonymous.

Comment 91. originally posted by dreijerbit on 2013-03-16T22:46:17.000Z:

comment 90.: Sweet. Progress on wmode support! :)

Quick question regarding the patch: You allow both opaque and transparent mode for Flash, but then you force it to opaque mode on line 53. Wouldn't it be better to let the value fall through all the way?

magreenblatt commented 11 years ago

Original comment by Anonymous.

Comment 92. originally posted by askthegimp on 2013-03-17T00:07:29.000Z:

checkout line 32, it's a bit confusing, flash is set to false when wmode is transparent or opaque....

magreenblatt commented 11 years ago

Original comment by Anonymous.

Comment 93. originally posted by dreijerbit on 2013-03-17T03:55:27.000Z:

comment 92.: Ah, right, you don't need to force anything if opaque and transparent are already set. Guess the naming of 'flash' (setting it to false is pretty misleading :) could probably be improved for readability, but that's up to Marshall to decide, I guess.

Excited to see this in trunk in some shape or form!

magreenblatt commented 11 years ago

Original comment by Anonymous.

Comment 94. originally posted by feibao.lou on 2013-03-25T07:04:44.000Z:

I'm waiting for the feature that get webkit-transform CSS style rendered properly in OSR......

magreenblatt commented 11 years ago

Original comment by Anonymous.

Comment 95. originally posted by rosca@adobe.com on 2013-03-25T09:18:10.000Z:

@ commentcomment 94. - What is the problem with webkit-transform in off-screen rendering? Please give more details. Are you referring to https://code.google.com/p/chromiumembedded/issues/detail?id=826?

magreenblatt commented 11 years ago

Original comment by Anonymous.

Comment 96. originally posted by efrencd on 2013-03-25T11:45:17.000Z:

Is there any plan to eneable accelerated compositing in Off-Screen mode?

magreenblatt commented 11 years ago

Original comment by Anonymous.

Comment 97. originally posted by askthegimp on 2013-03-27T15:31:25.000Z:

comment 91.: I just compiled the current trunk (r1161) with the flash/silverlight patch, if you want to try it you can download it from here: http://ytronix.com/cef3/

magreenblatt commented 11 years ago

Original comment by Anonymous.

Comment 98. originally posted by feibao.lou on 2013-03-27T15:43:37.000Z:

@ commentcomment 95. yes, seems no good ways to render 3D transform effect in OSR mode now, in CEF3.

magreenblatt commented 11 years ago

Comment 99. originally posted by magreenblatt on 2013-04-04T15:43:19.000Z:

@ commentcomment 88. (olaru@ adobe.com): Are you planning to update your patches in the near future? We're currently at the Chromium 27 branch point (revision 190564 (bb)) and it would be nice to have the OS-X support in that branch.

magreenblatt commented 11 years ago

Original comment by Anonymous.

Comment 100. originally posted by horia.olaru on 2013-04-04T15:59:00.000Z:

@ commentcomment 99. Yes, I will update my patches in the near future.

I have one quick question on commentcomment 89. - issue #3. You are right. It will, of course, affect windowed rendering and I'll fix it. To set the handling of popup menus correctly, the Renderer needs to be aware of the offscreen rendering mode. I was thinking of creating a new setting, and passing it as a command line parameter when the renderer is created. Does that sound like a good way to go?

magreenblatt commented 11 years ago

Original comment by Dmitry Azaraev (Bitbucket: Mystic Artifact).

Comment 101. originally posted by fddima on 2013-04-04T16:04:21.000Z:

@ commentcomment 10.0: (minor comment) so it is global setting, and both modes can't be mixed together? What happens if renderer limit reached and renderer process will be shared for host OSR browser and windowed browser inside?

magreenblatt commented 11 years ago

Comment 102. originally posted by magreenblatt on 2013-04-04T16:10:12.000Z:

@ commentcomment 10.0: A command-line flag won't work because a windowed and windowless browser may theoretically share the same render process if they have the same origin, or if running in single-process mode. Consider adding a new member to the CefBrowserInfo class instead. The information is then sent to the render process via CefProcessHostMsg_GetNewBrowserInfo (CefContentRendererClient::RenderViewCreated and CefBrowserMessageFilter::OnGetNewBrowserInfo).