Closed
Bug 1455740
Opened 6 years ago
Closed 6 years ago
Make GeckoView example app more useful
Categories
(GeckoView :: General, enhancement, P2)
Tracking
(firefox-esr52 wontfix, firefox-esr60 wontfix, firefox59 wontfix, firefox60 wontfix, firefox61 wontfix, firefox62 fixed)
RESOLVED
FIXED
mozilla62
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 | ||
Updated•6 years ago
|
Assignee: nobody → snorp
Comment 1•6 years ago
|
||
P2 because this is useful for Gecko developers but is not a Klar+GV blocker.
status-firefox59:
--- → wontfix
status-firefox60:
--- → wontfix
status-firefox61:
--- → affected
status-firefox-esr52:
--- → wontfix
status-firefox-esr60:
--- → wontfix
Priority: -- → P2
Comment hidden (mozreview-request) |
Comment 3•6 years ago
|
||
mozreview-review |
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+
Assignee | ||
Comment 4•6 years ago
|
||
mozreview-review-reply |
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 5•6 years ago
|
||
mozreview-review |
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 6•6 years ago
|
||
mozreview-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+
Assignee | ||
Comment 7•6 years ago
|
||
mozreview-review-reply |
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.
Assignee | ||
Comment 8•6 years ago
|
||
mozreview-review |
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.
Comment hidden (mozreview-request) |
Comment 10•6 years ago
|
||
Pushed by jwillcox@mozilla.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/aaac72d67c23 Make GeckoViewExample more useful r=esawin,jchen,droeh
Comment 11•6 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/aaac72d67c23
Status: NEW → RESOLVED
Closed: 6 years ago
status-firefox62:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 62
Updated•6 years ago
|
Updated•5 years ago
|
Product: Firefox for Android → GeckoView
Updated•5 years ago
|
Version: Firefox 59 → 59 Branch
Updated•5 years ago
|
Target Milestone: Firefox 62 → mozilla62
You need to log in
before you can comment on or make changes to this bug.
Description
•