Closed Bug 1415994 Opened 7 years ago Closed 7 years ago

Introduce GeckoDisplay / LayerSession

Categories

(GeckoView :: General, enhancement)

Product:
GeckoView
Component:
General
Platform:
All
Android
Type:
enhancement
Priority:
Not set
Severity:
normal

Tracking

(firefox58 wontfix, firefox59 fixed)

Status:
RESOLVED FIXED
Milestone:
mozilla59
Tracking Flags:
Tracking Status
firefox58 --- wontfix
firefox59 --- fixed

People

(Reporter: jchen, Assigned: jchen)

References

Details

Attachments

(9 files)

Bug 1415994 - 1. Don't report screen size in GeckoLayerClient;
59 bytes, text/x-review-board-request
rbarker
: review+
Details
Bug 1415994 - 2. Introduce GeckoDisplay;
59 bytes, text/x-review-board-request
snorp
: review+
Details
Bug 1415994 - 3. Add LayerSession;
59 bytes, text/x-review-board-request
snorp
: review+
Details
Bug 1415994 - 4. Use LayerSession in native code;
59 bytes, text/x-review-board-request
snorp
: review+
Details
Bug 1415994 - 5. Use LayerSession from GeckoSession;
59 bytes, text/x-review-board-request
snorp
: review+
Details
Bug 1415994 - 6. Implement GeckoDisplay in GeckoView;
59 bytes, text/x-review-board-request
snorp
: review+
Details
Bug 1415994 - 7. Migrate existing gfx code to use LayerSession;
59 bytes, text/x-review-board-request
snorp
: review+
Details
Bug 1415994 - 8. Update auto-generated bindings;
59 bytes, text/x-review-board-request
jchen
: review+
Details
Bug 1415994 - 6b. Track GeckoDisplay origin changes;
59 bytes, text/x-review-board-request
snorp
: review+
Details 
This bug will introduce a GeckoDisplay interface and a LayerSession class to manage GeckoDisplay within a session. GeckoSession will inherit from LayerSession, and GeckoView will implement GeckoDisplay to be used with its session.

Using LayerSession will let us remove a significant portion of LayerView/GeckoLayerClient, but not all. Eventually, everything from LayerView/GeckoLayerClient will migrate to LayerSession, and LayerView/GeckoLayerClient will be removed.
Comment on attachment 8927029 [details]
Bug 1415994 - 1. Don't report screen size in GeckoLayerClient;

https://reviewboard.mozilla.org/r/198226/#review203544
Attachment #8927029 - Flags: review?(rbarker) → review+
Comment on attachment 8927031 [details]
Bug 1415994 - 3. Add LayerSession;

https://reviewboard.mozilla.org/r/198230/#review203742

::: mobile/android/geckoview/src/main/java/org/mozilla/gecko/gfx/LayerSession.java:16
(Diff revision 1)
> +
> +import android.content.Context;
> +import android.util.Log;
> +import android.view.Surface;
> +
> +public class LayerSession {

I think the only way this name makes sense is if you know it used to be called LayerView and that we then mostly renamed View to Session.

Shouldn't most (all?) of this be in GeckoSession?
Comment on attachment 8927034 [details]
Bug 1415994 - 6. Implement GeckoDisplay in GeckoView;

https://reviewboard.mozilla.org/r/198236/#review203744

::: mobile/android/geckoview/src/main/java/org/mozilla/gecko/GeckoView.java:72
(Diff revision 1)
>                  return new SavedState[size];
>              }
>          };
>      }
>  
> -    public GeckoView(Context context) {
> +    private class Display implements GeckoDisplay,

I think it would make things clearer to just make GeckoView implement GeckoDisplay. I don't think there is a good reason to hide it from consumers.

The SurfaceHolder.Callback is fine as an inner class, though, since that's more of an implementation detail.

::: mobile/android/geckoview/src/main/java/org/mozilla/gecko/GeckoView.java:99
(Diff revision 1)
> +
> +            // Tell new listener there is already a surface.
> +            if (GeckoView.this.mSurfaceView != null) {
> +                final SurfaceHolder holder = GeckoView.this.mSurfaceView.getHolder();
> +                final Rect frame = holder.getSurfaceFrame();
> +                listener.surfaceChanged(holder.getSurface(), frame.right, frame.bottom);

We can actually remove the width and height arguments here, as Gecko can get those itself. Not a  big deal, but something we could clean up later.

::: mobile/android/geckoview/src/main/java/org/mozilla/gecko/GeckoView.java:104
(Diff revision 1)
> +                listener.surfaceChanged(holder.getSurface(), frame.right, frame.bottom);
> +            }
> +        }
> +
> +        @Override // GeckoDisplay
> +        public boolean getOriginOnScreen(final int[] origin) {

What is this actually needed for? The origin could change at any time and we have nothing to notify Gecko of this. Does that matter?

::: mobile/android/geckoview/src/main/java/org/mozilla/gecko/GeckoView.java:114
(Diff revision 1)
> +            return false;
> +        }
> +
> +        @Override // SurfaceHolder.Callback
> +        public void surfaceCreated(final SurfaceHolder holder) {
> +            mValid = true;

It is actually not valid until we get the surfaceChanged() following a surfaceCreated()

::: mobile/android/geckoview/src/main/java/org/mozilla/gecko/GeckoView.java:119
(Diff revision 1)
> +            mValid = true;
> +        }
> +
> +        @Override // SurfaceHolder.Callback
> +        public void surfaceChanged(final SurfaceHolder holder, final int format,
> +                                   final int width, final int height) {

Set mValid here instead
Attachment #8927034 - Flags: review?(snorp) → review-
Comment on attachment 8927031 [details]
Bug 1415994 - 3. Add LayerSession;

https://reviewboard.mozilla.org/r/198230/#review203742

> I think the only way this name makes sense is if you know it used to be called LayerView and that we then mostly renamed View to Session.
> 
> Shouldn't most (all?) of this be in GeckoSession?

I think it's cleaner for graphics stuff to stay in o.m.g.gfx, because LayerSession/LayerSession.Compositor is used internally by other classes in o.m.g.gfx. If they go inside GeckoSession, we should think about moving other things out of .gfx as well.

I'm up for names other than LayerSession.
Depends on: 1416310
Depends on: 1416316
Depends on: 1416319
Comment on attachment 8927034 [details]
Bug 1415994 - 6. Implement GeckoDisplay in GeckoView;

https://reviewboard.mozilla.org/r/198236/#review203744

> I think it would make things clearer to just make GeckoView implement GeckoDisplay. I don't think there is a good reason to hide it from consumers.
> 
> The SurfaceHolder.Callback is fine as an inner class, though, since that's more of an implementation detail.

Maybe add GeckoView.getDisplay? I don't want GeckoDisplay to be thought of as part of the GeckoView API (e.g. GeckoView.setListener doesn't make much sense until you realize it's inherited from GeckoDisplay).

> We can actually remove the width and height arguments here, as Gecko can get those itself. Not a  big deal, but something we could clean up later.

I think we still need the dimensions for keeping track of nsWindow bounds.

> What is this actually needed for? The origin could change at any time and we have nothing to notify Gecko of this. Does that matter?

Right, I made it a callback in GeckoDisplay.Listener in another patch. We do need the origin for some stuff like calculating IME coordinates and web APIs like screenX/screenY.
Blocks: 1416310
No longer depends on: 1416310
Blocks: 1416316
No longer depends on: 1416316
Blocks: 1416319
No longer depends on: 1416319
Comment on attachment 8927409 [details]
Bug 1415994 - 6b. Track GeckoDisplay origin changes;

https://reviewboard.mozilla.org/r/198716/#review203824

::: mobile/android/geckoview/src/main/java/org/mozilla/gecko/GeckoView.java:76
(Diff revision 1)
>          };
>      }
>  
>      private class Display implements GeckoDisplay,
> -                                     SurfaceHolder.Callback {
> +                                     SurfaceHolder.Callback,
> +                                     ViewTreeObserver.OnGlobalLayoutListener {

You don't need to implement this interface, right? You can just name the onGlobalLayout() whatever you want.

Also I still think GeckoView should implement GeckoDisplay directly. It should be clear that a GeckoView is a GeckoDisplay.
Attachment #8927409 - Flags: review?(snorp) → review+
Comment on attachment 8927032 [details]
Bug 1415994 - 4. Use LayerSession in native code;

https://reviewboard.mozilla.org/r/198232/#review203830
Attachment #8927032 - Flags: review?(snorp) → review+
Comment on attachment 8927033 [details]
Bug 1415994 - 5. Use LayerSession from GeckoSession;

https://reviewboard.mozilla.org/r/198234/#review203832
Attachment #8927033 - Flags: review?(snorp) → review+
Comment on attachment 8927035 [details]
Bug 1415994 - 7. Migrate existing gfx code to use LayerSession;

https://reviewboard.mozilla.org/r/198238/#review203834
Attachment #8927035 - Flags: review?(snorp) → review+
Comment on attachment 8927034 [details]
Bug 1415994 - 6. Implement GeckoDisplay in GeckoView;

https://reviewboard.mozilla.org/r/198236/#review204164
Attachment #8927034 - Flags: review?(snorp) → review+
I'll land the existing patches now, and follow-up with,

* Merging LayerSession into GeckoSession (and potentially o.m.g.gfx into o.m.g).

* Figuring out the API for exposing GeckoView's display to other sessions.
Comment on attachment 8927036 [details]
Bug 1415994 - 8. Update auto-generated bindings;

https://reviewboard.mozilla.org/r/198240/#review204620
Attachment #8927036 - Flags: review+
Pushed by nchen@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/cd55753cf9ff
1. Don't report screen size in GeckoLayerClient; r=rbarker
https://hg.mozilla.org/integration/autoland/rev/85151d94c1b9
2. Introduce GeckoDisplay; r=snorp
https://hg.mozilla.org/integration/autoland/rev/f1bce5251893
3. Add LayerSession; r=snorp
https://hg.mozilla.org/integration/autoland/rev/3b24c9b8f391
4. Use LayerSession in native code; r=snorp
https://hg.mozilla.org/integration/autoland/rev/bb53058de7e9
5. Use LayerSession from GeckoSession; r=snorp
https://hg.mozilla.org/integration/autoland/rev/44d0e1e39f89
6. Implement GeckoDisplay in GeckoView; r=snorp
https://hg.mozilla.org/integration/autoland/rev/f0e9749cfce5
6b. Track GeckoDisplay origin changes; r=snorp
https://hg.mozilla.org/integration/autoland/rev/e90da05fbff4
7. Migrate existing gfx code to use LayerSession; r=snorp
https://hg.mozilla.org/integration/autoland/rev/a4cef03e1c9e
8. Update auto-generated bindings; r=jchen
Depends on: 1417490
status-firefox58: affected → wontfix
Target Milestone: Firefox 58 → Firefox 59
Product: Firefox for Android → GeckoView
Target Milestone: Firefox 59 → mozilla59
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: