783066 - Gaia homescreen background isn't transparent when it's loaded out of process

Status: RESOLVED FIXED

Description

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Reproduces on desktop as well.  Might be a platform bug, but let's start off in b2g component.

Comment 1

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Vivien fixed or worked around this somehow in gaia, so not a blocker.

Comment 2

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

False alarm, still a problem.

Comment 3

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Vivien did you file a gaia issue for drawing the background in the homescreen itself?

Comment 4

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Epic bz fail!

Comment 5

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Vivien, do you have a gaia issue open for loading the background in the homescreen app itself yet?

Comment 6

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

https://github.com/mozilla-b2g/gaia/issues/3639

Comment 7

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

No longer a blocker since we have a better gaia fix.

Comment 8

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Etienne points out that this bug affects the dialer attention screen (and I guess all the other ones), for which there isn't a workaround.

Comment 9

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

The problem here seems to be that out-of-process content is picking up a default opaque white background from somewhere (CanvasFrame? I dunno).  However, in my simple tests, subdocument frames don't get a default background, they seem to be transparent by default.  Remote content is always a subdocument frame, so I think we should switch this to match same-process behavior.

I poked at nsCanvasFrame and nsViewportFrame but couldn't make them stop drawing a background.  roc/tn/mats, where should I be looking here?

Comment 10

[Timothy Nikkel (:tnikkel)]

It might be this line that is causing it
http://mxr.mozilla.org/mozilla-central/source/layout/base/nsPresShell.cpp#4931

Comment 11

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Ah!  Thanks for the pointer.

Comment 12

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

I see us getting to the ColorLayer optimization in FrameLayerBuilder for the nsDisplayCanvasBackground.  Its "mExtraBackgroundColor" is set to rgba(1,1,1,1).

Comment 13

[Timothy Nikkel (:tnikkel)]

mExtraBackgroundColor comes from only one place http://mxr.mozilla.org/mozilla-central/source/layout/base/nsPresShell.cpp#4835 which is called from the function right below it.

Comment 14

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

If I hack that code out, the white background goes away, but the root ContainerLayer still thinks it's opaque and I get a black background.

Comment 15

[Timothy Nikkel (:tnikkel)]

The layer probably is still getting the opaque flag set?

Depending on which code you commented out this is either because the code still thinks the layer is opaque but we just don't draw opaque or because we are setting the opaque flag from somewhere else.

Comment 16

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Created attachment 658233 [details] [diff] [review]
Hackery

Yeah, I mid-aired you, my comment was ambiguous in context.

Here's the hackery doo that gets us to no-white-background, but the root ContainerLayer still thinks it's opaque so we get a black background (unpainted CONTENT_COLOR surface, I would guess).

Comment 17

[Timothy Nikkel (:tnikkel)]

Ok that makes sense. Then I think we want to figure out where the opaque mExtraBackgroundColor is coming from and change that code to not do that for (only) the situation that is important here.

Comment 18

[Timothy Nikkel (:tnikkel)]

My quick look best guess is it is coming from PresShell::ComputeBackstopColor maybe?

Comment 19

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Forcing that to return rgba(0,0,0,0) in content processes doesn't change anything: opaque white background, ColorLayer optimization kicks in, and the root ContainerLayer thinks it's opaque content.

Comment 20

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Created attachment 658268 [details] [diff] [review]
Hackery v2

This hackery doo gets rid of the background.

Comment 21

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Making PuppetWidget pretend to be transparent doesn't change anything, but I guess that's expected.

Comment 22

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Created attachment 658310 [details] [diff] [review]
Paint the backgrounds of remote subdocuments like we paint those of same-process subdocuments

I don't know how to write a reftest for this in the current incarnation of the reftest harness.  We would need to allow reftests to set a non-default background color for the xul:window in the master process.

Comment 23

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 658310 [details] [diff] [review]
Paint the backgrounds of remote subdocuments like we paint those of same-process subdocuments

Review of attachment 658310 [details] [diff] [review]:
-----------------------------------------------------------------

::: layout/base/nsPresContext.cpp
@@ +2343,5 @@
> +bool
> +nsPresContext::IsCrossProcessRootContentDocument()
> +{
> +  return (XRE_GetProcessType() == GeckoProcessType_Default &&
> +          IsRootContentDocument());

Can you explain why this is correct? If we are a GeckoProcessType_Content, shouldn't this be able to return true in at least some cases?

Comment 24

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Oh, I misunderstood what "content" referred to in this code.  I see now that it means "not chrome".

You're right, we need to make this check more precise.  Will put up a v2 patch in a minute.

Comment 25

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Created attachment 658358 [details] [diff] [review]
Paint the backgrounds of remote subdocuments like we paint those of same-process subdocuments, v2

Comment 26

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 658358 [details] [diff] [review]
Paint the backgrounds of remote subdocuments like we paint those of same-process subdocuments, v2

Review of attachment 658358 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good, but why not just change IsRootContentDocument()? I don't think there's a good reason to have a function that's wrong in the cross-process case.

Comment 27

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

For uses like http://mxr.mozilla.org/mozilla-central/source/layout/base/nsDisplayList.cpp#1007, we need a concept of "local root" even if it's not the cross-process root.  So we either do this patch or another that creates IsLocalRoot().  I've already written this code, so I guess it's obvious which route I prefer ;).

Comment 28

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 658358 [details] [diff] [review]
Paint the backgrounds of remote subdocuments like we paint those of same-process subdocuments, v2

Review of attachment 658358 [details] [diff] [review]:
-----------------------------------------------------------------

OK.

Comment 29

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

https://hg.mozilla.org/integration/mozilla-inbound/rev/84fe2b3c5ed9

Comment 30

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/84fe2b3c5ed9