Closed Bug 1297853 Opened 3 years ago Closed 3 years ago

White flash when creating a private browsing tab

Categories

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

51 Branch
Unspecified
Android
defect

Tracking

()

VERIFIED FIXED
Firefox 51
Tracking Status
firefox49 --- unaffected
firefox50 --- unaffected
firefox51 --- verified

People

(Reporter: rbarker, Assigned: rbarker)

References

Details

(Keywords: regression)

Attachments

(1 file, 1 obsolete file)

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.
Depends on: 1291373
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.
OS: Unspecified → Android
Priority: -- → P2
Version: unspecified → 51 Branch
(In reply to Kartikaya Gupta (email:kats@mozilla.com) 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.
Assignee: nobody → rbarker
Attachment #8787351 - Flags: review?(nchen)
Attachment #8787351 - Flags: review?(bugmail)
Blocks: 1299922
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
Attachment #8787351 - Flags: review?(nchen) → feedback+
(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.
(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.
Attachment #8787797 - Flags: review?(nchen)
Attachment #8787797 - Flags: review?(bugmail)
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
Attachment #8787797 - Flags: review?(nchen) → review+
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
Attachment #8787797 - Flags: review?(bugmail) → review+
Pushed by rbarker@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/a981c221c363
White flash when creating a private browsing tab r=jchen,kats
https://hg.mozilla.org/mozilla-central/rev/a981c221c363
Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 51
Verified as fixed using Samsung Note 5(6.0.1) on Nightly 51.0a1(2016-09-08).
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.