MozillaReality / webxr-ios-js

INACTIVE - A JS implementation of WebXR used *only* in Mozilla's WebXR Viewer
Mozilla Public License 2.0
71 stars 30 forks source link

fix monkeypatch issues #77

Closed joshmarinacci closed 2 months ago

joshmarinacci commented 4 years ago

Eventually, I think we will need to instantiate class dummy objects for every class before loading the polyfill, so people can monkey-patch them. I think doing something with internal private methods may make this easy, so that we “implement” all the method of each class, but have them call internal private version, initially null. When we load the full polyfill, we just patch the internal methods. That way, if someone replaces “init()” in this code below, with a new method that calls the original init(), it will still work.

Class foo {
       privateMethodFooInit = null;

       init() { if (privateMethodFooInit) { privateMethodFooInit()} }
}

Off the top of my head, that’s it.

blairmacintyre commented 4 years ago

@takahirox @joshmarinacci I think the only way to make this work is as follows.

1) create dummy window[] objects as we do now, in the shim, but each of these objects has a dummy method for every required API call. These dummy methods look for a method of the same name inside a HIDDEN object and call in. This doesn't exist yet, so it does nothing 2) for each of these dummy methods method(), save a second reference to it in a hidden private object inside the dummy object. 3) before loading the polyfill, instead of deleting the dummy objects, just move them to a temporary object so we don't lose them 4) after loading the polyfill, look through the old dummy objects. For each of our dummy methods, if it has changed (doesn't match the original dummy), we need to move the polyfill's object method out of the way (into a HIDDEN object mentioned above, in the polyfill's object) and replace it with the non-matching method (this will be a method that somebody monkeypatched into our dummy object) 5) also look for any other new properties or methods in the dummy object, and move them to the polyfill object.

The upshot is that, if some website patches one of these dummy objects, we will:

I think this will work. Thoughts?

takahirox commented 4 years ago

Perhaps that sounds good?

Before going forward, I have a question. Don't we have an option that we directly embed the polyfill into the viewer? It should be less problematic. Of course it just wastes memory and boot up time in non-WebXR application page, but minified version of the polifill is just about 100KB. Can't be 100KB acceptable?

https://github.com/MozillaReality/webxr-ios-js/tree/develop/dist

Plus, xrshim is 5KB now. And it can be much bigger if we implement the suggestion. Then the advantage of xrshim can be smaller.

https://github.com/MozillaReality/webxr-ios-js/blob/master/xrshim.js

(I should refer to the size of minified version of xrshim to compare more fairly tho)

Update: A disadvantage of embedding is, if we embed the polyfill, the polyfill should be in the viewer code, that means we will no longer be able to frequently update the polyfill code without release the new version of the viewer.

blairmacintyre commented 4 years ago

The intent of the shim is that we want to see if we can make this work with low-impact on non-XR pages. 100kb is too much; I don't think the Firefox for iOS team will be willing to add WebXR to the browser if it has to inject that in every page.

I don't think the shim will be much bigger: the list of methods per object can be in an array, and we can use the variable argument parameter to just accept and pass on the parameters. The only "varability" is async (are there any async methods?), right?

Heck, most of the code I describe above that executes post-load could be in the polyfill itself; the shim just needs to do create the more elaborate temp objects.

takahirox commented 4 years ago

Ok, thanks for clarifying the implementation and that 100KB is too huge