1413698 - Split GeckoView into GeckoSession

Status: RESOLVED FIXED

(GeckoView :: General, enhancement)

Description

[(inactive) Jim Chen [:jchen] [:darchons]]

Move most of GeckoView into GeckoSession, and introduce a minimal GeckoView that makes use of GeckoSession.

Comment 1

[(inactive) Jim Chen [:jchen] [:darchons]]

Attachment: Bug 1413698 - 1. Separate out attach() from open() in GeckoView.Window;

Right now, `GeckoView.Window.open()` consists of opening a new Gecko
nsWindow and attaching it to the opening GeckoView. This patch separates
the attaching step into an `Window.attach()` function that was renamed
from `Window.reattach()`. Going forward, `Window.open()` and
`Window.close()` will correspond to opening and closing a session, which
`Window.attach()` will corrspond to attaching a display to a session.

Review commit: https://reviewboard.mozilla.org/r/195862/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/195862/

Comment 2

[(inactive) Jim Chen [:jchen] [:darchons]]

Attachment: Bug 1413698 - 2. Move GeckoView to GeckoSession;

Rename GeckoView to GeckoSession. Strip out parts of it that depended on
being a subclass of View. Also strip out parts of it that dealt with
switching EventDispatcher and NativeQueue, because now there's only one
copy of each for each GeckoSession.

Review commit: https://reviewboard.mozilla.org/r/195864/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/195864/

Comment 3

[(inactive) Jim Chen [:jchen] [:darchons]]

Attachment: Bug 1413698 - 3. Recorganize GeckoViewSettings;

* Move the chromeUri and screenId settings to GeckoViewSettings.

* Add a private data-dir setting that the debugger-socket-dir setting
  falls backs to. Set the data-dir setting inside `GeckoSession.openWindow`.

* Add optional init-only and values properties for settings.

* Use integer constants for display-mode setting.

Review commit: https://reviewboard.mozilla.org/r/195866/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/195866/

Comment 4

[(inactive) Jim Chen [:jchen] [:darchons]]

Attachment: Bug 1413698 - 4. Add minimal GeckoView;

Review commit: https://reviewboard.mozilla.org/r/195868/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/195868/

Comment 5

[(inactive) Jim Chen [:jchen] [:darchons]]

Attachment: Bug 1413698 - 5. Use GeckoSession where appropriate;

Review commit: https://reviewboard.mozilla.org/r/195870/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/195870/

Comment 6

[(inactive) Jim Chen [:jchen] [:darchons]]

Attachment: Bug 1413698 - 6. Remove PresentationView;

Remove PresentationView, which is no longer used. r=me for trivial
patch.

Review commit: https://reviewboard.mozilla.org/r/195872/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/195872/

Comment 7

[James Willcox (:snorp) ([e-mail]) (he/him)]

Comment on attachment 8924635 [details]
Bug 1413698 - 1. Separate out attach() from open() in GeckoView.Window;

https://reviewboard.mozilla.org/r/195862/#review201580

Comment 8

[James Willcox (:snorp) ([e-mail]) (he/him)]

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.

Comment 9

[James Willcox (:snorp) ([e-mail]) (he/him)]

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.

Comment 10

[James Willcox (:snorp) ([e-mail]) (he/him)]

Comment on attachment 8924638 [details]
Bug 1413698 - 4. Add minimal GeckoView;

https://reviewboard.mozilla.org/r/195868/#review201588

Comment 11

[James Willcox (:snorp) ([e-mail]) (he/him)]

Comment on attachment 8924639 [details]
Bug 1413698 - 5. Use GeckoSession where appropriate;

https://reviewboard.mozilla.org/r/195870/#review201594

Comment 12

[(inactive) Jim Chen [:jchen] [:darchons]]

(In reply to James Willcox (:snorp) ([e-mail]) 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) ([e-mail]) 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 13

[(inactive) Jim Chen [:jchen] [:darchons]]

Attachment: Bug 1413698 - 7. Rename GeckoView{Handler,Settings} to GeckoSession{..};

The old GeckoView was renamed to GeckoSession, so the old
GeckoViewHandler and GeckoViewSettings should be renamed to
GeckoSession.. as well. GeckoView JS files are left alone and may be
converted in a future bug. r=me for trivial patch.

Review commit: https://reviewboard.mozilla.org/r/196770/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/196770/

Comment 14

[(inactive) Jim Chen [:jchen] [:darchons]]

Comment on attachment 8924635 [details]
Bug 1413698 - 1. Separate out attach() from open() in GeckoView.Window;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/195862/diff/1-2/

Comment 15

[(inactive) Jim Chen [:jchen] [:darchons]]

Comment on attachment 8924636 [details]
Bug 1413698 - 2. Move GeckoView to GeckoSession;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/195864/diff/1-2/

Comment 16

[(inactive) Jim Chen [:jchen] [:darchons]]

Comment on attachment 8924637 [details]
Bug 1413698 - 3. Recorganize GeckoViewSettings;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/195866/diff/1-2/

Comment 17

[(inactive) Jim Chen [:jchen] [:darchons]]

Comment on attachment 8924638 [details]
Bug 1413698 - 4. Add minimal GeckoView;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/195868/diff/1-2/

Comment 18

[(inactive) Jim Chen [:jchen] [:darchons]]

Comment on attachment 8924639 [details]
Bug 1413698 - 5. Use GeckoSession where appropriate;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/195870/diff/1-2/

Comment 19

[(inactive) Jim Chen [:jchen] [:darchons]]

Comment on attachment 8924640 [details]
Bug 1413698 - 6. Remove PresentationView;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/195872/diff/1-2/

Comment 20

[(inactive) Jim Chen [:jchen] [:darchons]]

Comment on attachment 8924640 [details]
Bug 1413698 - 6. Remove PresentationView;

https://reviewboard.mozilla.org/r/195872/#review201980

Comment 21

[(inactive) Jim Chen [:jchen] [:darchons]]

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

https://reviewboard.mozilla.org/r/196770/#review201982

Comment 22

[Pulsebot]

Pushed by [e-mail]:
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

Comment 23

[Andreea Pavel [:apavel]]

https://hg.mozilla.org/mozilla-central/rev/1ed5527d3f11
https://hg.mozilla.org/mozilla-central/rev/2bb89a9a8308
https://hg.mozilla.org/mozilla-central/rev/8ce593cc1b2d
https://hg.mozilla.org/mozilla-central/rev/bff0682273b3
https://hg.mozilla.org/mozilla-central/rev/1bb6b4ff9df2
https://hg.mozilla.org/mozilla-central/rev/56ee4e33d6ab
https://hg.mozilla.org/mozilla-central/rev/858b513f90ac