Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1731373002:
Allow to have a transparent UA dependent background. (Closed)
|
Created:
4 years, 10 months ago by Julien Isorce Samsung Modified:
4 years, 4 months ago CC:
chromium-reviews, tfarina, cc-bugs_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionAllow to have a transparent UA dependent background.
* --no-ua-dependent-background: avoid the call to contents_container_->set_background in BrowserView::InitViews().
* --transparent-background-rwhv: testing option to manually call RenderWidgetHostView->SetBackgroundColor(SK_ColorTRANSPARENT).
* WebView::OnPaintBackground: check if bbackground is set to be opaque before drawing background setup from ContentsWebView::OnThemeChanged() (CreateSolidBackground)
* DesktopWindowTreeHostX11::ShouldWindowContentsBeTransparent(): return "use_argb_visual_;" from ) instead of always false.
It allows to have the non opaque layer for PaintLayerCompositor::rootGraphicsLayer() ("Frame Overflow Controls Host Layer"). It corresponds to BrowserView::frame_->GetLayer(). So the
layer for window named "BrowserFrameAura".
It also makes consistency with DesktopWindowTreeHostX11::SetWindowTransparency() that uses "use_argb_visual_".
R=
BUG=589509
TEST: ./out/Release/chrome --enable-transparent-visuals --no-ua-dependent-background
--transparent-background-rwhv
Patch Set 1 #
Total comments: 8
Patch Set 2 : Decouple background color and transparency in RenderWidgetHostView #
Total comments: 2
Patch Set 3 : #
Messages
Total messages: 31 (14 generated)
Description was changed from ========== If render pass has transparent background then create a quad with blending support This allows to really have transparent background when 1 + 2 + 3: 1- Page is defined with <body style="background: transparent;">. 2- One has called content::RenderWidgetHostView->SetBackgroundColor(SK_ColorTRANSPARENT). 3- Main window is 32-bit depth (On Linux it currently requires to pass --enable-transparent-visuals) R= BUG=589509 ========== to ========== If render pass has transparent background then create a quad with blending support This allows to really have transparent background when 1 + 2 + 3: 1- Page is defined with <body style="background: transparent;">. 2- One has called content::RenderWidgetHostView->SetBackgroundColor(SK_ColorTRANSPARENT). 3- Main window is 32-bit depth (On Linux it currently requires to pass --enable-transparent-visuals) R= BUG=589509 CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel ==========
Description was changed from ========== If render pass has transparent background then create a quad with blending support This allows to really have transparent background when 1 + 2 + 3: 1- Page is defined with <body style="background: transparent;">. 2- One has called content::RenderWidgetHostView->SetBackgroundColor(SK_ColorTRANSPARENT). 3- Main window is 32-bit depth (On Linux it currently requires to pass --enable-transparent-visuals) R= BUG=589509 CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel ========== to ========== If render pass has transparent background then create a quad with blending support This allows to really have transparent background when 1 + 2 + 3: 1- Page is defined with <body style="background: transparent;">. 2- One has called content::RenderWidgetHostView->SetBackgroundColor(SK_ColorTRANSPARENT). 3- Main window is 32-bit depth (On Linux it currently requires to pass --enable-transparent-visuals) R= BUG=589509 CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel ==========
j.isorce@samsung.com changed reviewers: + pkasting@chromium.org, vmpstr@chromium.org
j.isorce@samsung.com changed reviewers: + phistuck@gmail.com
j.isorce@samsung.com changed reviewers: + boliu@chromium.org, danakj@chromium.org, vollick@chromium.org - pkasting@chromium.org
Hi, the important bit in this CL is in cc/layers/picture_layer_impl.cc: needs_blending = render_pass->has_transparent_background; Changes in browser_view.cc and contents_web_view.cc just serve as demo since it can be changed directly from a customized theme. A call to "content::RenderWidgetHostView->SetBackgroundColor(SK_ColorTRANSPARENT)" can be found in NativeAppWindowViews::RenderViewCreated or in WebViewGuest::SetAllowTransparency. The idea of this CL is to let the theme decides whether it is possible to see behind the view on some conditions. Currently even if the theme defines a background as transparent, the fact that the quad is not using blending (see picture_layer_impl.cc change) prevents from seeing behind the view in all cases, which is wrong if all layers and views are transparent.
trchen@chromium.org changed reviewers: + trchen@chromium.org
https://codereview.chromium.org/1731373002/diff/1/cc/layers/picture_layer_imp... File cc/layers/picture_layer_impl.cc (right): https://codereview.chromium.org/1731373002/diff/1/cc/layers/picture_layer_imp... cc/layers/picture_layer_impl.cc:99: #endif This doesn't make sense to me. Why does it have to non-static member when the value will be the same globally? And why the same logic doesn't apply to other layer types? https://codereview.chromium.org/1731373002/diff/1/cc/layers/picture_layer_imp... cc/layers/picture_layer_impl.cc:335: visible_geometry_rect, needs_blending, It doesn't make sense to force blending here, and I don't understand the criteria. There is no reason why every quad needs to blend just because the window buffer has an alpha channel and the render pass being non-opaque. You should look at DrawQuad::ShouldDrawWithBlending(), if the layer indeed contains transparent pixels, then it would be a non-opaque layer, and its opaque_rect would be empty, thus ShouldDrawWithBlending() should return true even without your change. The only possibility is that the visible_rect is empty, but that doesn't make sense and the quad should not even draw. Or somehow the layer is considered opaque in the visible area, which contradict with the fact that your test document being transparent. Either way your are hacking around around the underlying problem, not solving it. https://codereview.chromium.org/1731373002/diff/1/chrome/browser/ui/views/fra... File chrome/browser/ui/views/frame/browser_view.cc (right): https://codereview.chromium.org/1731373002/diff/1/chrome/browser/ui/views/fra... chrome/browser/ui/views/frame/browser_view.cc:2135: 0x00FFFFFF)); What does this mean? Is it pre-multiplied alpha or not? If pre-multiplied, masking alpha to 0 will result in invalid color. If not pre-multiplied, masking alpha to 0 is equivalent to fully transparent. Either way this looks weird and deserves a comment. https://codereview.chromium.org/1731373002/diff/1/chrome/browser/ui/views/fra... File chrome/browser/ui/views/frame/contents_web_view.cc (right): https://codereview.chromium.org/1731373002/diff/1/chrome/browser/ui/views/fra... chrome/browser/ui/views/frame/contents_web_view.cc:63: theme->GetColor(ThemeProperties::COLOR_NTP_BACKGROUND) & 0x00FFFFFF; ditto.
j.isorce@samsung.com changed reviewers: - phistuck@gmail.com
Thx for the review here are my replies. If you have additional hints to properly solve the problem that would be great :) https://codereview.chromium.org/1731373002/diff/1/cc/layers/picture_layer_imp... File cc/layers/picture_layer_impl.cc (right): https://codereview.chromium.org/1731373002/diff/1/cc/layers/picture_layer_imp... cc/layers/picture_layer_impl.cc:99: #endif On 2016/02/25 02:30:12, trchen wrote: > This doesn't make sense to me. Why does it have to non-static member when the > value will be the same globally? And why the same logic doesn't apply to other > layer types? You are right it should be static but I found this change really hackish in the first place. But I could not find a generic proper way to know if the Root UI window has its background transparent. (i.e. someone has called content::RenderWidgetHostView->SetBackgroundColor(SK_ColorTRANSPARENT) and it is effective) https://codereview.chromium.org/1731373002/diff/1/cc/layers/picture_layer_imp... cc/layers/picture_layer_impl.cc:335: visible_geometry_rect, needs_blending, On 2016/02/25 02:30:12, trchen wrote: > It doesn't make sense to force blending here, and I don't understand the > criteria. There is no reason why every quad needs to blend just because the > window buffer has an alpha channel and the render pass being non-opaque. For now all of this is still not clear to me. I'll dig it more. But you are right it should be only done only on the last pass if it is the one that compose with the Window. > > You should look at DrawQuad::ShouldDrawWithBlending(), if the layer indeed > contains transparent pixels, then it would be a non-opaque layer, and its > opaque_rect would be empty, thus ShouldDrawWithBlending() should return true > even without your change. What I do not understand it that even if ShouldDrawWithBlending() return true the problem is that it has been created with needs_blending=false (this is the case when using quad->SetNew instead of quad->SetAll) so it won't be able to blend. What am I missing :) ? > > The only possibility is that the visible_rect is empty, but that doesn't make > sense and the quad should not even draw. Or somehow the layer is considered > opaque in the visible area, which contradict with the fact that your test > document being transparent. > > Either way your are hacking around around the underlying problem, not solving > it. I agree it is currently hackish that why also I took the freedom to ship "demo" code in that CL (see my first comment in this CL). https://codereview.chromium.org/1731373002/diff/1/chrome/browser/ui/views/fra... File chrome/browser/ui/views/frame/browser_view.cc (right): https://codereview.chromium.org/1731373002/diff/1/chrome/browser/ui/views/fra... chrome/browser/ui/views/frame/browser_view.cc:2135: 0x00FFFFFF)); On 2016/02/25 02:30:12, trchen wrote: > What does this mean? Is it pre-multiplied alpha or not? > If pre-multiplied, masking alpha to 0 will result in invalid color. > If not pre-multiplied, masking alpha to 0 is equivalent to fully transparent. > Either way this looks weird and deserves a comment. Sorry for the confusion, this code has no intention to be merged. In the first comment of this CL I refer to that as "demo" code. I just force the theme to have its background fully transparent. In real world, the hypothetic custom theme would defines its background as fully transparent. https://codereview.chromium.org/1731373002/diff/1/chrome/browser/ui/views/fra... File chrome/browser/ui/views/frame/contents_web_view.cc (right): https://codereview.chromium.org/1731373002/diff/1/chrome/browser/ui/views/fra... chrome/browser/ui/views/frame/contents_web_view.cc:63: theme->GetColor(ThemeProperties::COLOR_NTP_BACKGROUND) & 0x00FFFFFF; On 2016/02/25 02:30:12, trchen wrote: > ditto. Same
I see. Basically this is a proof of concept that is not meant to be submitted. Transparent background is a rarely used feature. We do have a few supported use cases, such as fullscreen video on Android, and transparent WebView. However it is sensitive to configuration / initialization steps. You will be on your own to investigate why it does not work in your specific use case. I would say it is very unlikely a cc/blink problem, because WebView is subject to very similar condition (cc doesn't own the framebuffer and has to blend with contents under) and it worked.
Description was changed from
==========
If render pass has transparent background then create a quad with blending
support
This allows to really have transparent background when 1 + 2 + 3:
1- Page is defined with <body style="background: transparent;">.
2- One has called
content::RenderWidgetHostView->SetBackgroundColor(SK_ColorTRANSPARENT).
3- Main window is 32-bit depth (On Linux it currently requires to pass
--enable-transparent-visuals)
R=
BUG=589509
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel
==========
to
==========
Decouple background color and transparency in RenderWidgetHostView
Currently it is not possible from the RenderWidgetHostView to set 0
as the background color and set it as opaque. Because the later
is computer from the former.
Whereas it is possible to control them indenpendently in RenderWidgetCompositor,
cc::LayerTreeHost and blink::WebView, see below:
RenderWidgetCompositor::setBackgroundColor
RenderWidgetCompositor::setHasTransparentBackground
cc::LayerTreeHost::set_background_color
cc::LayerTreeHost::set_has_transparent_background
blink::WebView::setBaseBackgroundColor
blink::WebView::setIsTransparent
So this CL add extra IPC ViewMsg_SetBackgroundColor for more granularity.
This CL also decouple background color and opacity in RenderWidgetHostView.
This CL allows to really have transparent background when 1 + 2:
1- Page is defined with <body style="background: transparent;"> which is the
default value.
2- One has called
content::RenderWidgetHostView->SetBackgroundColor(SK_ColorTRANSPARENT)
and content::RenderWidgetHostView->SetBackgroundOpaque(true)
Additionnal condition on Linux only:
3- Main window is 32-bit depth (On Linux it currently requires to pass
--enable-transparent-visuals)
R=trchen@chromium.org
BUG=589509
==========
On 2016/02/25 23:07:55, trchen wrote: > I see. Basically this is a proof of concept that is not meant to be submitted. > > Transparent background is a rarely used feature. We do have a few supported use > cases, such as fullscreen video on Android, and transparent WebView. However it > is sensitive to configuration / initialization steps. You will be on your own to > investigate why it does not work in your specific use case. > > I would say it is very unlikely a cc/blink problem, because WebView is subject > to very similar condition (cc doesn't own the framebuffer and has to blend with > contents under) and it worked. I had another look and I based the new CL on the fact that in Chromium it is possible for a layer to have its background color as rgba(0, 0, 0, 0) and but still being marked as opaque. So that it clears everything behind. I think this is also used for video hole where the color is rgba(0, 0, 0, 0) but the opaque_rect being equal to the quad rect. In this "Patch Set 2" I think the new IPC is fine since it allows more granularity (though maybe I should keep the previous behavior of RenderWidgetHostViewAura::SetBackgroundColor for compatibility purpose). The change in picture_layer_impl.cc have more impact but I think I did it correctly. But I am concern with the change I made in LayoutBox.cpp though it sounds logic. I did it because it is required to make contents_opaque() returns true from picture_layer_impl.cc. I have not found another way. Please let me know what you think about these changes, thx in advance.
In addition to the previous comment, it is worth to mention that I updated the description of this CL to match "Patch Set 2". Please have a look too.
I don't think the changes in cc and blink are correct or even needed. All you need is the IPC plumbing, given that there is no interface in RenderView to override base background color. However this CL doesn't show how will this new interface be used. You can't land a new interface that will be used only by your downstream code. Someone will found it unused and remove it before you know it. Either you need to discuss your new feature on chromium-dev and make it a supported feature. Or you will have to keep it downstream. https://codereview.chromium.org/1731373002/diff/20001/cc/layers/picture_layer... File cc/layers/picture_layer_impl.cc (right): https://codereview.chromium.org/1731373002/diff/20001/cc/layers/picture_layer... cc/layers/picture_layer_impl.cc:340: } Why is this change needed? This only changes the behavior when a layer is opaque, but what the thing you want to change is how non-opaque pages being handled. https://codereview.chromium.org/1731373002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/layout/LayoutBox.cpp (right): https://codereview.chromium.org/1731373002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/layout/LayoutBox.cpp:1291: return !isTransparent(); This change is incorrect. The document element indeed never paints backgrounds, and the body doesn't paint background if it is stolen as root background. And I don't think it is related to the thing you want to achieve. If your problem is things being considered opaque when they are not, why making more things considered opaque helps?
On 2016/03/08 01:57:21, trchen wrote: > I don't think the changes in cc and blink are correct or even needed. > All you need is the IPC plumbing, given that there is no interface in RenderView > to override base background color. Thx for your comments, that's really help full and sorry for the late reply. > > However this CL doesn't show how will this new interface be used. You can't land > a new interface that will be used only by your downstream code. Someone will > found it unused and remove it before you know it. All right I understand. If I get to a proper solution I'll make a unit test (browser test maybe) that uses this API. > > Either you need to discuss your new feature on chromium-dev and make it a > supported feature. Or you will have to keep it downstream. Oki I will consider this option. > > https://codereview.chromium.org/1731373002/diff/20001/cc/layers/picture_layer... > File cc/layers/picture_layer_impl.cc (right): > > https://codereview.chromium.org/1731373002/diff/20001/cc/layers/picture_layer... > cc/layers/picture_layer_impl.cc:340: } > Why is this change needed? This only changes the behavior when a layer is > opaque, but what the thing you want to change is how non-opaque pages being > handled. My point is the uses the same logic as VIDEO_HOLE: https://chromium.googlesource.com/chromium/src.git/+/master/cc/layers/video_l... It calls solid_color_draw_quad->SetAll with SK_ColorTRANSPARENT + opaque_rect = quad_rect. As a result the paint operation is equivalent as drawing with SkXfermode::kClear_Mode, like glClear(0, 0, 0, 0) would do. I.e. it punches through. > > https://codereview.chromium.org/1731373002/diff/20001/third_party/WebKit/Sour... > File third_party/WebKit/Source/core/layout/LayoutBox.cpp (right): > > https://codereview.chromium.org/1731373002/diff/20001/third_party/WebKit/Sour... > third_party/WebKit/Source/core/layout/LayoutBox.cpp:1291: return > !isTransparent(); > This change is incorrect. The document element indeed never paints backgrounds, > and the body doesn't paint background if it is stolen as root background. Right, and I saw now that it breaks layout tests :) > > And I don't think it is related to the thing you want to achieve. If your > problem is things being considered opaque when they are not, why making more > things considered opaque helps? There are 2 approach, this CL explores the one that actually punch through the theme/window. The other approach would be that a call to content::RenderWidgetHostView->SetBackgroundColor(SK_ColorTRANSPARENT) would also tell the them and native window to not draw anything. Because currently this call does not prevent the background setup in contents_web_view.cc::OnThemeChanged() "const int kBackgroundBrightness = 0x33; set_background(views::Background::CreateSolidBackground...". And I suspect another entity behind to draw something as well, maybe the native window.
On 2016/03/15 17:52:19, j.isorce wrote: > On 2016/03/08 01:57:21, trchen wrote: > https://codereview.chromium.org/1731373002/diff/20001/cc/layers/picture_layer... > > File cc/layers/picture_layer_impl.cc (right): > > > > > https://codereview.chromium.org/1731373002/diff/20001/cc/layers/picture_layer... > > cc/layers/picture_layer_impl.cc:340: } > > Why is this change needed? This only changes the behavior when a layer is > > opaque, but what the thing you want to change is how non-opaque pages being > > handled. > > My point is the uses the same logic as VIDEO_HOLE: > https://chromium.googlesource.com/chromium/src.git/+/master/cc/layers/video_l... > It calls solid_color_draw_quad->SetAll with SK_ColorTRANSPARENT + opaque_rect = > quad_rect. As a result the paint operation is equivalent as drawing with > SkXfermode::kClear_Mode, like glClear(0, 0, 0, 0) would do. I.e. it punches > through. If I understand correctly, the VIDEO_HOLE thing is exactly the opposite of what you want to achieve. For VIDEO_HOLE, I believe the render model is that we own our FBO, and the video playback is on another FBO that is stacked below our FBO by hardware. The mechanism was added to allow a video layer to replace the pixels below (e.g. a white background) to become (0,0,0,0) instead of src-over with (0,0,0,0) (i.e. is a no-op). For your application, I believe it is the case that we don't own the FBO. X11 owns the FBO and will pass us with existing pixel from windows that are below us. In this case we actually want to make sure 1. The base background is set to (0,0,0,0), and 2. We draw our stuff with src-over instead of just overwrite with raw pixel value. My understanding could be wrong. Especially I suspect X11 behaves differently whether a compositing window manager presents. Please verify and confirm. Thanks!
trchen@chromium.org changed reviewers: - boliu@chromium.org
On 2016/03/15 21:04:26, trchen wrote: > On 2016/03/15 17:52:19, j.isorce wrote: > > On 2016/03/08 01:57:21, trchen wrote: > > > https://codereview.chromium.org/1731373002/diff/20001/cc/layers/picture_layer... > > > File cc/layers/picture_layer_impl.cc (right): > > > > > > > > > https://codereview.chromium.org/1731373002/diff/20001/cc/layers/picture_layer... > > > cc/layers/picture_layer_impl.cc:340: } > > > Why is this change needed? This only changes the behavior when a layer is > > > opaque, but what the thing you want to change is how non-opaque pages being > > > handled. > > > > My point is the uses the same logic as VIDEO_HOLE: > > > https://chromium.googlesource.com/chromium/src.git/+/master/cc/layers/video_l... > > It calls solid_color_draw_quad->SetAll with SK_ColorTRANSPARENT + opaque_rect > = > > quad_rect. As a result the paint operation is equivalent as drawing with > > SkXfermode::kClear_Mode, like glClear(0, 0, 0, 0) would do. I.e. it punches > > through. > > If I understand correctly, the VIDEO_HOLE thing is exactly the opposite of what > you want to achieve. > > For VIDEO_HOLE, I believe the render model is that we own our FBO, and the video > playback is on another FBO that is stacked below our FBO by hardware. The > mechanism was added to allow a video layer to replace the pixels below (e.g. a > white background) to become (0,0,0,0) instead of src-over with (0,0,0,0) (i.e. > is a no-op). Your explanation is really helpful, thx. > > For your application, I believe it is the case that we don't own the FBO. X11 > owns the FBO and will pass us with existing pixel from windows that are below > us. In this case we actually want to make sure 1. The base background is set to > (0,0,0,0), and 2. We draw our stuff with src-over instead of just overwrite with > raw pixel value. > > My understanding could be wrong. You understood exactly what is my use case. > Especially I suspect X11 behaves differently > whether a compositing window manager presents. Please verify and confirm. > Thanks! I checked and everything seems to work as expected. content::RenderWidgetHostView->SetBackgroundColor(SK_ColorTRANSPARENT) calls: void RenderWidgetHostViewAura::SetBackgroundColor(SkColor color) { RenderWidgetHostViewBase::SetBackgroundColor(color); bool opaque = GetBackgroundOpaque(); host_->SetBackgroundOpaque(opaque); window_->layer()->SetFillsBoundsOpaquely(opaque); window_->layer()->SetColor(color); } But those calls do not affect all the 5 following "layers": (From below to over) 1: NativeWindow (DesktopWindowTreeHostX11) 2: views::RootView (See OnPaint below) 3: BrowserView (See InitViews below) 4: ContentsWebView (See OnThemeChanged below) 5: web layer tree 1: For the X11 window there is no background thanks too "swa.background_pixmap = None;" 2: Seems ok since it relies on "layer()->fills_bounds_opaquely()" which is set from the entry call above. RootView::OnPaint(gfx::Canvas* canvas) if (!layer()->fills_bounds_opaquely()) canvas->DrawColor(SK_ColorBLACK, SkXfermode::kClear_Mode); 3: Not ok, the following background will always be there. contents_container_->set_background(views::Background::CreateSolidBackground( GetThemeProvider()->GetColor(ThemeProperties::COLOR_CONTROL_BACKGROUND))); 4: Not ok, the following background will always be there. ContentsWebView::OnThemeChanged() set_background(views::Background::CreateSolidBackground(ThemeProperties::COLOR_NTP_BACKGROUND)) 5: Seems ok since the base background is set to (0, 0, 0, 0) at some point and if I skip 3 and 4 then I indeed see the non-owned FBO behind the browser (i.e. my desktop wallpaper :) is no other user window in between ) I also found the following piece of code interesting: GLRenderer::ClearFramebuffer if (frame->current_render_pass->has_transparent_background) gl_->ClearColor(0, 0, 0, 0); SoftwareRenderer::ClearFramebuffer if (frame->current_render_pass->has_transparent_background) current_canvas_->drawColor(SkColorSetARGB(0, 0, 0, 0), SkXfermode::kSrc_Mode) So those we can see that those 2 other entities also rely on a transparent_background flag. * Conclusion: So if I am not mistake there are 2 "layers" that go against my use case (that you described in your previous comment): 3 and 4. A draft solution for 4 would be to add: void ContentsWebView::OnPaintBackground(gfx::Canvas* canvas) { if (!GetWebContents()->GetRenderWidgetHostView()->GetBackgroundOpaque()) return; View::OnPaintBackground(canvas); } But at this point I do know if making non-fixed background for BrowserView and ContentsWebView is something acceptable or not. (non-fixed in the sense of the function OnPaintBackground just above where the goal is to make previous call to"set_background(views::Background::CreateSolidBackground(ThemeProperties::COLOR_NTP_BACKGROUND))" inactive) To come back to your question, "verify 1. The base background is set to (0,0,0,0), and 2. We draw our stuff with src-over instead of just overwrite with raw pixel value." I could only verify (0,0,0,0), but for src-over I assume it is actually the case because if I skip entities 3 and 4 then my use case is working. So unless the rough behavior of the ContentsWebView::OnPaintBackground function above is something you want, I think there is nothing to improve and I should just close the bug and this CL. Thx for your help.
On 2016/03/15 21:04:26, trchen wrote:
> For your application, I believe it is the case that we don't own the FBO. X11
> owns the FBO and will pass us with existing pixel from windows that are below
> us. In this case we actually want to make sure 1. The base background is set
to
> (0,0,0,0), and 2. We draw our stuff with src-over instead of just overwrite
with
> raw pixel value.
> My understanding could be wrong. Especially I suspect X11 behaves differently
> whether a compositing window manager presents. Please verify and confirm.
> Thanks!
Hi, please have a look to the "Patch Set 3" I just uploaded.
Especially please see the change in webview.cc::OnPaintBackground() and
desktop_window_tree_host_x11.cc::ShouldWindowContentsBeTransparent().
About ShouldUseNativeFrame, it is because the cc::Layer associated with the
blink::GraphicLayer::contentLayer() from
PaintLayerCompositor::rootGraphicsLayer() ("Frame Overflow Controls Host Layer")
was still having contents_opaque() returning true. Then its PictureLayerImpl was
rendering TileDrawQuad with opaque shader.
This layer is also the one returned by BrowserView::frame_->GetLayer(), so the
layer for window named "BrowserFrameAura".
But in the end the short cut is the fix I made in
DesktopWindowTreeHostX11::ShouldWindowContentsBeTransparent(), it makes it
consistent with DesktopWindowTreeHostX11::SetWindowTransparency().
TEST: ./out/Release/chrome --no-ua-dependent-background
--transparent-background-rwhv
The 2 switches are there one to disable the ua background and the other to
simulate the developer to have rwhv's background as transparent. Please see
previous comment for the explanation. Also note that the current CL description
does not match with "Patch Set 3".
Please let me know what you think about "Patch Set 3". Thx.
On 2016/03/23 17:32:20, j.isorce wrote: > Please let me know what you think about "Patch Set 3". Thx. I don't have the expertise to review code outside of cc/Blink. You can look at the git log and the OWNERS file to find appropriate reviewers.
On 2016/03/24 19:38:37, trchen wrote: > On 2016/03/23 17:32:20, j.isorce wrote: > > Please let me know what you think about "Patch Set 3". Thx. > > I don't have the expertise to review code outside of cc/Blink. You can look at > the git log and the OWNERS file to find appropriate reviewers. Sure, thx a lot anyway for all your comments, it was very helpful.
Description was changed from
==========
Decouple background color and transparency in RenderWidgetHostView
Currently it is not possible from the RenderWidgetHostView to set 0
as the background color and set it as opaque. Because the later
is computer from the former.
Whereas it is possible to control them indenpendently in RenderWidgetCompositor,
cc::LayerTreeHost and blink::WebView, see below:
RenderWidgetCompositor::setBackgroundColor
RenderWidgetCompositor::setHasTransparentBackground
cc::LayerTreeHost::set_background_color
cc::LayerTreeHost::set_has_transparent_background
blink::WebView::setBaseBackgroundColor
blink::WebView::setIsTransparent
So this CL add extra IPC ViewMsg_SetBackgroundColor for more granularity.
This CL also decouple background color and opacity in RenderWidgetHostView.
This CL allows to really have transparent background when 1 + 2:
1- Page is defined with <body style="background: transparent;"> which is the
default value.
2- One has called
content::RenderWidgetHostView->SetBackgroundColor(SK_ColorTRANSPARENT)
and content::RenderWidgetHostView->SetBackgroundOpaque(true)
Additionnal condition on Linux only:
3- Main window is 32-bit depth (On Linux it currently requires to pass
--enable-transparent-visuals)
R=trchen@chromium.org
BUG=589509
==========
to
==========
Allow to have a transparent UA dependent background.
R=
BUG=589509
==========
j.isorce@samsung.com changed reviewers: + sadrul@chromium.org
j.isorce@samsung.com changed reviewers: + jbauman@chromium.org
Description was changed from
==========
Allow to have a transparent UA dependent background.
R=
BUG=589509
==========
to
==========
Allow to have a transparent UA dependent background.
* --no-ua-dependent-background: avoid the call to
contents_container_->set_background in BrowserView::InitViews().
* --transparent-background-rwhv: testing option to manually call
RenderWidgetHostView->SetBackgroundColor(SK_ColorTRANSPARENT).
* WebView::OnPaintBackground: check if bbackground is set to be opaque before
drawing background setup from ContentsWebView::OnThemeChanged()
(CreateSolidBackground)
* DesktopWindowTreeHostX11::ShouldWindowContentsBeTransparent(): return
"use_argb_visual_;" from ) instead of always false.
It allows to have the non opaque layer for
PaintLayerCompositor::rootGraphicsLayer() ("Frame Overflow Controls Host
Layer"). It corresponds to BrowserView::frame_->GetLayer(). So the
layer for window named "BrowserFrameAura".
It also makes consistency with DesktopWindowTreeHostX11::SetWindowTransparency()
that uses "use_argb_visual_".
R=
BUG=589509
TEST: ./out/Release/chrome --enable-transparent-visuals
--no-ua-dependent-background
--transparent-background-rwhv
==========
j.isorce@samsung.com changed reviewers: + avi@chromium.org, jam@chromium.org, piman@chromium.org, sky@chromium.org
a few questions: 1) why are you changing src\chrome? Is your browser embedding at the content or chrome layer? chrome isn't really meant to be reused. If you're using that code, I imagine you already have a lot of patches so you can change it there. 2) why add kTransparentBackgroundRWHV instead of having delegate calls like the existing https://code.google.com/p/chromium/codesearch#chromium/src/content/public/bro... Generally that's how the embedder customizes these things, instead of a command line parameter.
On 2016/04/01 23:26:58, jam wrote: > a few questions: > 1) why are you changing src\chrome? Is your browser embedding at the content or > chrome layer? chrome isn't really meant to be reused. If you're using that code, > I imagine you already have a lot of patches so you can change it there. The changes in src/chrome are just there for the demo. Indeed I do not use them in our embedded app but it would allow to test the changes in content. I plan to make a browser test that would set --no-ua-dependent-background. Since the content of the browser will be fully transparent (it means I can see the desktop's wallpaper behind) I would need to setup another app behind and test the raw image content. Not sure how to do it for now. > 2) why add kTransparentBackgroundRWHV instead of having delegate calls like the > existing > https://code.google.com/p/chromium/codesearch#chromium/src/content/public/bro... > Generally that's how the embedder customizes these things, instead of a command > line parameter. Sure but the change https://codereview.chromium.org/1731373002/patch/40001/50004 is there so simulate a call to RenderWidgetHostView->SetBackgroundColor(SK_ColorTRANSPARENT) from the app that owns the chromium web view. Ideally this call would be done from the browser test I mentioned in 1). The point of browser test I would like to make is to test the following changes: a) https://codereview.chromium.org/1731373002/patch/40001/50009 b) https://codereview.chromium.org/1731373002/patch/40001/50007 For me a) is a fix of a real bug. See CL description. For b) it does not really fixes a bug but it allows ContentsWebView to take transparency of the layer into account in order to decode drawing or not its solid background. I provided more info in the CL description as well. Also I think it would give a good example for those who want to play with transparency. Indeed there are potentially many layers behind blink layers that needs to be transparent. I suggest 2 plans: p1: remove kTransparentBackgroundRWHV from this CL and make a browser test that sets --no-ua-dependent-background and calls RenderWidgetHostView->SetBackgroundColor(SK_ColorTRANSPARENT) somehow. p2: remove kTransparentBackgroundRWHV and kNoUADependentBackground, but keep fixes a) and/or b). And try to make a browser test but it won't be fully transparent, it will check that the last layer seen is the BrowserView's background (The one set by https://codereview.chromium.org/1731373002/patch/40001/50001) if a) + b) are kept. If only a) is kept then the test should check that the last layer seen is the ContentsWebView's solid background set in ContentsWebView::OnThemeChanged(). Because currently the last layer seen is the blink "Frame Overflow Controls Host Layer" (after calling RenderWidgetHostView->SetBackgroundColor(SK_ColorTRANSPARENT)) as described in the CL description. And I think it is a bug because this layer is set to be not opaque when calling RenderWidgetHostViewAura::SetBackgroundColor(SK_ColorTRANSPARENT). But somehow returning always false from DesktopWindowTreeHostX11::ShouldWindowContentsBeTransparent() makes this layer to be opaque back which is wrong.
I think the change https://codereview.chromium.org/1731373002/patch/40001/50009 fixes a real bug if I am not mistaken. Could anyone comment on this specific change ? Thx.
On 2016/04/28 17:53:32, Julien Isorce wrote: > I think the change https://codereview.chromium.org/1731373002/patch/40001/50009 > fixes a real bug if I am not mistaken. Could anyone comment on this specific > change ? Thx. That later change has landed in another CL of mine since then, and other parts of this CL are not a good to have in upstream so lets close it. Thx. |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
