Closed Bug 1455740 Opened 2 years ago Closed Last year

Make GeckoView example app more useful

Categories

(GeckoView :: General, enhancement, P2)

59 Branch
enhancement

Tracking

(firefox-esr52 wontfix, firefox-esr60 wontfix, firefox59 wontfix, firefox60 wontfix, firefox61 wontfix, firefox62 fixed)

RESOLVED FIXED
mozilla62
Tracking Status
firefox-esr52 --- wontfix
firefox-esr60 --- wontfix
firefox59 --- wontfix
firefox60 --- wontfix
firefox61 --- wontfix
firefox62 --- fixed

People

(Reporter: snorp, Assigned: snorp)

Details

Attachments

(1 file)

It needs some generic cleanup and also the ability to enter URLs, change GeckoSession and GeckoRuntime settings, etc.
Assignee: nobody → snorp
P2 because this is useful for Gecko developers but is not a Klar+GV blocker.
Comment on attachment 8971692 [details]
Bug 1455740 - Make GeckoViewExample more useful

https://reviewboard.mozilla.org/r/240460/#review246226

r+ with one nit.

::: mobile/android/geckoview_example/src/main/java/org/mozilla/geckoview_example/GeckoViewActivity.java:174
(Diff revision 1)
> +            session.disableTrackingProtection();
> +        }
> +    }
>  
> -        loadSettings(getIntent());
> -        loadFromIntent(getIntent());
> +    @Override
> +    public void onBackPressed() {

I think you want to conditionally have this exit fullscreen mode.
Attachment #8971692 - Flags: review?(droeh) → review+
Comment on attachment 8971692 [details]
Bug 1455740 - Make GeckoViewExample more useful

https://reviewboard.mozilla.org/r/240460/#review246226

> I think you want to conditionally have this exit fullscreen mode.

Indeed! Fixed.
Comment on attachment 8971692 [details]
Bug 1455740 - Make GeckoViewExample more useful

https://reviewboard.mozilla.org/r/240460/#review246272

::: mobile/android/geckoview_example/src/main/java/org/mozilla/geckoview_example/GeckoViewActivity.java:112
(Diff revision 1)
>              sGeckoRuntime = GeckoRuntime.create(this, runtimeSettingsBuilder.build());
>          }
>  
> -        final GeckoSessionSettings sessionSettings = new GeckoSessionSettings();
> -        sessionSettings.setBoolean(GeckoSessionSettings.USE_MULTIPROCESS,
> -                                   useMultiprocess);
> +        mGeckoSession = (GeckoSession)getIntent().getParcelableExtra("session");
> +        Log.d(LOGTAG, "Intent gecko session = " + mGeckoSession);
> +        if (mGeckoSession != null) {

`mUseE10s` et al should be updated to setting from `mGeckoSession` if it's not null.

::: mobile/android/geckoview_example/src/main/java/org/mozilla/geckoview_example/GeckoViewActivity.java:114
(Diff revision 1)
>  
> -        final GeckoSessionSettings sessionSettings = new GeckoSessionSettings();
> -        sessionSettings.setBoolean(GeckoSessionSettings.USE_MULTIPROCESS,
> -                                   useMultiprocess);
> -        mGeckoSession = new GeckoSession(sessionSettings);
> -
> +        mGeckoSession = (GeckoSession)getIntent().getParcelableExtra("session");
> +        Log.d(LOGTAG, "Intent gecko session = " + mGeckoSession);
> +        if (mGeckoSession != null) {
> +            hookupSession(mGeckoSession);
> +            mGeckoView.setSession(mGeckoSession);

This assumes `mGeckoSession` is already open, which is not necessarily the case in general.

::: mobile/android/geckoview_example/src/main/java/org/mozilla/geckoview_example/GeckoViewActivity.java:208
(Diff revision 1)
> +                break;
> +            case R.id.action_forward:
> +                mGeckoSession.goForward();
> +                break;
> +            case R.id.action_e10s:
> +                mUseE10s = !mUseE10s;

`= item.isChecked()`

::: mobile/android/geckoview_example/src/main/java/org/mozilla/geckoview_example/GeckoViewActivity.java:212
(Diff revision 1)
> +            case R.id.action_e10s:
> +                mUseE10s = !mUseE10s;
> +                recreateSession();
> +                break;
> +            case R.id.action_tp:
> +                mUseTp = !mUseTp;

Same

::: mobile/android/geckoview_example/src/main/java/org/mozilla/geckoview_example/GeckoViewActivity.java:217
(Diff revision 1)
> +                mUseTp = !mUseTp;
> +                updateTrackingProtection(mGeckoSession);
> +                mGeckoSession.reload();
> +                break;
> +            case R.id.action_pb:
> +                mUsePrivateBrowsing = !mUsePrivateBrowsing;

Same

::: mobile/android/geckoview_example/src/main/java/org/mozilla/geckoview_example/GeckoViewActivity.java:223
(Diff revision 1)
> +                recreateSession();
> +            case R.id.action_crash_native:
> +                GeckoThread.crash();
> +                break;
> +            case R.id.action_crash_java:
> +                throw new IllegalStateException("Crash via menu");

`RuntimeException`

::: mobile/android/geckoview_example/src/main/java/org/mozilla/geckoview_example/GeckoViewActivity.java:237
(Diff revision 1)
> +    }
> +
> +    @Override
> +    public void onDestroy() {
> +        super.onDestroy();
> +        mGeckoView.releaseSession().close();

Let GeckoView close the session. Otherwise this prevents GeckoView save/restore from working.

::: mobile/android/geckoview_example/src/main/java/org/mozilla/geckoview_example/LocationView.java:27
(Diff revision 1)
> +        super(context);
> +
> +        this.setInputType(EditorInfo.TYPE_CLASS_TEXT | EditorInfo.TYPE_TEXT_VARIATION_URI);
> +        this.setSingleLine(true);
> +        this.setSelectAllOnFocus(true);
> +        this.setHint("Enter URL or search keywords...");

Localize
Attachment #8971692 - Flags: review?(nchen) → review+
Comment on attachment 8971692 [details]
Bug 1455740 - Make GeckoViewExample more useful

https://reviewboard.mozilla.org/r/240460/#review246388

::: mobile/android/geckoview_example/src/main/java/org/mozilla/geckoview_example/GeckoViewActivity.java:47
(Diff revision 1)
>      private static final int REQUEST_PERMISSIONS = 2;
>  
>      private static GeckoRuntime sGeckoRuntime;
>      private GeckoSession mGeckoSession;
>      private GeckoView mGeckoView;
> +    private boolean mUseE10s = true;

We don't call it e10s over here.

::: mobile/android/geckoview_example/src/main/java/org/mozilla/geckoview_example/GeckoViewActivity.java:48
(Diff revision 1)
>  
>      private static GeckoRuntime sGeckoRuntime;
>      private GeckoSession mGeckoSession;
>      private GeckoView mGeckoView;
> +    private boolean mUseE10s = true;
> +    private boolean mUseTp = false;

mUseTrackingProtection

::: mobile/android/geckoview_example/src/main/java/org/mozilla/geckoview_example/GeckoViewActivity.java:111
(Diff revision 1)
>  
>              sGeckoRuntime = GeckoRuntime.create(this, runtimeSettingsBuilder.build());
>          }
>  
> -        final GeckoSessionSettings sessionSettings = new GeckoSessionSettings();
> -        sessionSettings.setBoolean(GeckoSessionSettings.USE_MULTIPROCESS,
> +        mGeckoSession = (GeckoSession)getIntent().getParcelableExtra("session");
> +        Log.d(LOGTAG, "Intent gecko session = " + mGeckoSession);

Will that print anything useful?

::: mobile/android/geckoview_example/src/main/java/org/mozilla/geckoview_example/GeckoViewActivity.java:134
(Diff revision 1)
> -        mGeckoSession.setContentDelegate(new MyGeckoViewContent());
> -        final MyTrackingProtection tp = new MyTrackingProtection();
> -        mGeckoSession.setTrackingProtectionDelegate(tp);
> -        mGeckoSession.setProgressDelegate(new MyGeckoViewProgress(tp));
> -        mGeckoSession.setNavigationDelegate(new Navigation());
> +        hookupSession(session);
> +
> +        return session;
> +    }
> +
> +    private void hookupSession(GeckoSession session) {

Maybe better (connect|set)Session?

::: mobile/android/geckoview_example/src/main/java/org/mozilla/geckoview_example/GeckoViewActivity.java:175
(Diff revision 1)
> +        }
> +    }
>  
> -        loadSettings(getIntent());
> -        loadFromIntent(getIntent());
> +    @Override
> +    public void onBackPressed() {
> +        if (mCanGoBack) {

Add mGeckoSession != null just in case.

::: mobile/android/geckoview_example/src/main/java/org/mozilla/geckoview_example/GeckoViewActivity.java:244
(Diff revision 1)
>  
>      @Override
>      protected void onNewIntent(final Intent intent) {
>          super.onNewIntent(intent);
>  
> -        if (ACTION_SHUTDOWN.equals(intent.getAction())) {
> +        Log.d(LOGTAG, "onNewIntent: " + intent);

Do we no longer need the shutdown intent for autophone?
Attachment #8971692 - Flags: review?(esawin) → review+
Comment on attachment 8971692 [details]
Bug 1455740 - Make GeckoViewExample more useful

https://reviewboard.mozilla.org/r/240460/#review246388

> Will that print anything useful?

Nope, debugging cruft. Removed.

> Do we no longer need the shutdown intent for autophone?

Ah, we probably do. Crap. I don't think this is going to work at all anymore because I changed the launch mode. I'll have to look into that some more.
Comment on attachment 8971692 [details]
Bug 1455740 - Make GeckoViewExample more useful

https://reviewboard.mozilla.org/r/240460/#review246530

::: mobile/android/geckoview_example/src/main/java/org/mozilla/geckoview_example/GeckoViewActivity.java:208
(Diff revision 1)
> +                break;
> +            case R.id.action_forward:
> +                mGeckoSession.goForward();
> +                break;
> +            case R.id.action_e10s:
> +                mUseE10s = !mUseE10s;

Actually this isn't very useful because the checked state is not changed automatically when it is selected. So we'd have to call item.setChecked(!item.isChecked()) at the top. I think it's better to flip the single source of truth, which is mUseE10s in this case.

::: mobile/android/geckoview_example/src/main/java/org/mozilla/geckoview_example/GeckoViewActivity.java:244
(Diff revision 1)
>  
>      @Override
>      protected void onNewIntent(final Intent intent) {
>          super.onNewIntent(intent);
>  
> -        if (ACTION_SHUTDOWN.equals(intent.getAction())) {
> +        Log.d(LOGTAG, "onNewIntent: " + intent);

I fixed this by using a separate Activity for the onNewSession() case. I'll put up another patch.
Pushed by jwillcox@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/aaac72d67c23
Make GeckoViewExample more useful r=esawin,jchen,droeh
https://hg.mozilla.org/mozilla-central/rev/aaac72d67c23
Status: NEW → RESOLVED
Closed: Last year
Resolution: --- → FIXED
Target Milestone: --- → Firefox 62
Product: Firefox for Android → GeckoView
Version: Firefox 59 → 59 Branch
Target Milestone: Firefox 62 → mozilla62
You need to log in before you can comment on or make changes to this bug.