Replace return with resolve

[beaufortfrancois]

FIX #226

---

Preview | Diff

[padenot]

Obvious fix, looks good.

[marcoscaceres]

Suggested change

	2. If the previous step threw an exception, reject |promise| with that
	exception and abort these steps.
	2. If the previous step [=exception/throws|threw=] an exception, [=queue a task=] using [=this=]'s [=media element event task source=] to [=/reject=] |promise| with that exception and abort these steps.

[beaufortfrancois]

Both WebKit and Chrome do not queue a task from what I can see at https://github.com/WebKit/WebKit/blob/d36ed108e92fb350723da8e7a88e7a7e8be1ef45/Source/WebCore/Modules/pictureinpicture/DocumentPictureInPicture.cpp#L54 and https://source.chromium.org/chromium/chromium/src/+/main:third_party/blink/renderer/modules/picture_in_picture/document_video_picture_in_picture.cc;l=43;drc=95cf77bcd2a1f44e564e8404d2d363f508d580ce

I'm not sure we want to update both implementations for this.

[marcoscaceres]

Wait! Then this algorithm is wrong then. Above it says it runs “in parallel”, and you can see from both implementations that it’s running on the main thread.

This needs be updated to return a promise rejected with X and the later run the steps in parallel.

[beaufortfrancois]

The reject path runs on the main thread because it's easy to catch some errors but the resolve path and some reject paths are not.

[marcoscaceres]

We need to split those up then. This should match implementations. I can see like 6 reject cases that run on the main thread. Only see this in WebKit getting sent off main thread:

    if (videoElement.webkitSupportsPresentationMode(HTMLVideoElement::VideoPresentationMode::PictureInPicture)) {
        videoElementPictureInPicture->m_enterPictureInPicturePromise = WTFMove(promise);
        videoElement.webkitSetPresentationMode(HTMLVideoElement::VideoPresentationMode::PictureInPicture);
    }

[marcoscaceres]

Even there... we would need to check what videoElement.webkitSetPresentationMode does and if it goes off main thread.

[marcoscaceres]

Suggested change

	4. [=/Resolve=] |promise| with the <a>Picture-in-Picture window</a> associated with
	{{pictureInPictureElement}}.
	4. [=Queue a task=] using |video|'s [=media element event task source=] to [=/resolve=] |promise| with the <a>Picture-in-Picture window</a> associated with |video|'s {{pictureInPictureElement}}.

[marcoscaceres]

Suggested change

	3. If the previous step threw an exception, reject |promise| with that
	exception and abort these steps.
	3. If the previous step [=exception/throws|threw=] an exception, [=queue a task=] using |video|'s [=media element event task source=] to [=/reject=] |promise| with that exception and abort these steps.

[beaufortfrancois]

For both resolve and reject paths, WebKit and Chrome don't queue task. See https://github.com/WebKit/WebKit/blob/d36ed108e92fb350723da8e7a88e7a7e8be1ef45/Source/WebCore/Modules/pictureinpicture/HTMLVideoElementPictureInPicture.cpp#L81 and https://source.chromium.org/chromium/chromium/src/+/main:third_party/blink/renderer/modules/document_picture_in_picture/picture_in_picture_controller_impl.cc;l=124;drc=20135c10f0869fdefb75d990ec84143a649d84c3

[marcoscaceres]

This should be:

The {{requestPictureInPicture()}} method, when invoked, MUST run the following steps:

1. Run the <a>request Picture-in-Picture algorithm</a> with [=this=].
2. If the previous step [=exception/throws|threw=] an exception, return [=a promise rejected with=] that exception.
3. Return [=a promise resolved with=] [=this=]'s associated [=Picture-in-Picture window=].

As it never seems to go into parallel.

[marcoscaceres]

Should be like this... also, we should make better use of [=this=], but we can do that as a followup. I need to go through the document and fix up how internal things are accessed.

[joeyparrish]

Thanks @beaufortfrancois for addressing this!

[beaufortfrancois]

What shall we do about this PR?
It seems like updating the spec with suggested changes from @marcoscaceres would make current implementations not compliant.

[beaufortfrancois]

If that's okay with everyone, I'll merge this PR and will open an issue about @marcoscaceres's feedback

[beaufortfrancois]

I've opened #229