Listen to messages before iframe append

[aghassemi]

Plus a few unrelated cleanups for video players.

[jridgewell]

👏

[jridgewell]

This can be dropped.

[jridgewell]

Hm?

[aghassemi]

sorry for the spammy commits :) PTAL. I added back all the playerReadyPromise stuff, realized we need the playerReadyPromise in buildCallback so we can queue up actions calls until layout is done. (e.g. we want <button on=tap:myVideo.mute> to work even if myVideo has not gone through layoutCallback yet`)

[jridgewell]

If we go down this road, we need to resolve the #playerReadyPromise with the loadPromise. This will drop all queued messages to the iframe if loading fails.

// bad
let resolve;
const promise = new Promise(r => resolve = r);

otherPromise.then(() => {
  // do some stuff
  resolve();
});

// good
let resolve;
const promise = new Promise(r => resolve = r);

resolve(otherPromise.then(() => {
  // do some stuff
}));

[aghassemi]

oh man Travis is running for every one of my back-to-back commits. @erwinmombay @rsimha-amp Can Travis become smart to cancel previous in-progress runs when a new commit comes in?

[aghassemi]

@jridgewell Would a rejected promise drop references to all of its handlers? (wondering if load().then(...).catch( ()=> { this.playerReadyPromise = new Promise... } ) would be any better for memory footprint)

[aghassemi]

@jridgewell Also I think neither the current code nor your proposal will actually send the messages to iframe if load fails (resolving with a rejected promise still results in a rejected promise meaning all the queued up sendCommands won't run). But I think that's what we want anyway.

[jridgewell]

Would a rejected promise drop references to all of its handlers?

Yes, that's why I want you to resolve with a promise and not the promise's fulfilled value. Doing the good example above rejects the original promise if otherPromise rejects.

wondering if load().then(...).catch( ()=> { this.playerReadyPromise = new Promise... } ) would be any better for memory footprint

This creates another promise instance, and takes an extra tick.

[jridgewell]

Also I think neither the current code nor your proposal will actually send the messages to iframe if load fails

Yes. But the difference is the current code keeps all those messages in memory because the promise is never resolved (neither fulfilled or rejected) and could be fulfilled at some point in the future. Rejecting (by resolving with a rejected promise) will drop all those from memory.

[aghassemi]

Got it. PTAL.

[jridgewell]

Nit: Can we join this with the previous block?

[rsimha]

@aghassemi, re: preventing Travis from running against every one of your commits, there seems to be hope: travis-ci/travis-ci#964

I'll investigate this tomorrow and see if it's something we can institute within amphtml.