Closed Bug 1321638 Opened 3 years ago Closed 3 years ago

[geckoview] Remove GeckoInterface.getActivity

Categories

(GeckoView :: General, defect)

defect
Not set

Tracking

(firefox53 fixed)

RESOLVED FIXED
mozilla53
Tracking Status
firefox53 --- fixed

People

(Reporter: nalexander, Assigned: nalexander)

References

Details

Attachments

(4 files)

It's true that each GeckoView is embedded in an Activity, but it's not the case that there is always an Activity when Gecko is running.

Removing getActivity isn't actually that difficult -- there are only a few consumers, and the main one -- GeckoScreenOrientation -- can use a system service for a bunch of it's work.  It can't, however, use a system service for `lock` and `unlock` (see https://dxr.mozilla.org/mozilla-central/source/mobile/android/geckoview/src/main/java/org/mozilla/gecko/GeckoScreenOrientation.java#242).  I don't think those functions really make sense for GeckoView consumers, to be honest, but they're implementing the Web Screen Orientation API so I don't want to neuter them for Fennec.

This ticket tracks unraveling whatever should happen here so that a non-Fennec GeckoView consumer can run without manually managing the current Activity.
Usually I'd just push the functions into GeckoInterface and move on with my life, but jchen's Android Onboarding (Platform Topics) notes explicitly ask not to do that: https://docs.google.com/presentation/d/1u9KtHjio34BOagGaDNqA7lWhTY5PEpMQ-ztl_oLn-48/edit#slide=id.g194a71d8b9_1_89.

So, can we sketch what needs to happen in this case?  I feel like this really is a GeckoInterface thing, since it's not specific to one GeckoView.  That is, we can't always connect the Android HAL layer to a particular GeckoView that executes the JS that requests the screen orientation, since it might be a global module or similar that does it.

jchen, can you outline how this should happen?  Can we just push into GeckoInterface for now?
No longer blocks: 1320587
Flags: needinfo?(nchen)
I think we can still move it out of GeckoInterface.

We could add another interface (e.g. ScreenOrientationDelegate + GeckoAppShell.setScreenOrientationDelegate + getScreenOrientationDelegate), and let GeckoApp (and whatever GV consumer) implement that. ScreenOrientationDelegate in this case would have lock and unlock methods.

