New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Support for ServoSession #498

Merged
merged 1 commit into from Oct 17, 2018

Conversation

Projects
None yet
3 participants
@paulrouget
Collaborator

paulrouget commented Sep 10, 2018

This is an early PR to support Servo. It toggles from Gecko to Servo when the homebutton is pressed (just as a temporary trigger). It overrides GeckoSession and a few other classes.

I'd like to get an early feedback to know if this approach is sensible.

@MortimerGoro Can I ask you to take a look?

@paulrouget paulrouget force-pushed the paulrouget:hotswapping branch 4 times, most recently from 2a3ee24 to cafd4b5 Sep 10, 2018

@paulrouget

This comment has been minimized.

Collaborator

paulrouget commented Sep 10, 2018

Note: I can't extend GeckoDisplay and PanZoomController. See https://bugzilla.mozilla.org/show_bug.cgi?id=1489933

@paulrouget paulrouget force-pushed the paulrouget:hotswapping branch 2 times, most recently from 1aca998 to c26e127 Sep 10, 2018

@MortimerGoro

MortimerGoro requested changes Sep 17, 2018 edited

Yes, I think overriding GeckoSession and the others is the easiest approach. Ideally we can get the GeckoDisplay & PanZooomController extend fixed in GeckoView. ServoSession code looks good.

We should make using servo compilation optional for now, until it's ready in maven and ready for production / WebVR working.

We can use user.properties file as we do with local GeckoView builds. Check this code for reference:

https://github.com/MozillaReality/FirefoxReality/blob/master/app/build.gradle#L200

Example user.properties file here. We could add something as servoViewLocal.

geckoViewLocal=/Users/mortimer/Projects/geckoLocal/geckoview.aar

When servoViewLocal is not defined we should not include the toggle in dev settings and inject the bridge classes. Probably the best way would be to move the classes to servoview gradle project in FxR which could only compiled if geckoview.aar exists. We could use reflection instead new ServoSessionso that part doesn't fail to compile when servo dependency is not available or disabled.

@paulrouget paulrouget force-pushed the paulrouget:hotswapping branch from bc47c48 to 7d74382 Sep 25, 2018

@paulrouget

This comment has been minimized.

Collaborator

paulrouget commented Sep 26, 2018

Can we make Servo support non optional? The AAR would be pulled from download.servo.org. Or at least make sure the CI builds and test Servo support?

If we don't build Servo automatically, I'm afraid Servo support will break every other day.

@MortimerGoro

This comment has been minimized.

Collaborator

MortimerGoro commented Sep 26, 2018

is Maven deploy possible?

It would make things a bit easier because that way it handles the cache and we can set a stable version, instead of pulling always the latest version from download.servo.org

@paulrouget

This comment has been minimized.

Collaborator

paulrouget commented Sep 26, 2018

is Maven deploy possible?

Yes. I'll see if we can use is maven.m.o or if we should setup something under servo.org.

@paulrouget paulrouget changed the title from [WIP] Support for ServoSession to [WIP] Support for ServoSession - take 1 Sep 27, 2018

@paulrouget paulrouget force-pushed the paulrouget:hotswapping branch 2 times, most recently from 24bdecf to fa1f324 Sep 28, 2018

@paulrouget

This comment has been minimized.

@paulrouget paulrouget force-pushed the paulrouget:hotswapping branch from fa1f324 to dc9d7a2 Sep 28, 2018

@paulrouget paulrouget changed the title from [WIP] Support for ServoSession - take 1 to [WIP] Support for ServoSession Sep 28, 2018

@paulrouget paulrouget force-pushed the paulrouget:hotswapping branch from dc9d7a2 to 41a3809 Sep 28, 2018

@paulrouget paulrouget changed the title from [WIP] Support for ServoSession to Support for ServoSession Sep 28, 2018

@paulrouget paulrouget force-pushed the paulrouget:hotswapping branch from 41a3809 to d1edfbc Sep 28, 2018

@paulrouget

This comment has been minimized.

Collaborator

paulrouget commented Sep 28, 2018

@MortimerGoro this is ready for review but not 100% ready to land. Waiting on the 2 bugs mentioned above. But I would appreciate an early review.

