Closed Bug 1413698 Opened 7 years ago Closed 7 years ago

Split GeckoView into GeckoSession

Categories

(GeckoView :: General, enhancement)

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

Tracking

(firefox58 fixed)

Status:
RESOLVED FIXED
Milestone:
mozilla58
Tracking Flags:
Tracking Status
firefox58 --- fixed

People

(Reporter: jchen, Assigned: jchen)

References

Details

Attachments

(7 files)

Bug 1413698 - 1. Separate out attach() from open() in GeckoView.Window;
59 bytes, text/x-review-board-request
snorp
: review+
Details
Bug 1413698 - 2. Move GeckoView to GeckoSession;
59 bytes, text/x-review-board-request
snorp
: review+
Details
Bug 1413698 - 3. Recorganize GeckoViewSettings;
59 bytes, text/x-review-board-request
snorp
: review+
Details
Bug 1413698 - 4. Add minimal GeckoView;
59 bytes, text/x-review-board-request
snorp
: review+
Details
Bug 1413698 - 5. Use GeckoSession where appropriate;
59 bytes, text/x-review-board-request
snorp
: review+
Details
Bug 1413698 - 6. Remove PresentationView;
59 bytes, text/x-review-board-request
jchen
: review+
Details
Bug 1413698 - 7. Rename GeckoView{Handler,Settings} to GeckoSession{..};
59 bytes, text/x-review-board-request
jchen
: review+
Details 
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.

Attachment

General

Creator:
Created:
Updated:
Size: