search

roadlabs / chromiumembedded

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

CEF3: Add off-screen rendering support #518

Closed GoogleCodeExporter closed 9 years ago

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

Original issue reported on code.google.com by magreenb...@gmail.com on 13 Feb 2012 at 7:15

GoogleCodeExporter commented 9 years ago 
I add transparent to <body> element, like this: <body 
style="background:rgba(255,255,255,0)">, but the bitmap got from render process 
is opaque.
How can I support transparent background? Do I need to change code in 
content/renderer?

Original comment by Zhaojun....@gmail.com on 7 Dec 2012 at 1:54

GoogleCodeExporter commented 9 years ago 
@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.

Original comment by goo...@dav1d.de on 10 Dec 2012 at 10:49

GoogleCodeExporter commented 9 years ago 
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.

Original comment by Zhaojun....@gmail.com on 11 Dec 2012 at 4:58

Attachments:

GoogleCodeExporter commented 9 years ago 
@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

Original comment by ro...@adobe.com on 11 Dec 2012 at 5:54

Attachments:

GoogleCodeExporter commented 9 years ago 
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

Original comment by ro...@adobe.com on 17 Dec 2012 at 10:57

Attachments:

GoogleCodeExporter commented 9 years ago 
Thanksssss!! :D

Original comment by a.dionisi@shinyweb.org on 18 Dec 2012 at 11:55

GoogleCodeExporter commented 9 years ago 
A release with OSR would be really, really awesome.. :) Pretty please?

Original comment by acr...@gmail.com on 4 Jan 2013 at 9:52

GoogleCodeExporter commented 9 years ago 
@comments#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=51800
55000&name=osr5.patch&token=emfylsLoUGsKNuMGw9Oax1Pa42Y%3A1357293242187#466

   ///
   // 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.

2. 
http://code.google.com/p/chromiumembedded/issues/attachmentText?id=518&aid=51800
55000&name=osr5.patch&token=emfylsLoUGsKNuMGw9Oax1Pa42Y%3A1357293242187#732

-  // If GetRootScreenRect is not implemented use the view rect as the root
-  // screen rect.
   if (browser_impl_->GetClient()->GetRenderHandler()->GetRootScreenRect(
-          browser_impl_->GetBrowser(), rc) ||
-      browser_impl_->GetClient()->GetRenderHandler()->GetViewRect(
           browser_impl_->GetBrowser(), rc)) {
     return gfx::Rect(rc.x, rc.y, rc.width, rc.height);
   }

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

3. 
http://code.google.com/p/chromiumembedded/issues/attachmentText?id=518&aid=51800
55000&name=osr5.patch&token=emfylsLoUGsKNuMGw9Oax1Pa42Y%3A1357293242187#896

+  if (!IsPopupWidget() && popup_host_view_) {
+    if (popup_host_view_->popup_position_.Contains(
+        event.x, event.y)) {
+      CefMouseEvent popup_event(event);
+      popup_event.x -= popup_host_view_->popup_position_.x();
+      popup_event.y -= popup_host_view_->popup_position_.y();

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

4. 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.

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

Original comment by magreenb...@gmail.com on 4 Jan 2013 at 11:01

GoogleCodeExporter commented 9 years ago 
@comment#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

Original comment by iohan...@gmail.com on 7 Jan 2013 at 4:53

Attachments:

GoogleCodeExporter commented 9 years ago 
@comment#59 The content_popups.patch file seems to be missing from the patch.

Original comment by tuo...@gmail.com on 8 Jan 2013 at 7:47

GoogleCodeExporter commented 9 years ago 
@comment#60 Thanks, attached again

Original comment by iohan...@gmail.com on 8 Jan 2013 at 8:11

Attachments:

GoogleCodeExporter commented 9 years ago 
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 
(http://src.chromium.org/viewvc/chrome?view=rev&revision=176092).

Original comment by ro...@adobe.com on 10 Jan 2013 at 10:30

GoogleCodeExporter commented 9 years ago 
Issue 826 has been merged into this issue.

Original comment by magreenb...@gmail.com on 11 Jan 2013 at 6:17

GoogleCodeExporter commented 9 years ago 
@comment#61: Thanks, I've committed your patch as revision 979 with the 
following changes:

A. Added a fix for #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=51800
59000&name=osr6.patch&token=uKS0CFEmpP4rHnjRV3Pdo3lOYlo%3A1357928243887#3006

+ // disable accelerated compositing
+ if (!settings.accelerated_compositing_disabled) {
+   settings.accelerated_compositing_disabled = true;
+ }

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.

2. 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().

Original comment by magreenb...@gmail.com on 11 Jan 2013 at 11:02

GoogleCodeExporter commented 9 years ago 
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

Original comment by ol...@adobe.com on 17 Jan 2013 at 5:17

Attachments:

GoogleCodeExporter commented 9 years ago 
@comment#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;
}

Original comment by nvine...@gmail.com on 24 Jan 2013 at 9:08

GoogleCodeExporter commented 9 years ago 
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).

Original comment by dreijer...@gmail.com on 24 Jan 2013 at 5:52

GoogleCodeExporter commented 9 years ago 
@67
I did some quick testing before, and:
- Flash should work with wmode: "opaque" and "transparent" - draw conent on 
screen, and receive events.
- Flash support is not currently implemented in OSR with wmode: window, direct, 
gpu. Content is not drawn on screen

YouTube uses wmode window, IIRC.

Original comment by ol...@adobe.com on 24 Jan 2013 at 6:12

GoogleCodeExporter commented 9 years ago 
#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.

Original comment by dreijer...@gmail.com on 24 Jan 2013 at 6:41

GoogleCodeExporter commented 9 years ago 
@comment#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

Original comment by magreenb...@gmail.com on 24 Jan 2013 at 6:44

GoogleCodeExporter commented 9 years ago 
@comment#65:

> 4. 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.

> 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.

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=51800
65000&name=osr_mac1.patch&token=LkRxNVilsLCsjFYP0KU5_M1KVec%3A1359053177756#182

+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?

3. 
http://code.google.com/p/chromiumembedded/issues/attachmentText?id=518&aid=51800
65000&name=osr_mac1.patch&token=LkRxNVilsLCsjFYP0KU5_M1KVec%3A1359053177756#227

+  // If window rendering is disabled no browser window will be created. Set
+  // |m_ParentView| to the window that will act as the parent for popup menus,
+  // dialog boxes, etc.

The variable is now called |parent_view|.

4. 
http://code.google.com/p/chromiumembedded/issues/attachmentText?id=518&aid=51800
65000&name=osr_mac1.patch&token=LkRxNVilsLCsjFYP0KU5_M1KVec%3A1359053177756#714

void CefBrowserHostImpl::PlatformTranslateKeyEvent(
-    content::NativeWebKeyboardEvent& native_event,
-    const CefKeyEvent& event) {
-  // TODO(port): Implement this method as part of off-screen rendering support.
-  NOTIMPLEMENTED();
+        content::NativeWebKeyboardEvent& native_event,
+        const CefKeyEvent& key_event) {

Too much indentation. Should be 4 spaces.

5. 
http://code.google.com/p/chromiumembedded/issues/attachmentText?id=518&aid=51800
65000&name=osr_mac1.patch&token=LkRxNVilsLCsjFYP0KU5_M1KVec%3A1359053177756#882

+    GetClient()->GetRenderHandler()->GetScreenPoint(GetBrowser(),

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

6. 
http://code.google.com/p/chromiumembedded/issues/attachmentText?id=518&aid=51800
65000&name=osr_mac1.patch&token=LkRxNVilsLCsjFYP0KU5_M1KVec%3A1359053177756#886

+    NSView* view = window_info_.parent_view;
+    NSRect bounds = [view bounds];

|view| may be NULL.

7. 
http://code.google.com/p/chromiumembedded/issues/attachmentText?id=518&aid=51800
65000&name=osr_mac1.patch&token=LkRxNVilsLCsjFYP0KU5_M1KVec%3A1359053177756#954

+      // Showing the menu in OSR is prettu much self contained, only using

Typo.

8. 
http://code.google.com/p/chromiumembedded/issues/attachmentText?id=518&aid=51800
65000&name=osr_mac1.patch&token=LkRxNVilsLCsjFYP0KU5_M1KVec%3A1359053177756#954

+      // Showing the menu in OSR is prettu much self contained, only using
+      // the initialized menu_controller_ in this function, and the scoped
+      // variables in this block
+      // Questions:
+      // - why does the non-OSR context menu need the extra event and 
parent_view

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

+      // - what does the bool in the return of this function mean? should it 
return
+      // the value of the menu runner function?

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."

+      // - should a menu be actually shown in OSR - this could probably be 
prevented
+      // by the menu handler

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

9. 
http://code.google.com/p/chromiumembedded/issues/attachmentText?id=518&aid=51800
65000&name=osr_mac1.patch&token=LkRxNVilsLCsjFYP0KU5_M1KVec%3A1359053177756#1070

+#if defined(OS_MACOSX)
+// Called just before GetBackingStore blocks for an updated frame.
+void CefRenderWidgetHostViewOSR::AboutToWaitForBackingStoreMsg() {
+  DLOG(INFO) << "Not implemented: 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.

10. 
http://code.google.com/p/chromiumembedded/issues/attachmentText?id=518&aid=51800
65000&name=osr_mac1.patch&token=LkRxNVilsLCsjFYP0KU5_M1KVec%3A1359053177756#1196

+  // Called just before GetBackingStore blocks for an updated frame.
+  virtual void AboutToWaitForBackingStoreMsg() OVERRIDE;

Don't duplicate comments from the parent class.

11. 
http://code.google.com/p/chromiumembedded/issues/attachmentText?id=518&aid=51800
65000&name=osr_mac1.patch&token=LkRxNVilsLCsjFYP0KU5_M1KVec%3A1359053177756#1291

+  webScreenInfo.rect = WebKit::WebRect(src.rect.x, src.rect.y,
+                                                                        src.rect.width, src.rect.height);
+  webScreenInfo.availableRect = WebKit::WebRect(src.available_rect.x,
+                                                                                             src.available_rect.y,
+                                                                                           src.available_rect.width,
+                                                                                         src.available_rect.height);
+

Something wrong with the indentation here.

12. 
http://code.google.com/p/chromiumembedded/issues/attachmentText?id=518&aid=51800
65000&name=osr_mac1.patch&token=LkRxNVilsLCsjFYP0KU5_M1KVec%3A1359053177756#1306

+  CefRefPtr<CefRenderHandler> handler =
+                           browser_impl_->client()->GetRenderHandler();

And here.

13. 
http://code.google.com/p/chromiumembedded/issues/attachmentText?id=518&aid=51800
65000&name=osr_mac1.patch&token=LkRxNVilsLCsjFYP0KU5_M1KVec%3A1359053177756#1885

+bool ClientOSRHandler::GetScreenPoint(CefRefPtr<CefBrowser> browser,
+                            int viewX,
+                            int viewY,
+                            int& screenX,
+                            int& screenY) {

And here.

14. 
http://code.google.com/p/chromiumembedded/issues/attachmentText?id=518&aid=51800
65000&name=osr_mac1.patch&token=LkRxNVilsLCsjFYP0KU5_M1KVec%3A1359053177756#1905

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

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

15. 
http://code.google.com/p/chromiumembedded/issues/attachmentText?id=518&aid=51800
65000&name=osr_mac1.patch&token=LkRxNVilsLCsjFYP0KU5_M1KVec%3A1359053177756#1981

+    for (size_t i = 0; i < dirtyRects.size(); ++i)
+    {

Open bracket goes on the previous line.

16. 
http://code.google.com/p/chromiumembedded/issues/attachmentText?id=518&aid=51800
65000&name=osr_mac1.patch&token=LkRxNVilsLCsjFYP0KU5_M1KVec%3A1359053177756#2453

+  // 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?

17. 
http://code.google.com/p/chromiumembedded/issues/attachmentText?id=518&aid=51800
65000&name=osr_mac1.patch&token=LkRxNVilsLCsjFYP0KU5_M1KVec%3A1359053177756#2588

+#if defined(OS_MACOSX)
+  #include "tests/unittests/os_rendering_unittest_mac.h"
+#endif
+
+#if !defined(RGB)
+  #define RGB(r,g,b) ( ((uint32_t)(uint8_t)r)|\
+                       ((uint32_t)((uint8_t)g)<<8)|\
+                       ((uint32_t)((uint8_t)b)<<16))
+#endif

Don't indent preprocessor directives.

18. 
http://code.google.com/p/chromiumembedded/issues/attachmentText?id=518&aid=51800
65000&name=osr_mac1.patch&token=LkRxNVilsLCsjFYP0KU5_M1KVec%3A1359053177756#2611

+#else
+#error
+#endif

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

19. 
http://code.google.com/p/chromiumembedded/issues/attachmentText?id=518&aid=51800
65000&name=osr_mac1.patch&token=LkRxNVilsLCsjFYP0KU5_M1KVec%3A1359053177756#2648

-        EXPECT_EQ(dirtyRects.size(), 1);
+        EXPECT_EQ(dirtyRects.size(), (size_t)1);

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

20. 
http://code.google.com/p/chromiumembedded/issues/attachmentText?id=518&aid=51800
65000&name=osr_mac1.patch&token=LkRxNVilsLCsjFYP0KU5_M1KVec%3A1359053177756#2660

-        EXPECT_EQ(*(reinterpret_cast<const uint32*>(buffer)), 0x7f7f0000);
+        EXPECT_EQ(*(reinterpret_cast<const uint32*>(buffer)),
+                  static_cast<uint32>(0x7f7f0000));

Use 0x7f7f0000U instead of static_cast<uint32>.

21. 
http://code.google.com/p/chromiumembedded/issues/attachmentText?id=518&aid=51800
65000&name=osr_mac1.patch&token=LkRxNVilsLCsjFYP0KU5_M1KVec%3A1359053177756#2808

+  #if defined(OS_WIN)
     windowInfo.SetAsOffScreen(GetDesktopWindow());
+  #elif defined(OS_MACOSX)
+    windowInfo.SetAsOffScreen(osr_unittests::GetFakeView());
+  #else
+  #endif

Don't indent preprocessor directives.

Original comment by magreenb...@gmail.com on 24 Jan 2013 at 7:40

GoogleCodeExporter commented 9 years ago 
@comment#65:
http://code.google.com/p/chromiumembedded/issues/attachmentText?id=518&aid=51800
65000&name=osr_mac1.patch&token=XXgRI-PFlIFqDbEEWdNfv9PC4YQ%3A1359177777718#2085
+  browser_->GetHost()->SendMouseClickEvent(mouseEvent, MBT_LEFT, false, 
clickCount);

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.

Original comment by nvine...@gmail.com on 26 Jan 2013 at 8:13

GoogleCodeExporter commented 9 years ago 
@Comment#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.

Original comment by nvine...@gmail.com on 30 Jan 2013 at 9:22

GoogleCodeExporter commented 9 years ago 
@71 Thanks for the comments Marshall. What is not mentioned below should be 
addressed in the patch.

>> 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.
>
>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.

>2. 
http://code.google.com/p/chromiumembedded/issues/attachmentText?id=518&aid=51800
65000&name=osr_mac1.patch&token=LkRxNVilsLCsjFYP0KU5_M1KVec%3A1359053177756#182
>
>+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.

>5. 
http://code.google.com/p/chromiumembedded/issues/attachmentText?id=518&aid=51800
65000&name=osr_mac1.patch&token=LkRxNVilsLCsjFYP0KU5_M1KVec%3A1359053177756#882
>
>+    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.

>16. 
http://code.google.com/p/chromiumembedded/issues/attachmentText?id=518&aid=51800
65000&name=osr_mac1.patch&token=LkRxNVilsLCsjFYP0KU5_M1KVec%3A1359053177756#2453
>
>+  // 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:
- I have changed some event tests, for which expected results compared 
rectangles, to expect a notification from JS when an event was received. Paint 
regions are not the same across platforms, and these should not be paint tests, 
but event tests. The JS communication mechanism changes the location property 
as a way to notify the unit tests of an event. I've done this for speed, but it 
would probably be better to register an extension, and use that to communicate 
back to the unittests.
- I've moved the html string to an external html file, because it was 
cumbersome to edit the string to change a unit test. It is also easier to load 
the same file in cefclient this way. For this, I've included the 
resource_util_* file from cefclient, thoug it might be better if they were 
moved to a common folder next to cefclient and unittests.

Original comment by ol...@adobe.com on 6 Feb 2013 at 5:59

Attachments:

GoogleCodeExporter commented 9 years ago 
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.

Original comment by ol...@adobe.com on 12 Feb 2013 at 12:13

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

Original comment by ol...@adobe.com on 19 Feb 2013 at 11:37

GoogleCodeExporter commented 9 years ago 
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.

Original comment by ol...@adobe.com on 21 Feb 2013 at 4:39

Attachments:

GoogleCodeExporter commented 9 years ago 
It appears that the GetScreenInfo Win changes in content.patch have already 
made their way into Chromium, before I could push for them myself.
- made virtual in rev 183246
- the MACOSX and AURA defines were removed in rev 179899

Original comment by ol...@adobe.com on 21 Feb 2013 at 6:03

GoogleCodeExporter commented 9 years ago 
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.

Original comment by ol...@adobe.com on 22 Feb 2013 at 11:55

Attachments:

GoogleCodeExporter commented 9 years ago 
[deleted comment]
GoogleCodeExporter commented 9 years ago 
@comment#78: I'll be updating CEF to a newer Chromium version shortly.

@comment#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=51800
79000&name=osr_mac2.1.patch_fix&token=JLPGuWjmkmz_2syRDBJ2YmPRO_4%3A136155177614
8#1186

+  NSPoint view_pt = {viewX, bounds.size.height - viewY};

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

3. 
http://code.google.com/p/chromiumembedded/issues/attachmentText?id=518&aid=51800
79000&name=osr_mac2.1.patch_fix&token=JLPGuWjmkmz_2syRDBJ2YmPRO_4%3A136155177614
8#2014

+  } else if (url.find("http://tests/OSRTest" == 0) {

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

4. 
http://code.google.com/p/chromiumembedded/issues/attachmentText?id=518&aid=51800
79000&name=osr_mac2.1.patch_fix&token=JLPGuWjmkmz_2syRDBJ2YmPRO_4%3A136155177614
8#234

+  void SetTransparentPainting(bool transparent_painting) {
+    this->transparent_painting = transparent_painting;
+  }

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=51800
79000&name=osr_mac2.1.patch_fix&token=JLPGuWjmkmz_2syRDBJ2YmPRO_4%3A136155177614
8#269

+  ///
+  // The bits per colour component. This assumes that the colours are balanced
+  // equally.
+  ///

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

2. 
http://code.google.com/p/chromiumembedded/issues/attachmentText?id=518&aid=51800
79000&name=osr_mac2.1.patch_fix&token=JLPGuWjmkmz_2syRDBJ2YmPRO_4%3A136155177614
8#333

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

Use proper punctuation (periods).

3. 
http://code.google.com/p/chromiumembedded/issues/attachmentText?id=518&aid=51800
79000&name=osr_mac2.1.patch_fix&token=JLPGuWjmkmz_2syRDBJ2YmPRO_4%3A136155177614
8#1233

+    // clear the popup rectangles, so that the next paint will not repaint the
+    // popup

And here.

4. 
http://code.google.com/p/chromiumembedded/issues/attachmentText?id=518&aid=51800
79000&name=osr_mac2.1.patch_fix&token=JLPGuWjmkmz_2syRDBJ2YmPRO_4%3A136155177614
8#376

+  void Set(float device_scale_factor,
+                int depth,
+                int depth_per_component,
+                bool is_monochrome,
+                const CefRect& rect,
+                const CefRect& available_rect) {

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

5. 
http://code.google.com/p/chromiumembedded/issues/attachmentText?id=518&aid=51800
79000&name=osr_mac2.1.patch_fix&token=JLPGuWjmkmz_2syRDBJ2YmPRO_4%3A136155177614
8#1114

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

Delete this line.

6. 
http://code.google.com/p/chromiumembedded/issues/attachmentText?id=518&aid=51800
79000&name=osr_mac2.1.patch_fix&token=JLPGuWjmkmz_2syRDBJ2YmPRO_4%3A136155177614
8#1258

+  // view_->renderer_->OnPopupSize(browser, rect);

Delete this line.

7. 
http://code.google.com/p/chromiumembedded/issues/attachmentText?id=518&aid=51800
79000&name=osr_mac2.1.patch_fix&token=JLPGuWjmkmz_2syRDBJ2YmPRO_4%3A136155177614
8#2479

+    // AddResource(kTestUrl, kOsrHtml, "text/html");

Delete this line.

8. 
http://code.google.com/p/chromiumembedded/issues/attachmentText?id=518&aid=51800
79000&name=osr_mac2.1.patch_fix&token=JLPGuWjmkmz_2syRDBJ2YmPRO_4%3A136155177614
8#2469

-                       public CefContextMenuHandler {
+                       public CefContextMenuHandler
+{

Open bracket should be on the same line.

Original comment by magreenb...@gmail.com on 22 Feb 2013 at 6:02

GoogleCodeExporter commented 9 years ago 
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

4. - 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.

4. @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

Original comment by ol...@adobe.com on 25 Feb 2013 at 10:36

Attachments:

GoogleCodeExporter commented 9 years ago 
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.

Original comment by dreijer...@gmail.com on 28 Feb 2013 at 11:24

GoogleCodeExporter commented 9 years ago 
@comment#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.

Original comment by magreenb...@gmail.com on 28 Feb 2013 at 11:28

GoogleCodeExporter commented 9 years ago 
Issue 901 has been merged into this issue.

Original comment by magreenb...@gmail.com on 5 Mar 2013 at 4:57

GoogleCodeExporter commented 9 years ago 
@comment#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. Thanks.

Original comment by magreenb...@gmail.com on 5 Mar 2013 at 5:13

GoogleCodeExporter commented 9 years ago

Original comment by magreenb...@gmail.com on 5 Mar 2013 at 5:17

GoogleCodeExporter commented 9 years ago 

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:
- Made a patch to Chromium to enable popups to behave as on Windows (using the 
same code). Chromium, without the patch, woud display popups as menus, passing 
a list of the items in the select box. Aside from this, not a lot
- offset rectangles in cefclient on Mac to fit inside the view / window, as on 
Winwods.
- adapted cefclient to accomodate HiDPI popups
- modified unit tests html to send IPC messages instead of using navigation to 
communicate with the cpp unittests.
- all unit tests, including Popups are passing
- added better defaults for screen info - this will fix Windows popups and 
popup unittests, after the GetScreenInfo changes in the previous Chromium 
updates.

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.

Original comment by ol...@adobe.com on 7 Mar 2013 at 5:45

Attachments:

GoogleCodeExporter commented 9 years ago 
@comment#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=5180
088001&name=osr_mac_popups.patch&token=J9X1L4DQD6VRytN5CylWGcVhT44%3A13633615211
94#72

+  if (!handler->GetScreenInfo(browser_impl_.get(), screen_info) ||
+      (screen_info.rect.width == 0 && screen_info.rect.height == 0) ||
+      (screen_info.available_rect.width == 0 &&
+       screen_info.available_rect.height == 0)) {

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

2. 
https://code.google.com/p/chromiumembedded/issues/attachmentText?id=518&aid=5180
088001&name=osr_mac_popups.patch&token=J9X1L4DQD6VRytN5CylWGcVhT44%3A13633615211
94#134

   bool rotating_;
+
+  bool _wasLastMouseDownOnView;

Follow the style of the existing variable names.

3. 
https://code.google.com/p/chromiumembedded/issues/attachmentText?id=518&aid=5180
088001&name=osr_mac_popups.patch&token=J9X1L4DQD6VRytN5CylWGcVhT44%3A13633615211
94#116

+===================================================================
+--- render_thread_impl.cc  (revision 184577)
++++ render_thread_impl.cc  (working copy)
+@@ -299,7 +299,7 @@
+ 
+ #if defined(OS_MACOSX) || defined(OS_ANDROID)
+   // On Mac and Android, the select popups are rendered by the browser.
+-  WebKit::WebView::setUseExternalPopupMenus(true);
++  // WebKit::WebView::setUseExternalPopupMenus(true);
+ #endif

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?

4. 
https://code.google.com/p/chromiumembedded/issues/attachmentText?id=518&aid=5180
088001&name=osr_mac_popups.patch&token=J9X1L4DQD6VRytN5CylWGcVhT44%3A13633615211
94#211

+  static bool painting_popup_ = false;

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

Original comment by magreenb...@gmail.com on 15 Mar 2013 at 4:08

GoogleCodeExporter commented 9 years ago 
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.

Original comment by asktheg...@gmail.com on 16 Mar 2013 at 10:39

Attachments:

GoogleCodeExporter commented 9 years ago 
#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?

Original comment by dreijer...@gmail.com on 16 Mar 2013 at 10:46

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

Original comment by asktheg...@gmail.com on 17 Mar 2013 at 12:07

GoogleCodeExporter commented 9 years ago 
#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!

Original comment by dreijer...@gmail.com on 17 Mar 2013 at 3:55

GoogleCodeExporter commented 9 years ago 
I'm waiting for the feature that get webkit-transform CSS style rendered 
properly in OSR......

Original comment by feibao....@gmail.com on 25 Mar 2013 at 7:04

GoogleCodeExporter commented 9 years ago 
@comment#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?

Original comment by ro...@adobe.com on 25 Mar 2013 at 9:18

GoogleCodeExporter commented 9 years ago 
Is there any plan to eneable accelerated compositing in Off-Screen mode?

Original comment by efre...@gmail.com on 25 Mar 2013 at 11:45

GoogleCodeExporter commented 9 years ago 
#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/

Original comment by asktheg...@gmail.com on 27 Mar 2013 at 3:31

GoogleCodeExporter commented 9 years ago 
@comment#95 yes, seems no good ways to render 3D transform effect in OSR mode 
now, in CEF3.

Original comment by feibao....@gmail.com on 27 Mar 2013 at 3:43

GoogleCodeExporter commented 9 years ago 
@comment#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) 
and it would be nice to have the OS-X support in that branch.

Original comment by magreenb...@gmail.com on 4 Apr 2013 at 3:43

GoogleCodeExporter commented 9 years ago 
@comment#99 Yes, I will update my patches in the near future.

I have one quick question on comment#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?

Original comment by horia.ol...@gmail.com on 4 Apr 2013 at 3:59