@Override
public void surfaceDestroyed() {
mServoSession = null;

This comment has been minimized.

@MortimerGoro

MortimerGoro Oct 2, 2018

Collaborator

on GeckoView surfaceDestroyed pauses the compositor internally. We should so something similar in Servo. It's used to pause the compositor while in WebVR, or also when the activity goes to background.

@@ -40,6 +40,13 @@
android:layout_marginStart="10dp"
android:src="@drawable/ic_icon_home" />
<org.mozilla.vrbrowser.ui.UIButton

This comment has been minimized.

@MortimerGoro

MortimerGoro Oct 2, 2018

Collaborator

this is for testing right?

This comment has been minimized.

@paulrouget

paulrouget Oct 2, 2018

Collaborator

Well, we want this to land. It only shows when the pref is on.
We will think of a better UI later I guess.

@MortimerGoro

This comment has been minimized.

Collaborator

MortimerGoro commented Oct 2, 2018

There are still some TODOs (e.g. handle all the kind of motion events correctly, pausing the compositor, ...) but code looks good on a high level

@paulrouget

This comment has been minimized.

Collaborator

paulrouget commented Oct 2, 2018

For the TODO in the panzoom controller, I was thinking about doing that in a follow-up PR.
For the pausing the compositor, or anything that has to do with stopping Servo, I was also plannign to do that later (in servo/servo#21834)

@paulrouget paulrouget force-pushed the paulrouget:hotswapping branch from d1edfbc to 4433afe Oct 3, 2018

@paulrouget

This comment has been minimized.

Collaborator

paulrouget commented Oct 3, 2018

@MortimerGoro I filed follow-up issues for all the FIXME. Once this lands, I will also file a follow-up for the mouse events.

There are only 2 "FIXME BEFORE LANDING" left that I will fix once the maven repo is published.

Can I get a formal review?

@paulrouget

This comment has been minimized.

Collaborator

paulrouget commented Oct 3, 2018

Before landing, we need to:

  • update to GV to 64.0.20181003
  • publish the maven repo and update this PR
@jdm

This comment has been minimized.

jdm commented Oct 5, 2018

@paulrouget

This comment has been minimized.

Collaborator

paulrouget commented Oct 8, 2018

Maven repo is available and GV has been updated.

This is ready to land.

@paulrouget paulrouget force-pushed the paulrouget:hotswapping branch from 5c68f89 to 58c0d96 Oct 8, 2018

@MortimerGoro

MortimerGoro requested changes Oct 9, 2018 edited

I've been testing this and we can create automated builds but I don't think it's ready for production release flavors:

  • The APK size goes from ~60MB to ~105MB. Google Play Store has a limit of 100MB per APK. This is going to block some store uploads. We'd need to split the libraries and implement expansion files https://developer.android.com/google/play/expansion-files#Downloading in order to be able to publish APKs on the store. Oculus Store has a bigger limit (500MB I think) but not sure about the other stores.
  • Tested some pages such as youtube, sketchfab and they didn't work. After loading sketchfab I couldn't navigate to more websites. Are DOM images broken? I can't see them in the home URL or in a google search.

I think the safest thing for now is create a new flavor (e.g oculusvrServo) and include that in the automated builds. I'm thinking on something like this:

  • Move all the files in common/shared/org/mozilla/servo to the servoview-local gradle module
  • servoview-local could be renamed to servo. It may be worth it to create a custom repository for this, in order to separate the Gecko/Servo internal "webview" work from the FxR work. We want to use commit differences between tags for release notes. Use compileOnly instead of implementation for the GeckoView dependency, so it's injected by the root project
  • Create oculusvrServo flavor and include oculusvrServoImplementation project(':servo') dependency
  • Use reflection instead of new ServoSession() so it works on the flavors which don't include Servo yet. (Remember to add a proguard filter)
  • oculusvrServo is used by default to generate Servo builds. In addition with a single line change oculusvrServoImplementation project(':servo') to implementation project(':servo') we can make Servo available on any flavor for local testing or when we decide it's ready to be public on release flavors.
@@ -0,0 +1,36 @@
package org.mozilla.servo;

This comment has been minimized.

@MortimerGoro

MortimerGoro Oct 9, 2018

Collaborator

Move files in common/shared/org/mozilla/servo to a servo module.

@@ -0,0 +1,36 @@
package org.mozilla.servo;
import android.view.Surface;

This comment has been minimized.

@MortimerGoro

MortimerGoro Oct 9, 2018

Collaborator

Move files in common/shared/org/mozilla/servo to a servo module.

@@ -0,0 +1,182 @@
package org.mozilla.servo;

This comment has been minimized.

@MortimerGoro

MortimerGoro Oct 9, 2018

Collaborator

Move files in common/shared/org/mozilla/servo to a servo module.

@@ -0,0 +1,227 @@
package org.mozilla.servo;

This comment has been minimized.

@MortimerGoro

MortimerGoro Oct 9, 2018

Collaborator

Move files in common/shared/org/mozilla/servo to a servo module.

if (!aSettings.servo) {
state.mSession = new GeckoSession();
} else {
state.mSession = new ServoSession(mContext);

This comment has been minimized.

@MortimerGoro

MortimerGoro Oct 9, 2018

Collaborator

Use reflection to create the Servo session. Make sure you add a proguard filter so it works with minification enabled too for release builds

@@ -161,6 +163,12 @@ public void onClick(View view) {
mMultiprocessSwitch.setSoundEffectsEnabled(false);
setMultiprocess(SettingsStore.getInstance(getContext()).isMultiprocessEnabled(), false);
mServoSwitchText = findViewById(R.id.developer_options_servo_switch_text);

This comment has been minimized.

@MortimerGoro

MortimerGoro Oct 9, 2018

Collaborator

Use reflection to detect if Servo is included, and hide the button if it's not included

@paulrouget

This comment has been minimized.

Collaborator

paulrouget commented Oct 10, 2018

@MortimerGoro we are going to target the Oculus store for now. We will implement that expansion system later.

Tested some pages such as youtube, sketchfab and they didn't work. After loading sketchfab I couldn't navigate to more websites. Are DOM images broken? I can't see them in the home URL or in a google search.

We are aware that many websites are broken. We will follow up on these issues after this lands. Our plan is to enable the Servo button only for websites we know work correctly.

@paulrouget paulrouget force-pushed the paulrouget:hotswapping branch from 58c0d96 to 7178776 Oct 12, 2018

@paulrouget

This comment has been minimized.

Collaborator

paulrouget commented Oct 12, 2018

@MortimerGoro I have addressed your comments. Can you take a look and confirm that this is what you wanted.

It's not ready for review because I started getting an "incompatible app" error. I have no idea why. This is unrelated to code change afaict. Still investigating.

@paulrouget paulrouget force-pushed the paulrouget:hotswapping branch 3 times, most recently from cf0ca4a to 61f885b Oct 15, 2018

@paulrouget

This comment has been minimized.

Collaborator

paulrouget commented Oct 15, 2018

I finally worked around the "incompatible app" error.

This is ready for review.

I don't have an Oculus Go device, so please someone give it a try (we might run into an issue where the oculus androidmanifest is not included in the apk).

dependencies {
// To see what the latest servoview version is go here:
// https://download.servo.org/nightly/maven/org/mozilla/servoview/servoview-armv7/maven-metadata.xml
implementation 'org.mozilla.servoview:servoview-armv7:0.0.1.20181005.caa4d190af'

This comment has been minimized.

@jdm

jdm Oct 15, 2018

Please use 0.0.1.20181015.532775 to pull in the latest fixes for oculus.

@jdm

This comment has been minimized.

jdm commented Oct 15, 2018

Looks like our generated maven manifest doesn't contain valid data, so that will have to wait.

@jdm

This comment has been minimized.

jdm commented Oct 15, 2018

0.0.1.20181015.5327758 should now work.

@MortimerGoro

This comment has been minimized.

Collaborator

MortimerGoro commented Oct 15, 2018

@jdm What does 0.0.1.20181015.5327758 fix?

@jdm

This comment has been minimized.

jdm commented Oct 15, 2018

@MortimerGoro Rendering of images without alpha channels (and many three.js examples), as well as reloading pages containing images.

@jdm

This comment has been minimized.

jdm commented Oct 15, 2018

I've tried debug and release servo builds, and I get this:

0-15 18:00:31.284   984  1243 I ActivityManager: Start proc 31704:org.mozilla.vrbrowser/u0a62 for activity org.mozilla.vrbrowser/.VRBrowserActivity
10-15 18:00:31.310   984  1288 D ActivityManager: RestoreAudioRecord called for pkg:org.mozilla.vrbrowser
10-15 18:00:31.311   984  1288 D ActivityManager: restoring op audio record to 1 for pkg:org.mozilla.vrbrowser
10-15 18:00:31.341 31278 31328 E VRB     : ======> Found PID 31704
10-15 18:00:31.397 31704 31723 E GeckoLibLoad: Load sqlite start
10-15 18:00:31.402 31704 31704 W Settings: Setting animator_duration_scale has moved from android.provider.Settings.System to android.provider.Settings.Global, returning read-only global URI.
10-15 18:00:31.409 31704 31723 W GeckoLinker: /data/app/org.mozilla.vrbrowser-1/base.apk!/assets/armeabi-v7a/libnss3.so: Relocation to NULL @0x0012ffd0 for symbol "__cxa_type_match"
10-15 18:00:31.409 31704 31723 E GeckoLibLoad: Load sqlite done
10-15 18:00:31.409 31704 31723 E GeckoLibLoad: Load nss start
10-15 18:00:31.409 31704 31723 E GeckoLibLoad: Load nss done
10-15 18:00:31.409 31704 31723 W GeckoLinker: Couldn't map /data/user/0/org.mozilla.vrbrowser/cache/libxul.so.crc to validate checksum
10-15 18:00:31.410 31704 31704 E VRB     : in onCreate
10-15 18:00:31.428 31704 31704 D AndroidRuntime: Shutting down VM
10-15 18:00:31.429 31704 31704 E GeckoCrashHandler: >>> REPORTING UNCAUGHT EXCEPTION FROM THREAD 1 ("main")
10-15 18:00:31.429 31704 31704 E GeckoCrashHandler: java.lang.IllegalArgumentException: Unable to find native library main using classloader: dalvik.system.PathClassLoader[DexPathList[[zip file "/data/app/org.mozilla.vrbrowser-1/base.apk"],nativeLibraryDirectories=[/data/app/org.mozilla.vrbrowser-1/lib/arm, /data/app/org.mozilla.vrbrowser-1/base.apk!/lib/armeabi-v7a, /system/lib, /vendor/lib]]]
10-15 18:00:31.429 31704 31704 E GeckoCrashHandler: 	at android.app.NativeActivity.onCreate(NativeActivity.java:168)
10-15 18:00:31.429 31704 31704 E GeckoCrashHandler: 	at org.mozilla.vrbrowser.PlatformActivity.onCreate(PlatformActivity.java:27)
10-15 18:00:31.429 31704 31704 E GeckoCrashHandler: 	at org.mozilla.vrbrowser.VRBrowserActivity.onCreate(VRBrowserActivity.java:130)
10-15 18:00:31.429 31704 31704 E GeckoCrashHandler: 	at android.app.Activity.performCreate(Activity.java:6682)
10-15 18:00:31.429 31704 31704 E GeckoCrashHandler: 	at android.app.Instrumentation.callActivityOnCreate(Instrumentation.java:1119)
10-15 18:00:31.429 31704 31704 E GeckoCrashHandler: 	at android.app.ActivityThread.performLaunchActivity(ActivityThread.java:2633)
10-15 18:00:31.429 31704 31704 E GeckoCrashHandler: 	at android.app.ActivityThread.handleLaunchActivity(ActivityThread.java:2741)
10-15 18:00:31.429 31704 31704 E GeckoCrashHandler: 	at android.app.ActivityThread.-wrap12(ActivityThread.java)
10-15 18:00:31.429 31704 31704 E GeckoCrashHandler: 	at android.app.ActivityThread$H.handleMessage(ActivityThread.java:1478)
10-15 18:00:31.429 31704 31704 E GeckoCrashHandler: 	at android.os.Handler.dispatchMessage(Handler.java:102)
10-15 18:00:31.429 31704 31704 E GeckoCrashHandler: 	at android.os.Looper.loop(Looper.java:154)
10-15 18:00:31.429 31704 31704 E GeckoCrashHandler: 	at android.app.ActivityThread.main(ActivityThread.java:6143)
10-15 18:00:31.429 31704 31704 E GeckoCrashHandler: 	at java.lang.reflect.Method.invoke(Native Method)
10-15 18:00:31.429 31704 31704 E GeckoCrashHandler: 	at com.android.internal.os.ZygoteInit$MethodAndArgsCaller.run(ZygoteInit.java:889)
10-15 18:00:31.429 31704 31704 E GeckoCrashHandler: 	at com.android.internal.os.ZygoteInit.main(ZygoteInit.java:779)

The non-servo builds work fine.

@paulrouget

This comment has been minimized.

Collaborator

paulrouget commented Oct 16, 2018

Oculus right? Pretty sure it's missing manifest. It's not as easy as for googlevr. Looking at it.

@paulrouget paulrouget force-pushed the paulrouget:hotswapping branch from 61f885b to 7ea0b97 Oct 16, 2018

@paulrouget

This comment has been minimized.

Collaborator

paulrouget commented Oct 16, 2018

I checked the Oculus APK and indeed the manifest was missing.
It should work now (also fixed the servo version).

@MortimerGoro

Looks good. One thing missing is the proguard rule for the reflection class. I have not tried but it may fail in release builds without that

@paulrouget paulrouget force-pushed the paulrouget:hotswapping branch from 7ea0b97 to b796eed Oct 16, 2018

@paulrouget

This comment has been minimized.

Collaborator

paulrouget commented Oct 16, 2018

I tested with a release build (googlevr) and it works.

@jdm

This comment has been minimized.

jdm commented Oct 16, 2018

The latest changes work on Oculus.

@paulrouget paulrouget force-pushed the paulrouget:hotswapping branch from b796eed to d29da61 Oct 17, 2018

@paulrouget paulrouget force-pushed the paulrouget:hotswapping branch from d29da61 to 971d99f Oct 17, 2018

@MortimerGoro MortimerGoro merged commit 388bfe3 into MozillaReality:master Oct 17, 2018

1 check passed

Taskcluster (pull_request) TaskGroup: success
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment