Skip to content
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

3dom and iframes #1025

Closed
wants to merge 1 commit into from
Closed

3dom and iframes #1025

wants to merge 1 commit into from

Conversation

judax
Copy link

@judax judax commented Feb 13, 2020

Use case: A page contains a model-viewer instance but the functionality to control
that instance through 3dom scripts is emedded in an iframe.

  • ThreeDOMExecutionContext supports different modes: Standalone, ExternalWorker, ExternalPort.

  • One MessageChannel in ThreeDOMExecutionContext.

  • Worker can be null in ThreeDOMExecutionContext.

  • port2 can be exposed in ThreeDOMExecutionContext.

  • The scene-graph feature exposes new method: setThreeDOMExecutionContext.

  • New demo to show the iframe functionality.

@googlebot
Copy link

We found a Contributor License Agreement for you (the sender of this pull request), but were unable to find agreements for all the commit author(s) or Co-authors. If you authored these, maybe you used a different email address in the git commits than was used to sign the CLA (login here to double check)? If these were authored by someone else, then they will need to sign a CLA as well, and confirm that they're okay with these being contributed to Google.
In order to pass this check, please resolve this problem and then comment @googlebot I fixed it.. If the bot doesn't comment, it means it doesn't think anything has changed.

ℹ️ Googlers: Go here for more info.

@judax judax force-pushed the 3dom-iframe branch 3 times, most recently from 3356e52 to 5779bb2 Compare February 14, 2020 05:59
@judax judax requested a review from cdata February 14, 2020 18:12
@cdata
Copy link
Contributor

cdata commented Feb 14, 2020

@judax very interesting idea. I wonder: could we spare most of the modifications required here if we abstracted at a lower level?

For example, we could allow specializations of the ThreeDOMExecutionContext like this:

class ThreeDOMExecutionContext extends EventTarget {
  // ...

  // Creates the isolated context where scripts run, and returns a reference to an object
  // with a Worker-like interface to communicate that context and control its lifecycle
  // NOTE: url may or may not be used depending on the implementation of this
  // method; there are good reasons for both approaches.
  protected createIsolatedContext(url: string): Worker {
    // Default implementation:
    return new Worker(url);
  }
}

With the above implementation, we could write a specialization for the host frame and a counterpart class for the child <iframe>:

/* === In the host frame context  === */

/**
 * A specialization of ThreeDOMExecutionContext that controls a Worker in a
 * remote <iframe>
 */
class HostThreeDOMExecutionContext extends ThreeDOMExecutionContext {
  // Override:
  protected createIsolatedContext(url: string): Worker {
    return new WorkerProxyInterfaceToIframe(url);
  }
}


/**
 * A class that looks like a Worker but actually controls execution in a remote
 * <iframe> context
 */
class WorkerProxyInterfaceToIframe implements Worker {
  protected iframe: HTMLIframeElement;

  constructor(url: string) {
    // Negotiate connection w/ iframe, pass in a dedicated MessagePort
    // Probably best to pass url into the iframe once communication is
    // established, rather than regenerate it inside the iframe
  }

  // Rest of Worker implementation goes here
  // @see https://github.com/microsoft/TypeScript/blob/master/lib/lib.dom.d.ts#L18821-L18830
}



/* === In the <iframe> context  === */

/**
 * Establishes connections with a host frame, creates Worker instances and
 * forwards messages to those workers
 */
class RemoteAgent {
  constructor() {
    self.addEventListener('message', (event) => {
      if (event.origin !== TRUSTED_GOOGLE_ORIGIN) {
        // Summarily ignored
        return;
      }
      // Establish connection with host context:
      this.negotiateConnection(event.ports[0]);
    });
  }

  protected onHostMessage(event: MessageEvent) {
    // Handle host commands to create workers on behalf of the
    // HostThreeDOMExecutionContext
    // Also forward messages from host to the workers
  }

  protected onWorkerMessage(event: MessageEvent) {
    // Forward message from the worker to the host context
  }
}

self.remoteAgent = new RemoteAgent();

In the example above, the WorkerProxyInterfaceToIframe class would be in an intermediary position between the ThreeDOMExecutionContext and the <iframe>. All messages passed from 3DOM intended for the Worker would be proxied by that class. The WorkerProxyInterfaceToIframe would be responsible for managing the relationship with the <iframe> and negotiating the connection to some special code (RemoteAgent in the example) in the <iframe>'s main thread.

