lock() should not fail if another lock() is called in the change event

[mounirlamouri]

Technically the spec requires that but it is a really unexpected behaviour given that the orientation at that point will have succeeded and we shouldn't pretend that it failed because another lock() was called when the page should be notified.

I think the steps in 7.5 should be changed so that we copy the promise and clear the pending promises before firing the event. So if another lock() is called in the change event, they will behave as successive locks, the same way await screen.orientation.lock(); await screen.orientation.lock(); would behave instead of conflicting locks like screen.orientation.lock(); screen.orientation.lock();.

@marcoscaceres WDYT?

FWIW, this was flagged because Chrome is now failing a WPT (https://bugs.chromium.org/p/chromium/issues/detail?id=980588) and I identified this as the root cause.

[marcoscaceres]

Hmm... so this was #120

Copying the promise to special case this doesn't feel right to me. If the caller has a reference to the original promise, then I wonder if they can just await it?:

const p1 = screen.orientation.lock(someOrientation);
let p2;
screen.orientation.onchange = async () => {
  await p1; // wait for p1 to settle...
  // change it!
  p2 = screen.orientation.lock("portrait");
}
await p1;
await p2; 

[mounirlamouri]

I'm not saying that we should special case this but as the spec is written, we are told to reject a promise for which the action was succeeded because we run the event handler of the action succeeding before we resolve the promise. It's a fairly common situation in CS to make copies of objects to avoid re-entrency and it's a fairly similar problem IMO: running the event callbacks will change the state of the next operations.

FWIW, in Blink we run the callbacks and resolve the promise asynchronously. Because they are both async, we avoid the bug and still keep the order. Could this be a valid alternative?

[marcoscaceres]

Let's discuss at TPAC. CC @kenchris should probably be there because #120.

[marcoscaceres]

Fixed in latest spec. We now take a copy of the promise and only resolve it after the event fires.