[content] Fix background color update handling in RWHVAura.

[content] Fix background color update handling in RWHVAura.

RenderWidgetHostViewAura uses the background color provided in the
CompositorFrame to update its own background color. This update should
not result in telling the RenderView to turn its background transparent.

BUG=689349
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation

Review-Url: https://codereview.chromium.org/2702153003
Cr-Commit-Position: refs/heads/master@{#462819}
Committed: https://chromium.googlesource.com/chromium/src/+/2b39ff29fd0735b45f37cd0f2d4997847695aecf

[Eric Seckler, 2017-02-20 15:22:39]

The CQ bit was checked by eseckler@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-02-20 15:22:50]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Eric Seckler, 2017-02-20 16:09:19]

eseckler@chromium.org changed reviewers:
+ dgozman@chromium.org

[Eric Seckler, 2017-02-20 16:09:19]

Dmitry, are you happy reviewing this, too?

cc-ing Dana and Chris, who have been changing this stuff last.

[commit-bot: I haz the power, 2017-02-20 16:16:27]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-02-20 16:16:28]

Dry run: This issue passed the CQ dry run.

[dgozman, 2017-02-21 19:44:31]

dgozman@chromium.org changed reviewers:
+ chrishtr@chromium.org

[dgozman, 2017-02-21 19:44:31]

I think Chris is a better reviewer here.

[danakj, 2017-02-21 19:51:50]

danakj@chromium.org changed reviewers:
+ danakj@chromium.org

[danakj, 2017-02-21 19:51:51]

I think you'd do better to keep track of a state of opaqueness here or
somewhere, that is based on the color as well as if we're showing a compositor
frame or the background, then forward that correct value on always.

[Eric Seckler, 2017-02-21 22:32:56]

On 2017/02/21 19:51:51, danakj wrote:
> I think you'd do better to keep track of a state of opaqueness here or
> somewhere, that is based on the color as well as if we're showing a compositor
> frame or the background, then forward that correct value on always.

Can you clarify what you mean by tracking the state of opaqueness? I was also
considering doing what the RWHVAndroid does - it applies the CompositorFrame
background only to an "internal" cached_background_color_, not to the actual
background_color_.

(The issue I'm trying to solve is that the background color of the
CompositorFrame (i.e. the WebView's background color) may be (semi-)transparent.
Currently, that causes a RWHVAura::SetBackgroundColor() and a
RWH::SetBackgroundOpaque(), which causes the background color of the next
CompositorFrame to be fully transparent. As a consequence, changing the
background color in the WebView later doesn't propagate back through the
CompositorFrame, because SetBackgroundOpaque() overrides the WebView's
background to be fully transparent.)

[danakj, 2017-02-21 22:54:27]

On 2017/02/21 22:32:56, Eric Seckler wrote:
> On 2017/02/21 19:51:51, danakj wrote:
> > I think you'd do better to keep track of a state of opaqueness here or
> > somewhere, that is based on the color as well as if we're showing a
compositor
> > frame or the background, then forward that correct value on always.
> 
> Can you clarify what you mean by tracking the state of opaqueness? I was also
> considering doing what the RWHVAndroid does - it applies the CompositorFrame
> background only to an "internal" cached_background_color_, not to the actual
> background_color_.
> 
> (The issue I'm trying to solve is that the background color of the
> CompositorFrame (i.e. the WebView's background color) may be
(semi-)transparent.
> Currently, that causes a RWHVAura::SetBackgroundColor() and a
> RWH::SetBackgroundOpaque(), which causes the background color of the next
> CompositorFrame to be fully transparent. As a consequence, changing the
> background color in the WebView later doesn't propagate back through the
> CompositorFrame, because SetBackgroundOpaque() overrides the WebView's
> background to be fully transparent.)

Hm, I think I didn't quite follow this. Can you maybe lay it out in a timeline
with states desired vs actual? I get that a renderer sending a frame (when u say
WebView you mean an OOPIF I think not an Android Webview right?) will change the
view's background color to match what the renderer sent. I am missing what's
going wrong after that.

What I meant to say above was basically.. there's two ways to change the
background color:
1. Externally via the public API. IIUC this is a source of absolute truth?
2. From the renderer sending a frame and using its background. IIUC this should
take lower priority than the former?

What's the defined behaviour you want? (2) if (1) has never been set, otherwise
(1)? Something else? Encode that behaviour in code and have both places update
appropriate state as needed to get your desired outcome to use.

[Eric Seckler, 2017-02-21 23:21:14]

On 2017/02/21 22:54:27, danakj wrote:
> On 2017/02/21 22:32:56, Eric Seckler wrote:
> > On 2017/02/21 19:51:51, danakj wrote:
> > > I think you'd do better to keep track of a state of opaqueness here or
> > > somewhere, that is based on the color as well as if we're showing a
> compositor
> > > frame or the background, then forward that correct value on always.
> > 
> > Can you clarify what you mean by tracking the state of opaqueness? I was
also
> > considering doing what the RWHVAndroid does - it applies the CompositorFrame
> > background only to an "internal" cached_background_color_, not to the actual
> > background_color_.
> > 
> > (The issue I'm trying to solve is that the background color of the
> > CompositorFrame (i.e. the WebView's background color) may be
> (semi-)transparent.
> > Currently, that causes a RWHVAura::SetBackgroundColor() and a
> > RWH::SetBackgroundOpaque(), which causes the background color of the next
> > CompositorFrame to be fully transparent. As a consequence, changing the
> > background color in the WebView later doesn't propagate back through the
> > CompositorFrame, because SetBackgroundOpaque() overrides the WebView's
> > background to be fully transparent.)
> 
> Hm, I think I didn't quite follow this. Can you maybe lay it out in a timeline
> with states desired vs actual? I get that a renderer sending a frame (when u
say
> WebView you mean an OOPIF I think not an Android Webview right?) will change
the
> view's background color to match what the renderer sent. I am missing what's
> going wrong after that.
I'm actually motivated by a headless use case, where we'd like to override the
default background color of the root frame's blink::WebViewImpl, via the
DevTools command Emulation.setDefaultBackgroundColorOverride. That works just
fine, as long as the overridden value is opaque. When it is transparent or
semi-transparent, things break.

That's because the RWHVAura interprets the color provided in the CompositorFrame
as input to decide whether ViewMsg_SetBackgroundOpaque is sent.

One possible sequence:
[Things go well]
RWHVAura initially #ffffffff, opaque=true
WebView with #ffff0000
CompositorFrame with #ffff0000
RWHVAura updates to #ffff0000, opaque=true
WebView changes to #ff0000ff
CompositorFrame with #ff0000ff
RWHVAura updates to #ff0000ff, opaque=true
[Things go bad]
WebView changes to #9900ff00
CompositorFrame with #9900ff00
RWHVAura updates to #9900ff00, opaque=false
ViewMsg_SetBackgroundOpaque overrides WebView to transparent
CompositorFrame with #00000000
RWHVAura updates to #00000000, opaque=false
[From hereon, WebView can't recover]
WebView changes to #ff0000ff, but is overridden to be transparent
CompositorFrame with #00000000
RWHVAura stays at #00000000, opaque=true

> What I meant to say above was basically.. there's two ways to change the
> background color:
> 1. Externally via the public API. IIUC this is a source of absolute truth?
> 2. From the renderer sending a frame and using its background. IIUC this
should
> take lower priority than the former?
> 
> What's the defined behaviour you want? (2) if (1) has never been set,
otherwise
> (1)? Something else? Encode that behaviour in code and have both places update
> appropriate state as needed to get your desired outcome to use.

I'm not entirely sure - I don't know who uses RWHVAura::SetBackgroundColor with
a transparent background (and why). I'm assuming that some users of a RWHV may
set that initially.

Generally, later color updates taken from the CompositorFrame override a
previously (explicitly) set color (and I don't want to change that). But this
doesn't happen if ViewMsg_SetBackgroundOpaque was once sent to force a
transparent background in the renderer. After that, the CompositorFrame
background is always fully transparent (until some RWHV user sets the background
explicitly to something else).

[danakj, 2017-02-22 17:19:53]

On 2017/02/21 23:21:14, Eric Seckler wrote:
> On 2017/02/21 22:54:27, danakj wrote:
> > On 2017/02/21 22:32:56, Eric Seckler wrote:
> > > On 2017/02/21 19:51:51, danakj wrote:
> > > > I think you'd do better to keep track of a state of opaqueness here or
> > > > somewhere, that is based on the color as well as if we're showing a
> > compositor
> > > > frame or the background, then forward that correct value on always.
> > > 
> > > Can you clarify what you mean by tracking the state of opaqueness? I was
> also
> > > considering doing what the RWHVAndroid does - it applies the
CompositorFrame
> > > background only to an "internal" cached_background_color_, not to the
actual
> > > background_color_.
> > > 
> > > (The issue I'm trying to solve is that the background color of the
> > > CompositorFrame (i.e. the WebView's background color) may be
> > (semi-)transparent.
> > > Currently, that causes a RWHVAura::SetBackgroundColor() and a
> > > RWH::SetBackgroundOpaque(), which causes the background color of the next
> > > CompositorFrame to be fully transparent. As a consequence, changing the
> > > background color in the WebView later doesn't propagate back through the
> > > CompositorFrame, because SetBackgroundOpaque() overrides the WebView's
> > > background to be fully transparent.)
> > 
> > Hm, I think I didn't quite follow this. Can you maybe lay it out in a
timeline
> > with states desired vs actual? I get that a renderer sending a frame (when u
> say
> > WebView you mean an OOPIF I think not an Android Webview right?) will change
> the
> > view's background color to match what the renderer sent. I am missing what's
> > going wrong after that.
> I'm actually motivated by a headless use case, where we'd like to override the
> default background color of the root frame's blink::WebViewImpl, via the
> DevTools command Emulation.setDefaultBackgroundColorOverride. That works just
> fine, as long as the overridden value is opaque. When it is transparent or
> semi-transparent, things break.
> 
> That's because the RWHVAura interprets the color provided in the
CompositorFrame
> as input to decide whether ViewMsg_SetBackgroundOpaque is sent.
> 
> One possible sequence:
> [Things go well]
> RWHVAura initially #ffffffff, opaque=true
> WebView with #ffff0000
> CompositorFrame with #ffff0000
> RWHVAura updates to #ffff0000, opaque=true
> WebView changes to #ff0000ff
> CompositorFrame with #ff0000ff
> RWHVAura updates to #ff0000ff, opaque=true
> [Things go bad]
> WebView changes to #9900ff00
> CompositorFrame with #9900ff00
> RWHVAura updates to #9900ff00, opaque=false
> ViewMsg_SetBackgroundOpaque overrides WebView to transparent
> CompositorFrame with #00000000
> RWHVAura updates to #00000000, opaque=false
> [From hereon, WebView can't recover]
> WebView changes to #ff0000ff, but is overridden to be transparent

Thanks for the details!

SetBackgroundOpaque is used by ChromeOS for webui pages that are meant to look
like part of the UI, such as the login screen. AFAIK there's been no use case to
support anything of than 00 and FF alpha channels thus far (which is why the
message is a bool now, it used to be a full color that encoded just a bool in
reality).

Some questions:
1) When WebView changes to #99 alpha, what do you want to happen? I assume
things work correctly up until SetBackgroundOpaque?
2) Do you know where the override from #99 to #00 happens? This looks bad.

It's a very strange feedback loop to have the renderer set a background color
causes the browser to tell the renderer what background color to use. It seems
like we really want two different things:
a) Outsider define background color of the view. This path seems to assume the
alpha is #ff or #00. We should DCHECK that along the way since that is all the
SetBackgroundOpaque message supports.
b) When the renderer changes the color we don't need to tell the renderer about
it. That looks like what you're doing in this patch.

So I think you're going in a good direction now that I understand more. But
instead of making it "Internal" can we be more explicit about it, like
"SetBackgroundColor()" which DCHECKs alpha 00 or FF (we should make this comment
more clear it /has/ to be fully transparent or opaque
https://cs.chromium.org/chromium/src/content/public/browser/render_widget_hos...),
and "UpdateBackgroundColorFromRenderer" or so? Then SetBackgroundColor tells the
renderer what color to use, the latter clearly doesn't.

We could perhaps have the public API call UpdateBackgroundColorFromRenderer()
with an explanation, the renderer will cause this to happen but we shortcut to
get the result immediately in the UI also.

Why isn't the #99 being overridden to #00 in the WebView case though, before
SetBackgroundOpaque happens? It seems like we're not sharing code that we could
be maybe in the renderer, or something is odd with that? This relates to
question 2 above.



> CompositorFrame with #00000000
> RWHVAura stays at #00000000, opaque=true
> 
> > What I meant to say above was basically.. there's two ways to change the
> > background color:
> > 1. Externally via the public API. IIUC this is a source of absolute truth?
> > 2. From the renderer sending a frame and using its background. IIUC this
> should
> > take lower priority than the former?
> > 
> > What's the defined behaviour you want? (2) if (1) has never been set,
> otherwise
> > (1)? Something else? Encode that behaviour in code and have both places
update
> > appropriate state as needed to get your desired outcome to use.
> 
> I'm not entirely sure - I don't know who uses RWHVAura::SetBackgroundColor
with
> a transparent background (and why). I'm assuming that some users of a RWHV may
> set that initially.
> 
> Generally, later color updates taken from the CompositorFrame override a
> previously (explicitly) set color (and I don't want to change that). But this
> doesn't happen if ViewMsg_SetBackgroundOpaque was once sent to force a
> transparent background in the renderer. After that, the CompositorFrame
> background is always fully transparent (until some RWHV user sets the
background
> explicitly to something else).

[Eric Seckler, 2017-02-22 19:52:13]

On 2017/02/22 17:19:53, danakj wrote:
> Thanks for the details!
> 
> SetBackgroundOpaque is used by ChromeOS for webui pages that are meant to look
> like part of the UI, such as the login screen. AFAIK there's been no use case
to
> support anything of than 00 and FF alpha channels thus far (which is why the
> message is a bool now, it used to be a full color that encoded just a bool in
> reality).
> 
> Some questions:
> 1) When WebView changes to #99 alpha, what do you want to happen? I assume
> things work correctly up until SetBackgroundOpaque?
Correct. All is good until it feeds back into the renderer.

> 2) Do you know where the override from #99 to #00 happens? This looks bad.
In blink::WebViewImpl::backgroundColor(), which returns transparent if
overridden by SetBackgroundOpaque/SetIsTransparent.
https://cs.chromium.org/chromium/src/third_party/WebKit/Source/web/WebViewImp...

(FYI, we use WebViewImpl::setBaseBackgroundColorOverride().)

Not sure if this is bad, sounds like it may be intended this way.. ?

> It's a very strange feedback loop to have the renderer set a background color
> causes the browser to tell the renderer what background color to use. It seems
> like we really want two different things:
> a) Outsider define background color of the view. This path seems to assume the
> alpha is #ff or #00. We should DCHECK that along the way since that is all the
> SetBackgroundOpaque message supports.
> b) When the renderer changes the color we don't need to tell the renderer
about
> it. That looks like what you're doing in this patch.
> 
> So I think you're going in a good direction now that I understand more. But
> instead of making it "Internal" can we be more explicit about it, like
> "SetBackgroundColor()" which DCHECKs alpha 00 or FF (we should make this
comment
> more clear it /has/ to be fully transparent or opaque
>
https://cs.chromium.org/chromium/src/content/public/browser/render_widget_hos...),
> and "UpdateBackgroundColorFromRenderer" or so? Then SetBackgroundColor tells
the
> renderer what color to use, the latter clearly doesn't.
> 
> We could perhaps have the public API call UpdateBackgroundColorFromRenderer()
> with an explanation, the renderer will cause this to happen but we shortcut to
> get the result immediately in the UI also.
This plan sounds good, yes.

> Why isn't the #99 being overridden to #00 in the WebView case though, before
> SetBackgroundOpaque happens? It seems like we're not sharing code that we
could
> be maybe in the renderer, or something is odd with that? This relates to
> question 2 above.
The scenario I was portraying assumes that the RWH's background was originally
opaque. In that case, WebViewImpl::isTransparent() is false, so the background
color isn't overridden there. Only after it receives the feed back from its
first update through RWHVAura and SetBackgroundOpaque, isTransparent() becomes
true and the #99 is overridden to #00.

I don't think that setBaseBackgroundColorOverride() is useful for
SetBackgroundOpaque / ChromeOS' use case, since the base background color is
only the "default" one, in case the page does not specify one. But maybe it
could use WebViewImpl::setBackgroundColorOverride(), which overrides the page's
background color, too. However, setBackgroundColorOverride() currently doesn't
support fully-transparent colors, because #00 alpha is its "override disabled"
value. We should probably change that anyway and support a transparent override
value there, too.

[Eric Seckler, 2017-02-23 19:01:09]

On 2017/02/22 19:52:13, Eric Seckler wrote:
> > It's a very strange feedback loop to have the renderer set a background
color
> > causes the browser to tell the renderer what background color to use. It
seems
> > like we really want two different things:
> > a) Outsider define background color of the view. This path seems to assume
the
> > alpha is #ff or #00. We should DCHECK that along the way since that is all
the
> > SetBackgroundOpaque message supports.
> > b) When the renderer changes the color we don't need to tell the renderer
> about
> > it. That looks like what you're doing in this patch.
> > 
> > So I think you're going in a good direction now that I understand more. But
> > instead of making it "Internal" can we be more explicit about it, like
> > "SetBackgroundColor()" which DCHECKs alpha 00 or FF (we should make this
> comment
> > more clear it /has/ to be fully transparent or opaque
> >
>
https://cs.chromium.org/chromium/src/content/public/browser/render_widget_hos...),
> > and "UpdateBackgroundColorFromRenderer" or so? Then SetBackgroundColor tells
> the
> > renderer what color to use, the latter clearly doesn't.
> > 
> > We could perhaps have the public API call
UpdateBackgroundColorFromRenderer()
> > with an explanation, the renderer will cause this to happen but we shortcut
to
> > get the result immediately in the UI also.
> This plan sounds good, yes.
Done.

Actually, there's still something weird happening with the WebView's background
color - it seems to be applied on two layers somehow. Which is fine if it's
opaque, but not so great if semi-transparent (alpha values multiply up). Will
have to figure out what that's about, but that's unrelated to this change. (It's
not the RWHV's window_->layer() that's re-applying it.)

[danakj, 2017-02-24 15:38:06]

On Wed, Feb 22, 2017 at 2:52 PM, <eseckler@chromium.org> wrote:

> On 2017/02/22 17:19:53, danakj wrote:
> > Thanks for the details!
> >
> > SetBackgroundOpaque is used by ChromeOS for webui pages that are meant
> to look
> > like part of the UI, such as the login screen. AFAIK there's been no use
> case
> to
> > support anything of than 00 and FF alpha channels thus far (which is why
> the
> > message is a bool now, it used to be a full color that encoded just a
> bool in
> > reality).
> >
> > Some questions:
> > 1) When WebView changes to #99 alpha, what do you want to happen? I
> assume
> > things work correctly up until SetBackgroundOpaque?
> Correct. All is good until it feeds back into the renderer.
>
> > 2) Do you know where the override from #99 to #00 happens? This looks
> bad.
> In blink::WebViewImpl::backgroundColor(), which returns transparent if
> overridden by SetBackgroundOpaque/SetIsTransparent.
> https://cs.chromium.org/chromium/src/third_party/
> WebKit/Source/web/WebViewImpl.cpp?q=blink::webviewimpl+
> package:%5Echromium$&l=2453
>
> (FYI, we use WebViewImpl::setBaseBackgroundColorOverride().)
>
> Not sure if this is bad, sounds like it may be intended this way.. ?
>

It sounds to me like two separate CLs tried to avoid or weren't aware of
each other and tried to build independent systems which have now collided.


>
>
> > It's a very strange feedback loop to have the renderer set a background
> color
> > causes the browser to tell the renderer what background color to use. It
> seems
> > like we really want two different things:
> > a) Outsider define background color of the view. This path seems to
> assume the
> > alpha is #ff or #00. We should DCHECK that along the way since that is
> all the
> > SetBackgroundOpaque message supports.
> > b) When the renderer changes the color we don't need to tell the renderer
> about
> > it. That looks like what you're doing in this patch.
> >
> > So I think you're going in a good direction now that I understand more.
> But
> > instead of making it "Internal" can we be more explicit about it, like
> > "SetBackgroundColor()" which DCHECKs alpha 00 or FF (we should make this
> comment
> > more clear it /has/ to be fully transparent or opaque
> >
> https://cs.chromium.org/chromium/src/content/public/
> browser/render_widget_host_view.h?rcl=43ac2e876ac8862c9dc65cdb9a74b7
> 49b14e62eb&l=131),
> > and "UpdateBackgroundColorFromRenderer" or so? Then SetBackgroundColor
> tells
> the
> > renderer what color to use, the latter clearly doesn't.
> >
> > We could perhaps have the public API call UpdateBackgroundColorFromRende
> rer()
> > with an explanation, the renderer will cause this to happen but we
> shortcut to
> > get the result immediately in the UI also.
> This plan sounds good, yes.
>
> > Why isn't the #99 being overridden to #00 in the WebView case though,
> before
> > SetBackgroundOpaque happens? It seems like we're not sharing code that we
> could
> > be maybe in the renderer, or something is odd with that? This relates to
> > question 2 above.
> The scenario I was portraying assumes that the RWH's background was
> originally
> opaque. In that case, WebViewImpl::isTransparent() is false, so the
> background
> color isn't overridden there. Only after it receives the feed back from its
> first update through RWHVAura and SetBackgroundOpaque, isTransparent()
> becomes
> true and the #99 is overridden to #00.
>
> I don't think that setBaseBackgroundColorOverride() is useful for
> SetBackgroundOpaque / ChromeOS' use case, since the base background color
> is
> only the "default" one, in case the page does not specify one. But maybe it
> could use WebViewImpl::setBackgroundColorOverride(), which overrides the
> page's
> background color, too. However, setBackgroundColorOverride() currently
> doesn't
> support fully-transparent colors, because #00 alpha is its "override
> disabled"
> value. We should probably change that anyway and support a transparent
> override
> value there, too.
>

It does sound like WebViewImpl should have one way to override, with a
single color. The IPC could enable it and set it to clear. And WebView
could enable it and set it to some other color.


>
> https://codereview.chromium.org/2702153003/
>

-- 
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[danakj, 2017-02-24 16:01:23]

https://codereview.chromium.org/2702153003/diff/20001/content/browser/rendere...
File content/browser/renderer_host/render_widget_host_view_aura.cc (right):

https://codereview.chromium.org/2702153003/diff/20001/content/browser/rendere...
content/browser/renderer_host/render_widget_host_view_aura.cc:757:
host_->SetBackgroundOpaque(GetBackgroundOpaque());
I notice that other RenderWidgetHostViews do similar as here.
https://cs.chromium.org/chromium/src/content/browser/renderer_host/render_wid...
etc..

Can we DCHECK that the color is fully opaque or transparent at each place that
does SetBackgroundOpaque? Actually, while we're at it, can we delete
SetBackgroundOpaque entirely and just check the color at each site? It doesn't
look like it needs to be part of the View API.

https://codereview.chromium.org/2702153003/diff/20001/content/browser/rendere...
content/browser/renderer_host/render_widget_host_view_aura.cc:766: if (color ==
cached_background_color_)
I think this cached_background_color_ will cause a problem. The view's
background_color() won't reflect it, so when we navigate and such, we'll see the
wrong color there and it will cause flashing.

It seems like UpdateBackgroundColorFromRenderer wants to call
Base::SetBackgroundColor, which is a bit weird maybe. Or maybe not?

[danakj, 2017-02-24 16:04:08]

https://codereview.chromium.org/2702153003/diff/20001/content/browser/rendere...
File content/browser/renderer_host/render_widget_host_view_aura.cc (right):

https://codereview.chromium.org/2702153003/diff/20001/content/browser/rendere...
content/browser/renderer_host/render_widget_host_view_aura.cc:766: if (color ==
cached_background_color_)
On 2017/02/24 16:01:23, danakj wrote:
> I think this cached_background_color_ will cause a problem. The view's
> background_color() won't reflect it, so when we navigate and such, we'll see
the
> wrong color there and it will cause flashing.
> 
> It seems like UpdateBackgroundColorFromRenderer wants to call
> Base::SetBackgroundColor, which is a bit weird maybe. Or maybe not?

Oh, and at that point, the comment in the base class is wrong, it does accept
partial alpha if it comes from the renderer... which makes me feel like the
inheritance pattern here is being problematic. Maybe each subclass should
implement BackgroundColor() and we drop the impl of SetBackgroundColor and
background_color() from the base class?

[Eric Seckler, 2017-02-27 12:45:38]

Description was changed from

==========
[content] Fix background color update handling in RWHVAura.

RenderWidgetHostViewAura uses the background color provided in the
CompositorFrame to update its own background color. This update should
not result in telling the RenderView to turn its background transparent.

BUG=689349
==========

to

==========
[content] Fix background color update handling in RWHVAura.

RenderWidgetHostViewAura uses the background color provided in the
CompositorFrame to update its own background color. This update should
not result in telling the RenderView to turn its background transparent.

BUG=689349
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========

[Eric Seckler, 2017-02-27 12:51:14]

Patchset #3 (id:40001) has been deleted

[Eric Seckler, 2017-02-27 12:51:20]

The CQ bit was checked by eseckler@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-02-27 12:51:35]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Eric Seckler, 2017-02-27 13:46:26]

PTAL :)

https://codereview.chromium.org/2702153003/diff/20001/content/browser/rendere...
File content/browser/renderer_host/render_widget_host_view_aura.cc (right):

https://codereview.chromium.org/2702153003/diff/20001/content/browser/rendere...
content/browser/renderer_host/render_widget_host_view_aura.cc:757:
host_->SetBackgroundOpaque(GetBackgroundOpaque());
On 2017/02/24 16:01:23, danakj wrote:
> I notice that other RenderWidgetHostViews do similar as here.
>
https://cs.chromium.org/chromium/src/content/browser/renderer_host/render_wid...
> etc..
> 
> Can we DCHECK that the color is fully opaque or transparent at each place that
> does SetBackgroundOpaque? Actually, while we're at it, can we delete
> SetBackgroundOpaque entirely and just check the color at each site? It doesn't
> look like it needs to be part of the View API.

Done.

https://codereview.chromium.org/2702153003/diff/20001/content/browser/rendere...
content/browser/renderer_host/render_widget_host_view_aura.cc:766: if (color ==
cached_background_color_)
On 2017/02/24 16:04:08, danakj wrote:
> On 2017/02/24 16:01:23, danakj wrote:
> > I think this cached_background_color_ will cause a problem. The view's
> > background_color() won't reflect it, so when we navigate and such, we'll see
> the
> > wrong color there and it will cause flashing.
> > 
> > It seems like UpdateBackgroundColorFromRenderer wants to call
> > Base::SetBackgroundColor, which is a bit weird maybe. Or maybe not?
> 
> Oh, and at that point, the comment in the base class is wrong, it does accept
> partial alpha if it comes from the renderer... which makes me feel like the
> inheritance pattern here is being problematic. Maybe each subclass should
> implement BackgroundColor() and we drop the impl of SetBackgroundColor and
> background_color() from the base class?

Got rid of the base class implementations and updated derivations.

[commit-bot: I haz the power, 2017-02-27 14:31:53]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-02-27 14:31:54]

Dry run: This issue passed the CQ dry run.

[Eric Seckler, 2017-02-27 15:51:08]

Patchset #4 (id:80001) has been deleted

[chrishtr, 2017-02-27 20:02:36]

Hi, back from vacation.

It's currently the case that Blink will never set a non-opaque background color
for a WebView.
I'm not sure why this should change with headless.

[chrishtr, 2017-02-27 20:03:15]

On 2017/02/27 at 20:02:36, chrishtr wrote:
> Hi, back from vacation.
> 
> It's currently the case that Blink will never set a non-opaque background
color for a WebView.
> I'm not sure why this should change with headless.

Sorry,  hit send too early. Meant to add: could you explain more about the use
case?

[Eric Seckler, 2017-02-27 20:11:37]

On 2017/02/27 20:03:15, chrishtr wrote:
> On 2017/02/27 at 20:02:36, chrishtr wrote:
> > Hi, back from vacation.
> > 
> > It's currently the case that Blink will never set a non-opaque background
> color for a WebView.
> > I'm not sure why this should change with headless.
> 
> Sorry,  hit send too early. Meant to add: could you explain more about the use
> case?

Headless users would like to set the "default" background color to a custom
value, including and in particular to transparent. If the web page specifies a
background color, this default color would be overridden by the web content. We
can only apply such a custom default background color setting within blink. At
the moment, we do this in WebViewImpl::SetBaseBackgroundColorOverride, via a
DevTools command.

[chrishtr, 2017-02-28 01:58:08]

On 2017/02/27 at 20:11:37, eseckler wrote:
> On 2017/02/27 20:03:15, chrishtr wrote:
> > On 2017/02/27 at 20:02:36, chrishtr wrote:
> > > Hi, back from vacation.
> > > 
> > > It's currently the case that Blink will never set a non-opaque background
> > color for a WebView.
> > > I'm not sure why this should change with headless.
> > 
> > Sorry,  hit send too early. Meant to add: could you explain more about the
use
> > case?
> 
> Headless users would like to set the "default" background color to a custom
value, including and in particular to transparent. If the web page specifies a
background color, this default color would be overridden by the web content. We
can only apply such a custom default background color setting within blink. At
the moment, we do this in WebViewImpl::SetBaseBackgroundColorOverride, via a
DevTools command.

Ok thanks.

[danakj, 2017-02-28 17:52:31]

LGTM

https://codereview.chromium.org/2702153003/diff/60001/content/browser/rendere...
File content/browser/renderer_host/render_widget_host_unittest.cc (right):

https://codereview.chromium.org/2702153003/diff/60001/content/browser/rendere...
content/browser/renderer_host/render_widget_host_unittest.cc:893:
EXPECT_EQ(static_cast<unsigned>(SK_AlphaTRANSPARENT),
You could just EXPECT_EQ(SK_ColorTRANSPARENT, view->background_color()).

And maybe EXPECT_NE it above, is simpler.

https://codereview.chromium.org/2702153003/diff/60001/content/browser/rendere...
File content/browser/renderer_host/render_widget_host_view_mac_unittest.mm
(right):

https://codereview.chromium.org/2702153003/diff/60001/content/browser/rendere...
content/browser/renderer_host/render_widget_host_view_mac_unittest.mm:1133:
EXPECT_EQ(static_cast<unsigned>(SK_AlphaTRANSPARENT),
can EXPECT_EQ(TRANSPARENT, background_color()) here too

https://codereview.chromium.org/2702153003/diff/60001/content/browser/rendere...
content/browser/renderer_host/render_widget_host_view_mac_unittest.mm:1148:
EXPECT_EQ(static_cast<unsigned>(SK_AlphaOPAQUE),
and EXPECT_EQ(WHITE, background_color()) if u like

https://codereview.chromium.org/2702153003/diff/60001/content/public/browser/...
File content/public/browser/render_widget_host_view.h (right):

https://codereview.chromium.org/2702153003/diff/60001/content/public/browser/...
content/public/browser/render_widget_host_view.h:135: virtual SkColor
background_color() const = 0;
Technically this breaks (already did) the style guide as virtuals should be
BackgroundColor() styled. But there's a billion call sites probably. If you're
adventurous it might be nice to fix it in another CL real quick.

[Eric Seckler, 2017-03-01 10:22:00]

The CQ bit was checked by eseckler@chromium.org to run a CQ dry run

[Eric Seckler, 2017-03-01 10:22:09]

https://codereview.chromium.org/2702153003/diff/60001/content/browser/rendere...
File content/browser/renderer_host/render_widget_host_unittest.cc (right):

https://codereview.chromium.org/2702153003/diff/60001/content/browser/rendere...
content/browser/renderer_host/render_widget_host_unittest.cc:893:
EXPECT_EQ(static_cast<unsigned>(SK_AlphaTRANSPARENT),
On 2017/02/28 17:52:31, danakj_OOO wrote:
> You could just EXPECT_EQ(SK_ColorTRANSPARENT, view->background_color()).
> 
> And maybe EXPECT_NE it above, is simpler.

Done.

https://codereview.chromium.org/2702153003/diff/60001/content/browser/rendere...
File content/browser/renderer_host/render_widget_host_view_mac_unittest.mm
(right):

https://codereview.chromium.org/2702153003/diff/60001/content/browser/rendere...
content/browser/renderer_host/render_widget_host_view_mac_unittest.mm:1133:
EXPECT_EQ(static_cast<unsigned>(SK_AlphaTRANSPARENT),
On 2017/02/28 17:52:31, danakj_OOO wrote:
> can EXPECT_EQ(TRANSPARENT, background_color()) here too

Done.

https://codereview.chromium.org/2702153003/diff/60001/content/browser/rendere...
content/browser/renderer_host/render_widget_host_view_mac_unittest.mm:1148:
EXPECT_EQ(static_cast<unsigned>(SK_AlphaOPAQUE),
On 2017/02/28 17:52:31, danakj_OOO wrote:
> and EXPECT_EQ(WHITE, background_color()) if u like

Done.

https://codereview.chromium.org/2702153003/diff/60001/content/public/browser/...
File content/public/browser/render_widget_host_view.h (right):

https://codereview.chromium.org/2702153003/diff/60001/content/public/browser/...
content/public/browser/render_widget_host_view.h:135: virtual SkColor
background_color() const = 0;
On 2017/02/28 17:52:31, danakj_OOO wrote:
> Technically this breaks (already did) the style guide as virtuals should be
> BackgroundColor() styled. But there's a billion call sites probably. If you're
> adventurous it might be nice to fix it in another CL real quick.

Yeah, I was considering doing that, but it seemed the effort required to find
all call sites on all platforms (afaik code search references only work on
linux?) was larger than the benefit -.-

Do we have a tool that can help with such a change, other than trial-and-error
on the different trybots?

[commit-bot: I haz the power, 2017-03-01 10:22:17]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Eric Seckler, 2017-03-01 10:32:49]

eseckler@chromium.org changed reviewers:
+ nasko@chromium.org

[Eric Seckler, 2017-03-01 10:32:50]

Ok, sounds like Dana is happy, but we still need a content owner :)

+nasko since it sounds like it's not quite Dmitry's field of expertise.

[commit-bot: I haz the power, 2017-03-01 10:36:00]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-03-01 10:36:00]

Dry run: Try jobs failed on following builders:
  linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[Eric Seckler, 2017-03-01 10:41:53]

The CQ bit was checked by eseckler@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-03-01 10:42:07]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-03-01 11:26:38]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-03-01 11:26:39]

Dry run: This issue passed the CQ dry run.

[danakj, 2017-03-01 17:05:15]

On Wed, Mar 1, 2017 at 5:22 AM, <eseckler@chromium.org> wrote:

> > Technically this breaks (already did) the style guide as virtuals
> should be
> > BackgroundColor() styled. But there's a billion call sites probably.
> If you're
> > adventurous it might be nice to fix it in another CL real quick.
>
> Yeah, I was considering doing that, but it seemed the effort required to
> find all call sites on all platforms (afaik code search references only
> work on linux?) was larger than the benefit -.-
>
> Do we have a tool that can help with such a change, other than
> trial-and-error on the different trybots?
>

Not that I know, no. You could write a matcher with a clang tool but it'd
probably be quicker for a single use-case to just do search/replace. You
could probably find 90%+ of them with some good regex.

(rwh|view).*(.|->)background_color()
f:/some/paths/ (.|->)background_color()
etc

-- 
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[nasko, 2017-03-01 18:24:52]

nasko@chromium.org changed reviewers:
+ lfg@chromium.org

[nasko, 2017-03-01 18:24:53]

Adding lfg@, who has dealt with background colors in the past and knows this
better than I do.

https://codereview.chromium.org/2702153003/diff/120001/content/browser/render...
File content/browser/renderer_host/render_widget_host_view_base.h (left):

https://codereview.chromium.org/2702153003/diff/120001/content/browser/render...
content/browser/renderer_host/render_widget_host_view_base.h:450: SkColor
background_color_;
Why remove it from the base class and have it added in each derived class?

[danakj, 2017-03-01 18:35:10]

https://codereview.chromium.org/2702153003/diff/120001/content/browser/render...
File content/browser/renderer_host/render_widget_host_view_base.h (left):

https://codereview.chromium.org/2702153003/diff/120001/content/browser/render...
content/browser/renderer_host/render_widget_host_view_base.h:450: SkColor
background_color_;
On 2017/03/01 18:24:53, nasko (slow) wrote:
> Why remove it from the base class and have it added in each derived class?

Because the subclass needs to change the value returned by background_color()
without calling SetBackgroundColor() which would have side effects.

[danakj, 2017-03-01 18:35:35]

https://codereview.chromium.org/2702153003/diff/120001/content/browser/render...
File content/browser/renderer_host/render_widget_host_view_base.h (left):

https://codereview.chromium.org/2702153003/diff/120001/content/browser/render...
content/browser/renderer_host/render_widget_host_view_base.h:450: SkColor
background_color_;
On 2017/03/01 18:35:10, danakj_OOO wrote:
> On 2017/03/01 18:24:53, nasko (slow) wrote:
> > Why remove it from the base class and have it added in each derived class?
> 
> Because the subclass needs to change the value returned by background_color()
> without calling SetBackgroundColor() which would have side effects.

https://codereview.chromium.org/2702153003/#msg18
https://codereview.chromium.org/2702153003/#msg19

[nasko, 2017-03-01 21:38:22]

On 2017/03/01 18:35:35, danakj wrote:
>
https://codereview.chromium.org/2702153003/diff/120001/content/browser/render...
> File content/browser/renderer_host/render_widget_host_view_base.h (left):
> 
>
https://codereview.chromium.org/2702153003/diff/120001/content/browser/render...
> content/browser/renderer_host/render_widget_host_view_base.h:450: SkColor
> background_color_;
> On 2017/03/01 18:35:10, danakj_OOO wrote:
> > On 2017/03/01 18:24:53, nasko (slow) wrote:
> > > Why remove it from the base class and have it added in each derived class?
> > 
> > Because the subclass needs to change the value returned by
background_color()
> > without calling SetBackgroundColor() which would have side effects.
> 
> https://codereview.chromium.org/2702153003/#msg18
> https://codereview.chromium.org/2702153003/#msg19

Thanks for the context! LGTM

[lfg, 2017-03-01 21:51:35]

I think there are multiple bugs here, but this isn't the right fix.

- As you noted, semi-transparent backgrounds don't work. This is because we
assume that transparent == !opaque, which isn't necessarily true. This should be
fixed in RenderViewImpl::OnSetBackgroundOpaque. The LayerTree adds the
background quads here:
https://cs.chromium.org/chromium/src/cc/trees/layer_tree_host_impl.cc?l=985 ,
based on the value of that flag.

- The main reason the WebViewImpl doesn't recover is that if you look at
WebViewImpl::backgroundColor(), it'll first check isTransparent(). This looks
very suspicious to me, and is indeed why your patch kind of works, because it
stops the feedback loop of setting isTransparent(). I think the right fix is to
get rid of isTransparent() and just use the {,base}BackgroundColorOverride
instead as the one source of overriding the background -- be it transparent or
opaque.

- After fixing this, we should be able to have the implementation in
RenderWidgetHostView* for setting the background color to be the same on all
platforms, and get rid of the feedback loop that dispatches a message to the
renderer about the transparent background.

[lfg, 2017-03-01 21:52:57]

not lgtm. Please address my comments before landing.

[Eric Seckler, 2017-03-01 22:15:43]

On 2017/03/01 21:51:35, lfg wrote:
> I think there are multiple bugs here, but this isn't the right fix.
> 
> - As you noted, semi-transparent backgrounds don't work. This is because we
> assume that transparent == !opaque, which isn't necessarily true. This should
be
> fixed in RenderViewImpl::OnSetBackgroundOpaque. The LayerTree adds the
> background quads here:
> https://cs.chromium.org/chromium/src/cc/trees/layer_tree_host_impl.cc?l=985 ,
> based on the value of that flag.
You're right, the blink-side needs some sort of fix as well. I was sketching a
possible solution here, but I'm not sure if that's all that's required either:
https://codereview.chromium.org/2715243004/

> - The main reason the WebViewImpl doesn't recover is that if you look at
> WebViewImpl::backgroundColor(), it'll first check isTransparent(). This looks
> very suspicious to me, and is indeed why your patch kind of works, because it
> stops the feedback loop of setting isTransparent(). I think the right fix is
to
> get rid of isTransparent() and just use the {,base}BackgroundColorOverride
> instead as the one source of overriding the background -- be it transparent or
> opaque.
Generally, this sounds reasonable, and I think this is what Dana was originally
proposing as well.

I discussed this with Dana briefly. The main difference between
WebViewImpl::setBackgroundColorOverride and WebViewImpl::SetIsTransparent seems
to be that SetIsTransparent also affects the background transparency of the
Frames within the WebViewImpl. To unify the two, would we set the transparency
bit of the frames based on the WebViewImpl's background color (overrides), and
if so how?

> - After fixing this, we should be able to have the implementation in
> RenderWidgetHostView* for setting the background color to be the same on all
> platforms, and get rid of the feedback loop that dispatches a message to the
> renderer about the transparent background.

Yay! :)

[lfg, 2017-03-01 22:48:27]

On 2017/03/01 22:15:43, Eric Seckler wrote:
> On 2017/03/01 21:51:35, lfg wrote:
> > I think there are multiple bugs here, but this isn't the right fix.
> > 
> > - As you noted, semi-transparent backgrounds don't work. This is because we
> > assume that transparent == !opaque, which isn't necessarily true. This
should
> be
> > fixed in RenderViewImpl::OnSetBackgroundOpaque. The LayerTree adds the
> > background quads here:
> > https://cs.chromium.org/chromium/src/cc/trees/layer_tree_host_impl.cc?l=985
,
> > based on the value of that flag.
> You're right, the blink-side needs some sort of fix as well. I was sketching a
> possible solution here, but I'm not sure if that's all that's required either:
> https://codereview.chromium.org/2715243004/

Ah, great, this is definitely in the right direction.

> > - The main reason the WebViewImpl doesn't recover is that if you look at
> > WebViewImpl::backgroundColor(), it'll first check isTransparent(). This
looks
> > very suspicious to me, and is indeed why your patch kind of works, because
it
> > stops the feedback loop of setting isTransparent(). I think the right fix is
> to
> > get rid of isTransparent() and just use the {,base}BackgroundColorOverride
> > instead as the one source of overriding the background -- be it transparent
or
> > opaque.
> Generally, this sounds reasonable, and I think this is what Dana was
originally
> proposing as well.
> 
> I discussed this with Dana briefly. The main difference between
> WebViewImpl::setBackgroundColorOverride and WebViewImpl::SetIsTransparent
seems
> to be that SetIsTransparent also affects the background transparency of the
> Frames within the WebViewImpl. To unify the two, would we set the transparency
> bit of the frames based on the WebViewImpl's background color (overrides), and
> if so how?

I assume you are talking about the bit in FrameView, that gets set here:
https://cs.chromium.org/chromium/src/third_party/WebKit/Source/web/WebLocalFr...
That bit only affects the main frame, it already feels like a hack and should be
cleaned up, but you don't need to do everything in one CL. I think for now it
would be reasonable to just use something like
alphachannel(WebViewImpl::backgroundColor()) == 0xff) instead of
isTransparent().

It's still important to support having the embedder overriding the transparency,
so in RenderViewImpl::OnSetBackgroundOpaque you'd have to call
WebFrameWidgetImpl::setBackgroundColorOverride(Color::transparent).

> > - After fixing this, we should be able to have the implementation in
> > RenderWidgetHostView* for setting the background color to be the same on all
> > platforms, and get rid of the feedback loop that dispatches a message to the
> > renderer about the transparent background.
> 
> Yay! :)

Thanks for fixing this!

[Eric Seckler, 2017-03-01 23:00:41]

On 2017/03/01 22:48:27, lfg wrote:
> On 2017/03/01 22:15:43, Eric Seckler wrote:
> > On 2017/03/01 21:51:35, lfg wrote:
> > > I think there are multiple bugs here, but this isn't the right fix.
> > > 
> > > - As you noted, semi-transparent backgrounds don't work. This is because
we
> > > assume that transparent == !opaque, which isn't necessarily true. This
> should
> > be
> > > fixed in RenderViewImpl::OnSetBackgroundOpaque. The LayerTree adds the
> > > background quads here:
> > >
https://cs.chromium.org/chromium/src/cc/trees/layer_tree_host_impl.cc?l=985
> ,
> > > based on the value of that flag.
> > You're right, the blink-side needs some sort of fix as well. I was sketching
a
> > possible solution here, but I'm not sure if that's all that's required
either:
> > https://codereview.chromium.org/2715243004/
> 
> Ah, great, this is definitely in the right direction.
> 
> > > - The main reason the WebViewImpl doesn't recover is that if you look at
> > > WebViewImpl::backgroundColor(), it'll first check isTransparent(). This
> looks
> > > very suspicious to me, and is indeed why your patch kind of works, because
> it
> > > stops the feedback loop of setting isTransparent(). I think the right fix
is
> > to
> > > get rid of isTransparent() and just use the {,base}BackgroundColorOverride
> > > instead as the one source of overriding the background -- be it
transparent
> or
> > > opaque.
> > Generally, this sounds reasonable, and I think this is what Dana was
> originally
> > proposing as well.
> > 
> > I discussed this with Dana briefly. The main difference between
> > WebViewImpl::setBackgroundColorOverride and WebViewImpl::SetIsTransparent
> seems
> > to be that SetIsTransparent also affects the background transparency of the
> > Frames within the WebViewImpl. To unify the two, would we set the
transparency
> > bit of the frames based on the WebViewImpl's background color (overrides),
and
> > if so how?
> 
> I assume you are talking about the bit in FrameView, that gets set here:
>
https://cs.chromium.org/chromium/src/third_party/WebKit/Source/web/WebLocalFr...
> That bit only affects the main frame, it already feels like a hack and should
be
> cleaned up, but you don't need to do everything in one CL. I think for now it
> would be reasonable to just use something like
> alphachannel(WebViewImpl::backgroundColor()) == 0xff) instead of
> isTransparent().
> 
> It's still important to support having the embedder overriding the
transparency,
> so in RenderViewImpl::OnSetBackgroundOpaque you'd have to call
> WebFrameWidgetImpl::setBackgroundColorOverride(Color::transparent).
Okay, I think I'm following, thanks!

I was actually looking at SetIsTransparent, where the same flag of the FrameView
is set as a consequence of OnSetBackgroundOpaque:
https://cs.chromium.org/chromium/src/third_party/WebKit/Source/web/WebViewImp...

But I think we'd handle that the same way.

> 
> > > - After fixing this, we should be able to have the implementation in
> > > RenderWidgetHostView* for setting the background color to be the same on
all
> > > platforms, and get rid of the feedback loop that dispatches a message to
the
> > > renderer about the transparent background.
> > 
> > Yay! :)
> 
> Thanks for fixing this!

[Eric Seckler, 2017-04-06 11:39:26]

lfg@: now that https://codereview.chromium.org/2715243004/ has landed, can we
revisit this?

Are you OK with landing this as-is?

Thanks!

[Eric Seckler, 2017-04-06 11:45:43]

The CQ bit was checked by eseckler@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-04-06 11:46:04]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-04-06 12:41:10]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-04-06 12:41:11]

Dry run: This issue passed the CQ dry run.

[lfg, 2017-04-06 18:37:41]

On 2017/04/06 11:39:26, Eric Seckler wrote:
> lfg@: now that https://codereview.chromium.org/2715243004/ has landed, can we
> revisit this?
> 
> Are you OK with landing this as-is?

lgtm with nit.

[lfg, 2017-04-06 18:37:59]

https://codereview.chromium.org/2702153003/diff/140001/content/browser/render...
File content/browser/renderer_host/render_widget_host_view_mac.mm (right):

https://codereview.chromium.org/2702153003/diff/140001/content/browser/render...
content/browser/renderer_host/render_widget_host_view_mac.mm:1438:
SetBackgroundColor(frame.metadata.root_background_color);
Let's apply the same fix here as you have done for Aura.

[Eric Seckler, 2017-04-07 08:25:14]

The CQ bit was checked by eseckler@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-04-07 08:25:29]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Eric Seckler, 2017-04-07 08:27:41]

Thanks!

https://codereview.chromium.org/2702153003/diff/140001/content/browser/render...
File content/browser/renderer_host/render_widget_host_view_mac.mm (right):

https://codereview.chromium.org/2702153003/diff/140001/content/browser/render...
content/browser/renderer_host/render_widget_host_view_mac.mm:1438:
SetBackgroundColor(frame.metadata.root_background_color);
On 2017/04/06 18:37:59, lfg wrote:
> Let's apply the same fix here as you have done for Aura.

Ah, didn't realize this was added to RWHVMac now, too. Done.

[commit-bot: I haz the power, 2017-04-07 09:17:34]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-04-07 09:17:35]

Dry run: This issue passed the CQ dry run.

[Eric Seckler, 2017-04-07 09:22:10]

The CQ bit was unchecked by eseckler@chromium.org

[Eric Seckler, 2017-04-07 09:22:11]

The CQ bit was checked by eseckler@chromium.org

[Eric Seckler, 2017-04-07 09:22:11]

The patchset sent to the CQ was uploaded after l-g-t-m from danakj@chromium.org,
nasko@chromium.org, lfg@chromium.org
Link to the patchset: https://codereview.chromium.org/2702153003/#ps160001
(title: "rebase: employ same mechanism in RWHVMac.")

[commit-bot: I haz the power, 2017-04-07 09:22:27]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-04-07 09:27:00]

CQ is committing da patch.

Bot data: {"patchset_id": 160001, "attempt_start_ts": 1491556931105100,
"parent_rev": "145d2c1c93f9f35b80ac22e6dd2cdfcf9cafffe5", "commit_rev":
"2b39ff29fd0735b45f37cd0f2d4997847695aecf"}

[commit-bot: I haz the power, 2017-04-07 09:27:45]

Description was changed from

==========
[content] Fix background color update handling in RWHVAura.

RenderWidgetHostViewAura uses the background color provided in the
CompositorFrame to update its own background color. This update should
not result in telling the RenderView to turn its background transparent.

BUG=689349
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========

to

==========
[content] Fix background color update handling in RWHVAura.

RenderWidgetHostViewAura uses the background color provided in the
CompositorFrame to update its own background color. This update should
not result in telling the RenderView to turn its background transparent.

BUG=689349
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation

Review-Url: https://codereview.chromium.org/2702153003
Cr-Commit-Position: refs/heads/master@{#462819}
Committed:
https://chromium.googlesource.com/chromium/src/+/2b39ff29fd0735b45f37cd0f2d49...
==========

[commit-bot: I haz the power, 2017-04-07 09:27:47]

Committed patchset #7 (id:160001) as
https://chromium.googlesource.com/chromium/src/+/2b39ff29fd0735b45f37cd0f2d49...