The advantage of this approach is that 3DOM can generally remain agnostic to where the Worker runs the related script. It doesn't have to care about whether the Worker is spawned from the same execution context, or is perhaps running in Unity or some other remote context. Instead, it just cares that the thing it is communicating with has a Worker-like interface.

I'm interested to hear your thoughts on this.

Use case: A page contains a model-viewer instance but the functionality to control
that instance through 3dom scripts is emedded in an iframe.

* Modified ThreeDOMExecutionContext to create the worker using a factory method.

* 2 new classes that inherit from ThreeDOMExecutionContext and override the worker
creation method factory method.
  - HostThreeDOMExecutionContext: For the host page that includes the iframe. The
returned worker is fake and just used to intercept the passing of the MessagePort.
The port is transferred not to the worker in the host page but to a worker in the
iframe page. This class handles all the handshake necessary with the iframe.
  - IFrameThreeDOMExecutionContext: For the iframe page. The returned worker is
an actual worker but it waits to hand over the port until it is received from the
host page.

* The scene-graph feature exposes new method: setThreeDOMExecutionContext.

* New demo to show the iframe functionality.
@judax
Copy link
Author

judax commented Feb 20, 2020

Thanks for the feedback Chris. I took a new stab at the proposal following your advise.

  • Added an internal createWorker method in ThreeDOMExecutionContext to delegate the creation of the worker to a method that an inheriting class could override.

  • Created 2 new classes that inherit from ThreeDOMExecutionContext, one fort the host (main) page and one for the iframe page.

    • HostThreeDOMExecutionContext: The worker is fake and captures the port when it is sent out to the worker. It handles all the handshake needed with the iframe page to hand over the captured port and inject the 3dom script passed from the iframe.

    • IFrameThreeDOMExecutionContext: The worker is functional but it also captures the port passing and delays it until the port is received from the host page.

In this new approach I maintained the modification in the scene-graph mixin to set the execution context to model viewer in the host page. I was thinking that this could be done with yet another mixin, but some symbols from the current scene-graph mixin might need to be exported. This would let anyone to create new "flavors" of model-viewer as needed. Let me know what you think of this idea of using a mixin instead or if you have any other ideas.

Copy link

@lincolnfrog lincolnfrog left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks great!

this.hostExecutionContext = hostExecutionContext;
}

postMessage(message: any, transfer: Array<Transferable>): void;

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

what type is message? it seems like it is just an Object? Linter is complaining we're supposed to avoid any in general...

const $iframe = Symbol('iframe');
const $iframeLoaded = Symbol('iframeLoaded');

export class HostThreeDOMExecutionContext extends ThreeDOMExecutionContext {

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Docstring needed here

capabilities: Array<ThreeDOMCapability>,
iframe: HTMLIFrameElement|null = null) {
super(capabilities);
// Make sure there is an iframe to connect with

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this[$iframe] = iframe ? iframe : document.querySelector('iframe');
if (!this[$iframe]) {
throw new Error(...
}

super(capabilities);
// Make sure there is an iframe to connect with
if (!iframe) {
iframe = document.querySelector('iframe');

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

constant?

this.sendHandshakeToIFrame();
}

protected sendHandshakeToIFrame() {

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

docstring

if (!this[$iframeLoaded] || !port2) {
return;
}
// Listen for messages from the iframe

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

any reason this isn't just a class method?

// If the iframe send the handshake response, it will contain the 3DOM
// script to be loaded, so evaluate it (it will actually be received and
// evaluated in the iframe's worker).
if (event.data.action === 'handshakeResponse') {

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should all these literal strings be constants?

}
}

export class IFrameThreeDOMExecutionContext extends ThreeDOMExecutionContext {

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

docstring

super(capabilities);
const onMessageReceived = (event: MessageEvent) => {
switch (event.data.action) {
case 'handshakeRequest': {

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

especially if used in multiple places, these string literals should be constants

break;
}
};
window.addEventListener('message', onMessageReceived);

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this too

@elalish
Copy link
Collaborator

elalish commented Jul 9, 2020

We've gone a different direction with 3DOM: #1302.

@elalish elalish closed this Jul 9, 2020
@elalish elalish deleted the 3dom-iframe branch September 15, 2020 23:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants