1297853 - White flash when creating a private browsing tab

Status: VERIFIED FIXED

(Firefox for Android Graveyard :: Toolbar, defect, P2)

Description

[Randall Barker [:rbarker]]

With the removal of JPZ, Java no longer is responsible for doing the gl clear. One side effect is that when private browsing tabs are created, there is a white flash since that is the default gl clear color in the compositor. It would be nice to get rid of the white flash by allowing the compositor clear color to be set from Java. The clear color would only be in effect util the page starts rendering and then it would be over ridden with the page's background color.

Comment 1

[Kartikaya Gupta (email:[e-mail])]

Is this a regression from the JPZ removal? Or from the switch to APZ? I though the JPZ removal was a no-op with APZ enabled.

Comment 2

[Randall Barker [:rbarker]]

(In reply to Kartikaya Gupta (email:[e-mail]) from comment #1)
> Is this a regression from the JPZ removal? Or from the switch to APZ? I
> though the JPZ removal was a no-op with APZ enabled.

It was my fault when I removed the JPZ gl clear code (which was still being used in APZ). I didn't realize the private tab was using that to override the clear color. So Bug 1291373 is when I introduced the regression, but it wasn't until working on Bug 1297850 that I realized what had happened.

Comment 3

[Kartikaya Gupta (email:[e-mail])]

Ok, thanks.

Comment 4

[Randall Barker [:rbarker]]

Attachment: 0001-Bug-1297853-White-flash-when-creating-a-private-browsing-tab-r-jchen-kats-16090112-ec86b20.patch

Comment 5

[(inactive) Jim Chen [:jchen] [:darchons]]

Comment on attachment 8787351 [details] [diff] [review]
0001-Bug-1297853-White-flash-when-creating-a-private-browsing-tab-r-jchen-kats-16090112-ec86b20.patch

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

Instead of keeping a clear color, can we just use the background color for the GeckoView/LayerVew? i.e. set the view background color from Tabs, and set the clear color to the view background color on pre-render.

::: mobile/android/base/java/org/mozilla/gecko/GeckoApp.java
@@ +2879,5 @@
>          // Use the chrome URI specified by Gecko's defaultChromeURI pref.
>          return null;
>      }
> +
> +    public LayerView getLayerView() {

Call it getGeckoView and return GeckoView.

::: mobile/android/base/java/org/mozilla/gecko/Tabs.java
@@ +76,5 @@
>      private static final AtomicInteger sTabId = new AtomicInteger(0);
>      private volatile boolean mInitialTabsAdded;
>  
>      private Context mAppContext;
> +    private GeckoApp mGeckoApp;

Store a reference to the LayerView instead of GeckoApp.

::: mobile/android/geckoview/src/main/java/org/mozilla/gecko/gfx/GeckoLayerClient.java
@@ +106,5 @@
>      private volatile boolean mContentDocumentIsDisplayed;
>  
>      private SynthesizedEventState mPointerState;
>  
> +    private int mClearColor = Color.WHITE;

I think this should be volatile.

@@ +696,5 @@
>      }
>  
> +    @WrapForJNI
> +    public int getClearColor() {
> +        return mClearColor;

I would just put @WrapForJNI(stubName = "ClearColor") on mClearColor itself.

::: widget/android/nsWindow.cpp
@@ +3503,5 @@
> +    if (NativePtr<LayerViewSupport>::Locked lvs{mLayerViewSupport}) {
> +        client = lvs->GetLayerClient();
> +    }
> +
> +    if (compositor) {

compositor && client

Comment 6

[Randall Barker [:rbarker]]

(In reply to Jim Chen [:jchen] [:darchons] from comment #5)
> Comment on attachment 8787351 [details] [diff] [review]
> 0001-Bug-1297853-White-flash-when-creating-a-private-browsing-tab-r-jchen-
> kats-16090112-ec86b20.patch
> 
> Review of attachment 8787351 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Instead of keeping a clear color, can we just use the background color for
> the GeckoView/LayerVew? i.e. set the view background color from Tabs, and
> set the clear color to the view background color on pre-render.
>

I'm not sure I understand. The LayerView background color does not stay the same and becomes transparent after first paint. So unless I'm completely missing what you are saying, the mClearColor does not always match the background color of the LayerView.

> ::: widget/android/nsWindow.cpp
> @@ +3503,5 @@
> > +    if (NativePtr<LayerViewSupport>::Locked lvs{mLayerViewSupport}) {
> > +        client = lvs->GetLayerClient();
> > +    }
> > +
> > +    if (compositor) {
> 
> compositor && client

Okay, the only reason I didn't do this is the function immediately after (nsWindow::DrawWindowUnderlay) used client without validating the pointer so I just mimicked it.

Comment 7

[Randall Barker [:rbarker]]

(In reply to Jim Chen [:jchen] [:darchons] from comment #5)
> Comment on attachment 8787351 [details] [diff] [review]
> 0001-Bug-1297853-White-flash-when-creating-a-private-browsing-tab-r-jchen-
> kats-16090112-ec86b20.patch
> 
> Review of attachment 8787351 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: mobile/android/base/java/org/mozilla/gecko/Tabs.java
> @@ +76,5 @@
> >      private static final AtomicInteger sTabId = new AtomicInteger(0);
> >      private volatile boolean mInitialTabsAdded;
> >  
> >      private Context mAppContext;
> > +    private GeckoApp mGeckoApp;
> 
> Store a reference to the LayerView instead of GeckoApp.
> 

I ran into a small problem doing this, when GeckoApp is passed into Tabs.attachToContext, GeckoApp.getGeckoView() returns null. So I have to keep a reference to GeckoApp so I can get the GeckoView later when it has been initialized. Unless I'm misunderstanding, I don't see a good way to just store a reference to the GeckoView as a LayerView.

Comment 8

[Randall Barker [:rbarker]]

Attachment: 0001-Bug-1297853-White-flash-when-creating-a-private-browsing-tab-r-jchen-kats-16090216-d678109.patch

Comment 9

[(inactive) Jim Chen [:jchen] [:darchons]]

Comment on attachment 8787797 [details] [diff] [review]
0001-Bug-1297853-White-flash-when-creating-a-private-browsing-tab-r-jchen-kats-16090216-d678109.patch

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

LGTM with nits.

(In reply to Randall Barker [:rbarker] from comment #6)
> (In reply to Jim Chen [:jchen] [:darchons] from comment #5)
> 
> I'm not sure I understand. The LayerView background color does not stay the
> same and becomes transparent after first paint. So unless I'm completely
> missing what you are saying, the mClearColor does not always match the
> background color of the LayerView.

Ah okay. I assumed we didn't use the LayerView background color (via View.setBackgroundColor) because the SurfaceView carves out a hole in the window, so we could use it for our purposes (but now looking at it, that'd be kind of painful in any case).

(In reply to Randall Barker [:rbarker] from comment #7)
> (In reply to Jim Chen [:jchen] [:darchons] from comment #5)
> 
> I ran into a small problem doing this, when GeckoApp is passed into
> Tabs.attachToContext, GeckoApp.getGeckoView() returns null. So I have to
> keep a reference to GeckoApp so I can get the GeckoView later when it has
> been initialized. Unless I'm misunderstanding, I don't see a good way to
> just store a reference to the GeckoView as a LayerView.

You should move the `Tabs.attachToContext()` call [1] to after we create the GeckoView [2]. That also lets you add a GeckoView parameter to Tabs.attachToContext, so that you just pass in the GeckoView and don't need to add `GeckoApp.getGeckoView()`.

[1] https://dxr.mozilla.org/mozilla-central/rev/8c9c4e816e86f903c1d820f3f29715dc070a5a4a/mobile/android/base/java/org/mozilla/gecko/GeckoApp.java#1157
[2] https://dxr.mozilla.org/mozilla-central/rev/8c9c4e816e86f903c1d820f3f29715dc070a5a4a/mobile/android/base/java/org/mozilla/gecko/GeckoApp.java#1268

::: mobile/android/geckoview/src/main/java/org/mozilla/gecko/gfx/GeckoLayerClient.java
@@ +906,5 @@
>          mDrawListeners.remove(listener);
>      }
> +
> +    public void setClearColor(int color) {
> +      mClearColor = color;

Four spaces

Comment 10

[Kartikaya Gupta (email:[e-mail])]

Comment on attachment 8787797 [details] [diff] [review]
0001-Bug-1297853-White-flash-when-creating-a-private-browsing-tab-r-jchen-kats-16090216-d678109.patch

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

::: mobile/android/base/java/org/mozilla/gecko/Tabs.java
@@ +81,3 @@
>      private ContentObserver mBookmarksContentObserver;
>      private PersistTabsRunnable mPersistTabsRunnable;
> +    private int mPrivateClearColor = Color.RED;

nit: I generally prefer initializing member variables in the constructor

Comment 11

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/a981c221c363
White flash when creating a private browsing tab r=jchen,kats

Comment 12

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/a981c221c363

Comment 13

[Alex Caziuc]

Verified as fixed using Samsung Note 5(6.0.1) on Nightly 51.0a1(2016-09-08).