Remove GLController.java

RESOLVED FIXED in Firefox 51

Status

()

Firefox for Android
Toolbar
P3
normal
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: droeh, Assigned: droeh)

Tracking

(Blocks: 1 bug)

50 Branch
Firefox 51
Unspecified
Android
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(fennec51+, firefox51 fixed)

Details

Attachments

(1 attachment, 6 obsolete attachments)

(Assignee)

Description

2 years ago
We need to kill off GLController.java and move code that's still necessary into LayerView.java.
(Assignee)

Comment 1

2 years ago
Created attachment 8773349 [details] [diff] [review]
Proposed patch

This kills GLController; it's working on my device at the moment, we'll see how the Try run goes.
Attachment #8773349 - Flags: review?(snorp)
(In reply to Dylan Roeh (:droeh) from comment #0)
> We need to 

Why do we need to do this? (Just looking for more context, I don't necessarily disagree)
(Assignee)

Comment 3

2 years ago
Well, need might be a bit strong. But the main purpose for splitting out the GLController code in the first place (as I understand it) was that we originally needed to create the EGL surface in java. With that moved to native code (bug 1136364), we already pretty much gutted GLController; what was left was little more than a wrapper around some native calls that mostly got used by LayerView. We can streamline a bit by getting rid of GLController and moving the functions to LayerView (in some cases merging them).
Ok, fair enough.
Comment on attachment 8773349 [details] [diff] [review]
Proposed patch

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

Looks pretty good to me! Let's also have jchen take a look.

::: mobile/android/base/java/org/mozilla/gecko/gfx/GeckoLayerClient.java
@@ +281,5 @@
>              Log.d(LOGTAG, "Window-size changed to " + mWindowSize);
>          }
>  
>          if (mView != null) {
> +            mView.getLayerViewNatives().onSizeChanged(mWindowSize.width, mWindowSize.height,

I think it's better to just add a package-scoped function to LayerView for this instead of getting the LayerViewNatives directly. Call it notifySizeChanged() or something (onFoo looks like an event handler from Android or something)

::: mobile/android/base/java/org/mozilla/gecko/gfx/LayerView.java
@@ +79,5 @@
> +     * is blocked on it) and read by the UI thread. */
> +    private volatile boolean mCompositorCreated;
> +
> +
> +    public class LayerViewNatives extends JNIObject {    

whitespace
Attachment #8773349 - Flags: review?(snorp)
Attachment #8773349 - Flags: review?(nchen)
Attachment #8773349 - Flags: review+
OS: Unspecified → Android
Priority: -- → P3
Version: unspecified → 50 Branch
(Assignee)

Comment 7

2 years ago
Created attachment 8773773 [details] [diff] [review]
Proposed patch (updated)

Updated to fix snorp's nits and to fix most of the failures on try. An updated try run has a few failures remaining, I'm looking into them at the moment:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=dbe2a60cf21a&selectedJob=24332333
Attachment #8773349 - Attachment is obsolete: true
Attachment #8773349 - Flags: review?(nchen)
Attachment #8773773 - Flags: review?(nchen)
Comment on attachment 8773773 [details] [diff] [review]
Proposed patch (updated)

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

::: mobile/android/base/java/org/mozilla/gecko/gfx/LayerView.java
@@ +79,5 @@
> +     * is blocked on it) and read by the UI thread. */
> +    private volatile boolean mCompositorCreated;
> +
> +
> +    public class LayerViewNatives extends JNIObject {

Make it package-private access and just call it Natives. You can also annotate it with '@WrapForJNI' to avoid annotation on each individual method.

@@ +513,5 @@
>          final NativePanZoomController npzc = AppConstants.MOZ_ANDROID_APZ ?
>                  (NativePanZoomController) mPanZoomController : null;
>  
>          if (GeckoThread.isStateAtLeast(GeckoThread.State.PROFILE_READY)) {
> +            mLayerViewNatives.attachToJava(mLayerClient, npzc);

Not sure why it's not failing, but you should be getting a NPE here, because you have not associated the new LayerViewNatives with its native object (LayerViewSupport) at this point yet.

::: widget/android/nsWindow.cpp
@@ -864,5 @@
>  
> -/**
> - * GLController has some unique requirements for its native calls, so make it
> - * separate from GeckoViewSupport.
> - */

Why did you delete this comment?

@@ +955,5 @@
>                  mozilla::Move(aCall)));
>      }
>  
> +    LayerViewSupport(nsWindow* aWindow,
> +                        const LayerView::LayerViewNatives::LocalRef& aInstance)

alignment
Attachment #8773773 - Flags: review?(nchen) → feedback+
(Assignee)

Comment 9

2 years ago
Created attachment 8774782 [details] [diff] [review]
Proposed patch (updated)

This addresses everything you recommended except changing the name of LayerViewNatives to Natives; I think that results in bad generated C++ code, as I don't think a class can have an inner class with the same name (LayerView::Natives::Natives). If I'm wrong on that or you have alternate suggestions I'm open to ideas, as LayerViewNatives is definitely a bit verbose.
Attachment #8773773 - Attachment is obsolete: true
Attachment #8774782 - Flags: review?(nchen)
Comment on attachment 8774782 [details] [diff] [review]
Proposed patch (updated)

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

(In reply to Dylan Roeh (:droeh) from comment #9)
> Created attachment 8774782 [details] [diff] [review]
> Proposed patch (updated)
> 
> This addresses everything you recommended except changing the name of
> LayerViewNatives to Natives; I think that results in bad generated C++ code,
> as I don't think a class can have an inner class with the same name
> (LayerView::Natives::Natives). If I'm wrong on that or you have alternate
> suggestions I'm open to ideas, as LayerViewNatives is definitely a bit
> verbose.

YOu can use "Compositor" or something.

::: mobile/android/base/java/org/mozilla/gecko/gfx/LayerView.java
@@ +82,5 @@
> +
> +
> +    @WrapForJNI
> +    protected class LayerViewNatives extends JNIObject {
> +        @WrapForJNI @Override protected native void disposeNative();

no @WrapForJNI

@@ +102,5 @@
> +        private native void syncResumeResizeCompositor(int width, int height);
> +
> +        private native void syncInvalidateAndScheduleComposite();
> +
> +        // DYLANTODO -- ??? @WrapForJNI(allowMultithread = true)

Specify allowMultithread in the class @WrapForJNI and remove this.

@@ +170,5 @@
>              mOverscroll = null;
>          }
>          Tabs.registerOnTabsChangedListener(this);
> +
> +        mLayerViewNatives = new LayerViewNatives();

This is still not quite right. When we recreate a LayerView and reattach it to an existing nsWindow, we used to preserve the GLController instance because the old instance is what is currently associated with the nsWindow (the association originally made in GeckoView.Window.open). The way we kept the GLController instance was to keep it inside GeckoView.Window. With LayerViewNatives, you can choose to do something similar. Either keep LayerViewNatives in GeckoView.Window or roll your own code to save/restore LayerViewNatives. You can also modify GeckoView.Widnow.reattach to reassociate the nsWindow with the new LayerViewNatives instance (probably the easiest way actually).

@@ +505,5 @@
>      Listener getListener() {
>          return mListener;
>      }
>  
> +    public void attachLayerViewNatives() {

private

@@ +519,5 @@
>                      NativePanZoomController.class, npzc);
>          }
>      }
>  
> +    public LayerViewNatives getLayerViewNatives() {

protected, package-private, or private
Attachment #8774782 - Flags: review?(nchen) → feedback+
(Assignee)

Comment 11

2 years ago
Created attachment 8775177 [details] [diff] [review]
Proposed patch (updated)

Alright, changes made. One thing I did slightly differently was to make the LayerViewNatives (now Compositor) a static member of GeckoView, rather than a member of GeckoView.Window; is there a reason why it should be in GeckoView.Window? I know that's what we did with GLController, but it seemed a bit odd to me.
Attachment #8774782 - Attachment is obsolete: true
Attachment #8775177 - Flags: review?(nchen)
Comment on attachment 8775177 [details] [diff] [review]
Proposed patch (updated)

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

(In reply to Dylan Roeh (:droeh) from comment #11)
> Created attachment 8775177 [details] [diff] [review]
> Proposed patch (updated)
> 
> Alright, changes made. One thing I did slightly differently was to make the
> LayerViewNatives (now Compositor) a static member of GeckoView, rather than
> a member of GeckoView.Window; is there a reason why it should be in
> GeckoView.Window? I know that's what we did with GLController, but it seemed
> a bit odd to me.

mCompositor cannot be static in GeckoView because each GeckoView/LayerView has a different compositor object. When you have multiple GeckoViews, a single static mCompositor will fail to work. So each GeckoView/LayerView needs to keep its own compositor instance, but because we want to preserve the compositor instance across GeckoView recreation, we stored it in GeckoView.Window.

But even if you keep mCompositor in GeckoView.Window, there are some problems with it. Because Compositor is now a child class of LayerView, it keeps a reference to the original LayerView that created it. So upon LayerView recreation, even if you restore the previous Compositor, the old Compositor still keeps a reference to the previous LayerView and not the new LayerView. That's going to break a whole bunch of things, not to mention leak the previous LayerView.

So like I said, it's probably easiest to change GeckoView.Window.reattach to detach the previous Compositor and attach a new Compositor to the existing nsWindow.
Attachment #8775177 - Flags: review?(nchen) → feedback+
(Assignee)

Comment 13

2 years ago
Created attachment 8775616 [details] [diff] [review]
Proposed patch (updated)

Alright, I see what you're saying. I went with changing GeckoView.Window.reattach(); hopefully this should do it.
Attachment #8775177 - Attachment is obsolete: true
Attachment #8775616 - Flags: review?(nchen)
Comment on attachment 8775616 [details] [diff] [review]
Proposed patch (updated)

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

Almost there!

::: mobile/android/base/java/org/mozilla/gecko/gfx/LayerView.java
@@ +95,5 @@
> +        /* package */ native void onSizeChanged(int windowWidth, int windowHeight,
> +                                                int screenWidth, int screenHeight);
> +
> +        // Gecko thread creates compositor; blocks UI thread.
> +        private native void createCompositor(int width, int height);

Change to package-private.

@@ +98,5 @@
> +        // Gecko thread creates compositor; blocks UI thread.
> +        private native void createCompositor(int width, int height);
> +
> +        // Gecko thread pauses compositor; blocks UI thread.
> +        private native void pauseCompositor();

Change to package-private, and rename to syncPauseCompositor

@@ +101,5 @@
> +        // Gecko thread pauses compositor; blocks UI thread.
> +        private native void pauseCompositor();
> +
> +        // UI thread resumes compositor and notifies Gecko thread; does not block UI thread.
> +        private native void syncResumeResizeCompositor(int width, int height);

Change to package-private.

@@ +103,5 @@
> +
> +        // UI thread resumes compositor and notifies Gecko thread; does not block UI thread.
> +        private native void syncResumeResizeCompositor(int width, int height);
> +
> +        private native void syncInvalidateAndScheduleComposite();

Change to package-private.

@@ +628,5 @@
>              mOverscroll.setSize(width, height);
>          }
>      }
>  
> +    public void notifySizeChanged(int windowWidth, int windowHeight, int screenWidth, int screenHeight) {

Change to package-private.

::: widget/android/nsWindow.cpp
@@ +883,4 @@
>      , public UsesGeckoThreadProxy
>  {
>      nsWindow& window;
> +    LayerView::Compositor::GlobalRef mLayerView;

mCompositor

@@ +933,2 @@
>  
>              // These calls are blocking.

This call is blocking

@@ +937,4 @@
>              return;
>  
> +        } else if (aCall.IsTarget(&LayerViewSupport::PauseCompositor)) {
> +            aCall.SetTarget(&LayerViewSupport::SyncPauseCompositor);

Remove this block.

@@ +955,5 @@
>                      mozilla::Move(aCall))));
>              return;
>  
>          } else if (aCall.IsTarget(
> +                &LayerViewSupport::SyncInvalidateAndScheduleComposite)) {

Add "|| aCall.IsTarget(&LayerViewSupport::SyncPauseCompositor)"

@@ +1226,5 @@
>      mEditable->OnViewChange(aView);
> +
> +    gGeckoViewWindow->mLayerViewSupport = mozilla::MakeUnique<LayerViewSupport>(
> +            gGeckoViewWindow, LayerView::Compositor::LocalRef(
> +            jni::GetGeckoThreadEnv(), LayerView::Compositor::Ref::From(aCompositor)));

Please don't use gGeckoViewWindow. It's doesn't work with multiple GeckoViews; use |window| instead.
Attachment #8775616 - Flags: review?(nchen) → feedback+
(Assignee)

Comment 15

2 years ago
Created attachment 8776139 [details] [diff] [review]
Proposed patch (updated)

OK, changes made. I also killed off nsWindow::GeckoViewSupport::PauseCompositor, as it's no longer used by anything (actually hasn't been since bug 1280594 I think). Let me know if that's not a good idea.
Attachment #8775616 - Attachment is obsolete: true
Attachment #8776139 - Flags: review?(nchen)
tracking-fennec: --- → ?
(Assignee)