We could also track the active activity ourselves (outside of GeckoInterface) using ActivityLifecycleCallbacks.
Flags: needinfo?(nchen)
(In reply to Jim Chen [:jchen] [:darchons] from comment #2)
> I think we can still move it out of GeckoInterface.
> 
> We could add another interface (e.g. ScreenOrientationDelegate +
> GeckoAppShell.setScreenOrientationDelegate + getScreenOrientationDelegate),
> and let GeckoApp (and whatever GV consumer) implement that.
> ScreenOrientationDelegate in this case would have lock and unlock methods.

OK.  I don't see this as significantly different or better than sticking them in GeckoInterface, and it adds  yet another piece of state that needs to be tracked and managed.  (Fun question: what happens if any of these interfaces change when Gecko is running?)  But I'm willing to play with it.

> We could also track the active activity ourselves (outside of
> GeckoInterface) using ActivityLifecycleCallbacks.

Sadly, that's API 14+, and requires a consumer to either extend from a GeckoViewApplication or to notify us of events.  Both are no better than the existing alternatives :(
> > We could also track the active activity ourselves (outside of
> > GeckoInterface) using ActivityLifecycleCallbacks.
> 
> Sadly, that's API 14+, and requires a consumer to either extend from a
> GeckoViewApplication or to notify us of events.  Both are no better than the
> existing alternatives :(

14+ remains, but I am incorrect: we could, in setApplicationContext, register a lifecycle callback as part of GeckoView.  It's not a bad idea, and were we thinking of Fennec only I would choose this path; but I think it will be bad to maintain the delusion that Gecko can always access an Activity context -- we should "do it right" and only access a context through an actual GeckoView.
Comment on attachment 8816640 [details]
Bug 1321638 - Part 1: Remove "app.orientation.default".

https://reviewboard.mozilla.org/r/97306/#review97690
Attachment #8816640 - Flags: review?(nchen) → review+
Comment on attachment 8816641 [details]
Bug 1321638 - Part 2: Remove fullscreen support for GeckoView consumers.

https://reviewboard.mozilla.org/r/97308/#review97692
Attachment #8816641 - Flags: review?(nchen) → review+
Comment on attachment 8816642 [details]
Bug 1321638 - Part 3: Add ScreenOrientationDelegate.

https://reviewboard.mozilla.org/r/97310/#review97694

For lock and unlock, I think I'd rather have GeckoAppShell.lock => GeckoScreenOrientation.lock => ScreenOrientationDelegate.lock, rather than GeckoAppShell => ScreenOrientationDelegate => GeckoScreenOrientation. What do you think?

::: mobile/android/geckoview/src/main/java/org/mozilla/gecko/GeckoAppShell.java:1829
(Diff revision 1)
> +     * A delegate for supporting the Screen Orientation API.
> +     * <p>
> +     * A null delegate is explicitly accepted; most non-browser GeckoView consumers will not
> +     * support locking and unlocking the screen orientation.
> +     */
> +    private static @Nullable  ScreenOrientationDelegate sScreenOrientationDelegate;

For consistency with the other listener/delegate types in GeckoAppShell, maybe implement a default `SceenOrientationDelegate` in `GeckoAppShell$DefaultListeners`?

::: mobile/android/geckoview/src/main/java/org/mozilla/gecko/GeckoAppShell.java:2087
(Diff revision 1)
>      private static void enableScreenOrientationNotifications() {
>          GeckoScreenOrientation.getInstance().enableNotifications();
>      }
>  
>      @WrapForJNI(calledFrom = "gecko")
>      private static void disableScreenOrientationNotifications() {

We should move these static screen orientation methods from `GeckoAppShell` to `GeckoScreenOrientation` itself
Comment on attachment 8816643 [details]
Bug 1321638 - Part 4: Remove last GeckoInterface.getActivity call in geckoview.

https://reviewboard.mozilla.org/r/97312/#review97696

Hm, I'm not even sure minimizing a top-level window makes sense for Fennec (we don't support maximuzing). Maybe we could just get rid of moveTaskToBack?
Comment on attachment 8816642 [details]
Bug 1321638 - Part 3: Add ScreenOrientationDelegate.

https://reviewboard.mozilla.org/r/97310/#review97694

This implies that `GeckoScreenOrientation` has some reference to the delegate.  Earlier, you wanted to split the delegate out of `GeckoInterface`, and now you want to push the delegates down to the leaves of the tree.  You clearly have some vision that is different than mine.  I have generally found that having one "controller" object with a bunch of configuration that then pushes the bits down the tree is better than having lots and lots of configuration in the leaves.  When multiple leaves need to access the same delegate, you now have a sharing and ownership problem which you wouldn't have when the configuration is higher up the tree...

If `GeckoScreenOrientation` wasn't a static singleton, but was instead a member of the root `GeckoInterface`, your approach would make sense to me.

> For consistency with the other listener/delegate types in GeckoAppShell, maybe implement a default `SceenOrientationDelegate` in `GeckoAppShell$DefaultListeners`?

Sure, I'll do this.

> We should move these static screen orientation methods from `GeckoAppShell` to `GeckoScreenOrientation` itself

I am happy to do this, but in general I really want the platform team to make the compiled code changes.  Every one is a pain in the ass for me, breaking me out of my artifact mode flow.  (Although right now https://bugzilla.mozilla.org/show_bug.cgi?id=1318820 is completely destroying my artifact mode flow anyway, boo urns.)
Comment on attachment 8816643 [details]
Bug 1321638 - Part 4: Remove last GeckoInterface.getActivity call in geckoview.

https://reviewboard.mozilla.org/r/97312/#review97696

I didn't want to change the compiled code.  What would make sense is to untangle "minimize/maximize" from "fullscreen" in the `nsWindow` implementation, and expose both to the underlying GeckoView.  See my commit comment where I say I don't want to do this :)  Can doing better here be follow-up?
(In reply to Nick Alexander :nalexander (leave until January 2017) from comment #14)
> Comment on attachment 8816642 [details]
> Bug 1321638 - Part 3: Add ScreenOrientationDelegate.
> 
> https://reviewboard.mozilla.org/r/97310/#review97694
> 
> This implies that `GeckoScreenOrientation` has some reference to the
> delegate.  Earlier, you wanted to split the delegate out of
> `GeckoInterface`, and now you want to push the delegates down to the leaves
> of the tree.  You clearly have some vision that is different than mine.  I
> have generally found that having one "controller" object with a bunch of
> configuration that then pushes the bits down the tree is better than having
> lots and lots of configuration in the leaves.  When multiple leaves need to
> access the same delegate, you now have a sharing and ownership problem which
> you wouldn't have when the configuration is higher up the tree...
> 
> If `GeckoScreenOrientation` wasn't a static singleton, but was instead a
> member of the root `GeckoInterface`, your approach would make sense to me.

Can you clarify what you mean by "configuration" here?

What I was saying is, in `GeckoScreenOrientation.setRequestedOrientation`, instead of,

    Activity activity = getActivity();
    // ...
    activity.setRequestedOrientation(activityOrientation);

you would do,

    ScreenOrientationDelegate delegate = GeckoAppShell.getScreenOrientationDelegate();
    // ...
    delegate.setScreenOrientation(orientation);

That gets rid of the `getActivity` call without too much fuss, and doesn't make `GeckoScreenOrientation.lock` and `GeckoScreenOrientation.unlock` into public APIs that we have to maintain (because in your patch, every implementer of ScreenOrientationDelegate has to call `GeckoScreenOrientation.lock` and `GeckoScreenOrientation.unlock`).

> > We should move these static screen orientation methods from `GeckoAppShell` to `GeckoScreenOrientation` itself
> 
> I am happy to do this, but in general I really want the platform team to
> make the compiled code changes.  Every one is a pain in the ass for me,
> breaking me out of my artifact mode flow.  (Although right now
> https://bugzilla.mozilla.org/show_bug.cgi?id=1318820 is completely
> destroying my artifact mode flow anyway, boo urns.)

File a follow-up to do this?
(In reply to Nick Alexander :nalexander (leave until January 2017) from comment #15)
> Comment on attachment 8816643 [details]
> Bug 1321638 - Part 4: Remove last GeckoInterface.getActivity call in
> geckoview.
> 
> https://reviewboard.mozilla.org/r/97312/#review97696
> 
> I didn't want to change the compiled code.  What would make sense is to
> untangle "minimize/maximize" from "fullscreen" in the `nsWindow`
> implementation, and expose both to the underlying GeckoView.  See my commit
> comment where I say I don't want to do this :)  Can doing better here be
> follow-up?

Follow-up sounds good. For now, I think it'd be okay to make GeckoAppShell.moveTaskToBack an empty method.
(In reply to Jim Chen [:jchen] [:darchons] from comment #16)
> (In reply to Nick Alexander :nalexander (leave until January 2017) from
> comment #14)
> > Comment on attachment 8816642 [details]
> > Bug 1321638 - Part 3: Add ScreenOrientationDelegate.
> > 
> > https://reviewboard.mozilla.org/r/97310/#review97694
> > 
> > This implies that `GeckoScreenOrientation` has some reference to the
> > delegate.  Earlier, you wanted to split the delegate out of
> > `GeckoInterface`, and now you want to push the delegates down to the leaves
> > of the tree.  You clearly have some vision that is different than mine.  I
> > have generally found that having one "controller" object with a bunch of
> > configuration that then pushes the bits down the tree is better than having
> > lots and lots of configuration in the leaves.  When multiple leaves need to
> > access the same delegate, you now have a sharing and ownership problem which
> > you wouldn't have when the configuration is higher up the tree...
> > 
> > If `GeckoScreenOrientation` wasn't a static singleton, but was instead a
> > member of the root `GeckoInterface`, your approach would make sense to me.
> 
> Can you clarify what you mean by "configuration" here?

I just meant "an extension point', like a delegate or listener.

In general I think this code is way too permissive about setting and changing delegates.  I'd be much happier if we had a bunch of final members; it's going to cause bugs when we start tracking state across delegate changes.  But at least your suggestion keeps the orientation listener in the central location (`GeckoAppShell`).

> What I was saying is, in `GeckoScreenOrientation.setRequestedOrientation`,
> instead of,
> 
>     Activity activity = getActivity();
>     // ...
>     activity.setRequestedOrientation(activityOrientation);
> 
> you would do,
> 
>     ScreenOrientationDelegate delegate =
> GeckoAppShell.getScreenOrientationDelegate();
>     // ...
>     delegate.setScreenOrientation(orientation);
> 
> That gets rid of the `getActivity` call without too much fuss,
 and doesn't
> make `GeckoScreenOrientation.lock` and `GeckoScreenOrientation.unlock` into
> public APIs that we have to maintain (because in your patch, every
> implementer of ScreenOrientationDelegate has to call
> `GeckoScreenOrientation.lock` and `GeckoScreenOrientation.unlock`).

Yeah, that's fine.  I see these static methods as a temporary detail, just to support Fennec, but I'm happy to just push those bits into Fennec.  It just seemed to not be in the spirit of the existing `GeckoScreenOrientation` implementation.  This functionality has to live somewhere; I'm happy to get it out of GeckoView.

> > > We should move these static screen orientation methods from `GeckoAppShell` to `GeckoScreenOrientation` itself
> > 
> > I am happy to do this, but in general I really want the platform team to
> > make the compiled code changes.  Every one is a pain in the ass for me,
> > breaking me out of my artifact mode flow.  (Although right now
> > https://bugzilla.mozilla.org/show_bug.cgi?id=1318820 is completely
> > destroying my artifact mode flow anyway, boo urns.)
> 
> File a follow-up to do this?

OK.
Comment on attachment 8816642 [details]
Bug 1321638 - Part 3: Add ScreenOrientationDelegate.

https://reviewboard.mozilla.org/r/97310/#review99154

::: mobile/android/geckoview/src/main/java/org/mozilla/gecko/GeckoScreenOrientation.java:190
(Diff revision 2)
>       *
>       * @return Whether the locking was successful.
>       */
>      public boolean lock(ScreenOrientation aScreenOrientation) {
>          Log.d(LOGTAG, "locking to " + aScreenOrientation);
>          update(aScreenOrientation);

We should only call update if set orientation succeeded.

::: mobile/android/geckoview/src/main/java/org/mozilla/gecko/GeckoScreenOrientation.java:206
(Diff revision 2)
>      public boolean unlock() {
>          Log.d(LOGTAG, "unlocking");
> -        setRequestedOrientation(mDefaultScreenOrientation);
> +        final ScreenOrientationDelegate delegate = GeckoAppShell.getScreenOrientationDelegate();
> +        final int activityInfoOrientation = screenOrientationToActivityInfoOrientation(ScreenOrientation.DEFAULT);
> +        delegate.setRequestedOrientationForCurrentActivity(activityInfoOrientation);
>          return update();

Same
Attachment #8816642 - Flags: review?(nchen) → review+
Comment on attachment 8816643 [details]
Bug 1321638 - Part 4: Remove last GeckoInterface.getActivity call in geckoview.

https://reviewboard.mozilla.org/r/97312/#review99158
Attachment #8816643 - Flags: review?(nchen) → review+
Pushed by nalexander@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/00a8b8e0ca9e
Part 1: Remove "app.orientation.default". r=jchen
https://hg.mozilla.org/integration/autoland/rev/47cb57bd5eed
Part 2: Remove fullscreen support for GeckoView consumers. r=jchen
https://hg.mozilla.org/integration/autoland/rev/76f783fafa65
Part 3: Add ScreenOrientationDelegate. r=jchen
https://hg.mozilla.org/integration/autoland/rev/6a26aae5ea15
Part 4: Remove last GeckoInterface.getActivity call in geckoview. r=jchen
Depends on: 1333241
Depends on: 1407835
Product: Firefox for Android → GeckoView
Target Milestone: Firefox 53 → mozilla53
You need to log in before you can comment on or make changes to this bug.