Closed Bug 1413698 Opened 3 years ago Closed 3 years ago

Split GeckoView into GeckoSession

Categories

(GeckoView :: General, enhancement)

All
Android
enhancement
Not set
normal

Tracking

(firefox58 fixed)

RESOLVED FIXED
mozilla58
Tracking Status
firefox58 --- fixed

People

(Reporter: jchen, Assigned: jchen)

References

Details

Attachments

(7 files)

Move most of GeckoView into GeckoSession, and introduce a minimal GeckoView that makes use of GeckoSession.
Comment on attachment 8924635 [details]
Bug 1413698 - 1. Separate out attach() from open() in GeckoView.Window;

https://reviewboard.mozilla.org/r/195862/#review201580
Attachment #8924635 - Flags: review?(snorp) → review+
Comment on attachment 8924636 [details]
Bug 1413698 - 2. Move GeckoView to GeckoSession;

https://reviewboard.mozilla.org/r/195864/#review201582

I wish the s/GeckoView/GeckoSession/ bits were separated out to make it easier to review, but lgtm.

::: mobile/android/geckoview/src/main/java/org/mozilla/gecko/GeckoSession.java:381
(Diff revision 1)
> -        super(context);
> +    public int describeContents() {
> +        return 0;
> +    }
>  
> -        final GeckoViewSettings newSettings = new GeckoViewSettings(settings, getEventDispatcher());
> -        init(context, settings);
> +    @Override // Parcelable
> +    public void writeToParcel(Parcel out, int flags) {

Does it make sense to support this? The mWindow is not portable across processes, at least...

::: mobile/android/geckoview/src/main/java/org/mozilla/gecko/GeckoSession.java:472
(Diff revision 1)
> -     * default GeckoView URI. Can only be called before the chrome window is opened during
> +     * default GeckoSession URI. Can only be called before the chrome window is opened during
>       * {@link #onAttachedToWindow}.
>       *
>       * @param uri New chrome URI or null.
>       */
>      public void setChromeUri(final String uri) {

Honestly I'm in favor of just removing this API. I don't think any good can come from it, except I guess we still technically need it for Fennec.
Attachment #8924636 - Flags: review?(snorp) → review+
Comment on attachment 8924637 [details]
Bug 1413698 - 3. Recorganize GeckoViewSettings;

https://reviewboard.mozilla.org/r/195866/#review201586

::: mobile/android/geckoview/src/main/java/org/mozilla/gecko/GeckoViewSettings.java:18
(Diff revision 1)
>  import android.util.Log;
>  
> +import java.util.Arrays;
> +import java.util.Collection;
> +
>  public final class GeckoViewSettings implements Parcelable {

Should be GeckoSessionSettings eventually, right?

::: mobile/android/geckoview/src/main/java/org/mozilla/gecko/GeckoViewSettings.java:26
(Diff revision 1)
> -    public enum DisplayMode {
> -        // This needs to match nsIDocShell.idl
> +    // This needs to match nsIDocShell.idl
> -        BROWSER(0),
> -        MINIMAL_UI(1),
> -        STANDALONE(2),
> -        FULLSCREEN(3);
> +    public static final int DISPLAY_MODE_BROWSER = 0;
> +    public static final int DISPLAY_MODE_MINIMAL_UI = 1;
> +    public static final int DISPLAY_MODE_STANDALONE = 2;
> +    public static final int DISPLAY_MODE_FULLSCREEN = 3;

There are other enums we should convert too if we are going to do this. I agree that the java enum is pretty difficult to use in the way we need.
Attachment #8924637 - Flags: review?(snorp) → review+
Comment on attachment 8924639 [details]
Bug 1413698 - 5. Use GeckoSession where appropriate;

https://reviewboard.mozilla.org/r/195870/#review201594
Attachment #8924639 - Flags: review?(snorp) → review+
(In reply to James Willcox (:snorp) (jwillcox@mozilla.com) from comment #8)
> Comment on attachment 8924636 [details]
> Bug 1413698 - 2. Move GeckoView to GeckoSession;
> 
> >  
> > -        final GeckoViewSettings newSettings = new GeckoViewSettings(settings, getEventDispatcher());
> > -        init(context, settings);
> > +    @Override // Parcelable
> > +    public void writeToParcel(Parcel out, int flags) {
> 
> Does it make sense to support this? The mWindow is not portable across
> processes, at least...

Yeah we need Parcelable support to implement onSaveInstanceState and onRestoreInstanceState.

(In reply to James Willcox (:snorp) (jwillcox@mozilla.com) from comment #9)
> Comment on attachment 8924637 [details]
> Bug 1413698 - 3. Recorganize GeckoViewSettings;
> 
> >  
> > +import java.util.Arrays;
> > +import java.util.Collection;
> > +
> >  public final class GeckoViewSettings implements Parcelable {
> 
> Should be GeckoSessionSettings eventually, right?

Yeah good call.
Comment on attachment 8925642 [details]
Bug 1413698 - 7. Rename GeckoView{Handler,Settings} to GeckoSession{..};

https://reviewboard.mozilla.org/r/196770/#review201982
Attachment #8925642 - Flags: review+
Pushed by nchen@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/1ed5527d3f11
1. Separate out attach() from open() in GeckoView.Window; r=snorp
https://hg.mozilla.org/integration/autoland/rev/2bb89a9a8308
2. Move GeckoView to GeckoSession; r=snorp
https://hg.mozilla.org/integration/autoland/rev/8ce593cc1b2d
3. Recorganize GeckoViewSettings; r=snorp
https://hg.mozilla.org/integration/autoland/rev/bff0682273b3
4. Add minimal GeckoView; r=snorp
https://hg.mozilla.org/integration/autoland/rev/1bb6b4ff9df2
5. Use GeckoSession where appropriate; r=snorp
https://hg.mozilla.org/integration/autoland/rev/56ee4e33d6ab
6. Remove PresentationView; r=jchen
https://hg.mozilla.org/integration/autoland/rev/858b513f90ac
7. Rename GeckoView{Handler,Settings} to GeckoSession{..}; r=jchen
Depends on: 1415074
Depends on: 1435500
Product: Firefox for Android → GeckoView
Target Milestone: Firefox 58 → mozilla58
You need to log in before you can comment on or make changes to this bug.