Comment 16

2 years ago
Created attachment 8776624 [details] [diff] [review]
Proposed patch (updated)

Just happened to notice I was still using gGeckoViewWindow in one place where I shouldn't, otherwise it's the same as the previous patch.
Attachment #8776139 - Attachment is obsolete: true
Attachment #8776139 - Flags: review?(nchen)
Attachment #8776624 - Flags: review?(nchen)
Comment on attachment 8776624 [details] [diff] [review]
Proposed patch (updated)

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

::: widget/android/nsWindow.cpp
@@ +1210,5 @@
>      // Associate our previous GeckoEditable with the new GeckoView.
>      mEditable->OnViewChange(aView);
> +
> +    window.mLayerViewSupport = mozilla::MakeUnique<LayerViewSupport>(
> +            &window, LayerView::Compositor::LocalRef(jni::GetGeckoThreadEnv(), 

Extra space at the end.
Attachment #8776624 - Flags: review?(nchen) → review+

Comment 18

2 years ago
Pushed by droeh@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/38b0f9d15f84
This patch removes GLController.java entirely, moving necessary functionality into LayerView and a new Compositor class. r=jchen

Comment 19

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/38b0f9d15f84
Status: NEW → RESOLVED
Last Resolved: 2 years ago
status-firefox51: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 51
tracking-fennec: ? → 51+
Depends on: 1296757
You need to log in before you can comment on or make changes to this bug.