Mac: Remove white CALayers

Mac: Remove white CALayers

These cause white flashes on otherwise dark themes. Leave them
transparent, so that the theme color shows through.

There are comments in the code that, without these layers being white,
some parts of UI appear wrong. I haven't been able to find the
associated bugs, and haven't been able to reproduce this (with profile
switcher, extensions, etc).

Should bugs appear, where a different background color was expected, we
should change the embedding view to have the expected background color.

BUG=470669
Review-Url: https://codereview.chromium.org/2824613002
Cr-Commit-Position: refs/heads/master@{#464962}
Committed: https://chromium.googlesource.com/chromium/src/+/e1979f60524da9dab9d8cc19a23910f8d5dcf180

[ccameron, 2017-04-14 22:54:04]

ccameron@chromium.org changed reviewers:
+ avi@chromium.org

[ccameron, 2017-04-14 22:54:04]

This is likely to cause bugs ... but I'll go fix those bugs when we hit them
(after all, we branched yesterday, so we have all the time in the world).

[ccameron, 2017-04-14 23:04:14]

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

[commit-bot: I haz the power, 2017-04-14 23:04:46]

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-14 23:56:01]

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

[commit-bot: I haz the power, 2017-04-14 23:56:02]

Dry run: Try jobs failed on following builders:
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[Avi (use Gerrit), 2017-04-15 00:02:44]

RenderWidgetHostViewMacTest.Background is failing, and that sounds relevant.

[ccameron, 2017-04-15 20:38:53]

Description was changed from

==========
Mac: Remove white CALayers

These cause white flashes on otherwise dark themes. Leave them
transparent, so that the theme color shows through.

There are comments in the code that, without these layers being white,
some parts of UI appear wrong. I haven't been able to find the
associated bugs, and haven't been able to reproduce this (with profile
switcher, extensions, etc).

Should bugs appear, where a different background color was expected, we
should change the embedding view to have the expected background color.

BUG=470669
==========

to

==========
Mac: Remove white CALayers

These cause white flashes on otherwise dark themes. Leave them
transparent, so that the theme color shows through.

There are comments in the code that, without these layers being white,
some parts of UI appear wrong. I haven't been able to find the
associated bugs, and haven't been able to reproduce this (with profile
switcher, extensions, etc).

Should bugs appear, where a different background color was expected, we
should change the embedding view to have the expected background color.

BUG=470669
==========

[ccameron, 2017-04-15 20:38:53]

ccameron@chromium.org changed reviewers:
+ chrishtr@chromium.org, yusufo@chromium.org

[ccameron, 2017-04-15 20:54:19]

On 2017/04/15 00:02:44, Avi wrote:
> RenderWidgetHostViewMacTest.Background is failing, and that sounds relevant.

Updated the test to expect an initially transparent view.

I don't we have a coherent view of how background color is supposed to be
managed in RenderWidgetHostView and WebContentsView. I've added chrishtr and
yusufo to contribute to this discussion.

IMO, it should be the case that
1. RenderWidgetHostView should display
  - if we have web content, then the web content
  - if we had web content but paged it out, then a solid color of the web page's
background
  - if we have never had web content, then should be transparent, allowing
whatever is behind to show through

2. WebContentsView should display
  - maybe a solid color background, specified by the embedder of the WCV
  - probably should be transparent by default, maybe should always be
transparent letting the embedder of the WCV show through?

There are some confusing things here, though

3. RenderWidgetHostView has a "background color", and the meaning of this is
unclear. Is this supposed to
  - tell the RWHV what background color to draw when it has no content?
  - inform the RWHV of the background color so it can tell the renderer if the
background is opaque or not?
  - see https://codereview.chromium.org/2466413009 for some of the issues here

4. WebContentsImpl has a theme_color (see
https://codereview.chromium.org/975293004)
  - this originates in HTML at
https://cs.chromium.org/chromium/src/third_party/WebKit/Source/core/html/HTML...
  - how does this differ from web content background color? should we merge
them?
  - are we supposed to draw this at some point? or is this just informing us of
the "background color"? we only ever handle this on Android
  - i've never seen this IPC actually be invoked...

5. RenderWidgetHostImpl has a "is background opaque" method that is sent down
  - what should the initial state of this be?
  - what impact does this have on the renderer? is it an optimization?
  - is this the purpose for which we send background color to the
RenderWidgetHostView at all?

I'd love to land this patch just to see what happens, but I do get the feeling
that this area is snowballing incoherent-with-each-other one-off fixes, and
needs some cleanup. I'm happy to do the cleanup, but I'd like to get everyone on
the same page first (and I don't know what that page would be).

[Avi (use Gerrit), 2017-04-16 03:50:06]

On 2017/04/15 20:54:19, ccameron wrote:
> On 2017/04/15 00:02:44, Avi wrote:
> > RenderWidgetHostViewMacTest.Background is failing, and that sounds relevant.
> 
> Updated the test to expect an initially transparent view.
> 
> I don't we have a coherent view of how background color is supposed to be
> managed in RenderWidgetHostView and WebContentsView. I've added chrishtr and
> yusufo to contribute to this discussion.
> 
> IMO, it should be the case that
> 1. RenderWidgetHostView should display
>   - if we have web content, then the web content
>   - if we had web content but paged it out, then a solid color of the web
page's
> background
>   - if we have never had web content, then should be transparent, allowing
> whatever is behind to show through
> 
> 2. WebContentsView should display
>   - maybe a solid color background, specified by the embedder of the WCV
>   - probably should be transparent by default, maybe should always be
> transparent letting the embedder of the WCV show through?
> 
> There are some confusing things here, though
> 
> 3. RenderWidgetHostView has a "background color", and the meaning of this is
> unclear. Is this supposed to
>   - tell the RWHV what background color to draw when it has no content?
>   - inform the RWHV of the background color so it can tell the renderer if the
> background is opaque or not?
>   - see https://codereview.chromium.org/2466413009 for some of the issues here
> 
> 4. WebContentsImpl has a theme_color (see
> https://codereview.chromium.org/975293004)
>   - this originates in HTML at
>
https://cs.chromium.org/chromium/src/third_party/WebKit/Source/core/html/HTML...
>   - how does this differ from web content background color? should we merge
> them?
>   - are we supposed to draw this at some point? or is this just informing us
of
> the "background color"? we only ever handle this on Android
>   - i've never seen this IPC actually be invoked...
> 
> 5. RenderWidgetHostImpl has a "is background opaque" method that is sent down
>   - what should the initial state of this be?
>   - what impact does this have on the renderer? is it an optimization?
>   - is this the purpose for which we send background color to the
> RenderWidgetHostView at all?
> 
> I'd love to land this patch just to see what happens, but I do get the feeling
> that this area is snowballing incoherent-with-each-other one-off fixes, and
> needs some cleanup. I'm happy to do the cleanup, but I'd like to get everyone
on
> the same page first (and I don't know what that page would be).

I have no idea. You may need to dig and take ownership of this all.

[Avi (use Gerrit), 2017-04-16 03:50:41]

On 2017/04/15 20:54:19, ccameron wrote:
> On 2017/04/15 00:02:44, Avi wrote:
> > RenderWidgetHostViewMacTest.Background is failing, and that sounds relevant.
> 
> Updated the test to expect an initially transparent view.

Can you upload a new patchset?

[ccameron, 2017-04-16 07:47:05]

On 2017/04/16 03:50:41, Avi wrote:
> On 2017/04/15 20:54:19, ccameron wrote:
> > On 2017/04/15 00:02:44, Avi wrote:
> > > RenderWidgetHostViewMacTest.Background is failing, and that sounds
relevant.
> > 
> > Updated the test to expect an initially transparent view.
> 
> Can you upload a new patchset?

Oops -- uploaded.

[Avi (use Gerrit), 2017-04-16 22:26:02]

So this change LGTM, and clarification on the issues you raise would be great
too.

[ccameron, 2017-04-17 18:35:56]

On 2017/04/16 22:26:02, Avi (concussion NO REVIEWS) wrote:
> So this change LGTM, and clarification on the issues you raise would be great
> too.

Thanks. I'm sending this through, and following up on email about how to unify
things.

[ccameron, 2017-04-17 18:36:01]

The CQ bit was checked by ccameron@chromium.org

[commit-bot: I haz the power, 2017-04-17 18:36:40]

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-17 18:56:54]

CQ is committing da patch.

Bot data: {"patchset_id": 20001, "attempt_start_ts": 1492454161353350,
"parent_rev": "57d77a1786e9b7a6c7a86a6d8f9f0d914e317cd4", "commit_rev":
"e1979f60524da9dab9d8cc19a23910f8d5dcf180"}

[commit-bot: I haz the power, 2017-04-17 18:57:01]

Description was changed from

==========
Mac: Remove white CALayers

These cause white flashes on otherwise dark themes. Leave them
transparent, so that the theme color shows through.

There are comments in the code that, without these layers being white,
some parts of UI appear wrong. I haven't been able to find the
associated bugs, and haven't been able to reproduce this (with profile
switcher, extensions, etc).

Should bugs appear, where a different background color was expected, we
should change the embedding view to have the expected background color.

BUG=470669
==========

to

==========
Mac: Remove white CALayers

These cause white flashes on otherwise dark themes. Leave them
transparent, so that the theme color shows through.

There are comments in the code that, without these layers being white,
some parts of UI appear wrong. I haven't been able to find the
associated bugs, and haven't been able to reproduce this (with profile
switcher, extensions, etc).

Should bugs appear, where a different background color was expected, we
should change the embedding view to have the expected background color.

BUG=470669

Review-Url: https://codereview.chromium.org/2824613002
Cr-Commit-Position: refs/heads/master@{#464962}
Committed:
https://chromium.googlesource.com/chromium/src/+/e1979f60524da9dab9d8cc19a239...
==========

[commit-bot: I haz the power, 2017-04-17 18:57:02]

Committed patchset #2 (id:20001) as
https://chromium.googlesource.com/chromium/src/+/e1979f60524da9dab9d8cc19a239...