LGTM1 to relaunch
This looks perfectly reasonable. Thanks for doing everything necessary
to minimize the damage through what I'm guessing wasn't what one may
call a "fun experience".
On Wed, Sep 28, 2022 at 9:51 PM Matthew Wolenetz
<wolen...@chromium.org> wrote:
Regarding the scope of the fix involved in this relaunch, it
removes visibility of the new handle attribute from the
main/Window context (and updates test expectations, accordingly.)
While that change alone would fix the regression, it risks
potential for unexpected exception in future if that attribute
were ever exposed again on the main/Window context.
Therefore, both the spec and implementation involved in this
relaunch also remove the ability to throw NotSupportedError
exception by this attribute's getter.
With the net change versus the previous launch being specific to
fixing this regression, and with the relaunch CL also including
base::Feature guards for the two blink RuntimeEnabled features for
MSE-in-Workers, I believe the risk of relaunch causing respin to
be small even if some similar scope of regression were found after
the feature reached stable.
Matt
On Wed, Sep 28, 2022 at 12:40 PM Matthew Wolenetz
<wolen...@chromium.org> wrote:
Per previous LGTMs on this, the feature launched in M105:
https://chromium-review.googlesource.com/c/chromium/src/+/3795560
Unfortunately, both the MSE spec and Chromium implementation
for this feature included logic which regressed some media
playback sites, and the regression was found late after the
feature reached stable in M105.
The launch was reverted from 105 (and 106-108 also have had
this feature reverted explicitly back to experimental).
The regression was due to the MediaSource object's new handle
attribute being:
1. Visible on both main/Window and on Dedicated Worker
contexts (not just the latter), and
2. Throwing NotSupportedError exception if read from on a
context (main/Window, in this case) where the
implementation does not support using a MediaSourceHandle
for attaching the MediaSource object owned by that context.
The specific MSE spec language involved in this regression
(from https://github.com/w3c/media-source/pull/306):
"If the implementation does not support creating a handle
for this MediaSource, then throw a NotSupportedError
exception and abort these steps.NOTE Implementations MAY
choose to only allow handle creation for MediaSource
objects in a DedicatedWorkerGlobalScope, as a minimum
requirement for enabling attachment of such a MediaSource
object to an HTMLMediaElement."
Uncaught by myself nor multiple reviewers (except partially by
cas...@chromium.org, who asked about precedent for throwing
exceptions in attribute getters
<https://chromium-review.googlesource.com/c/chromium/src/+/3750140/comment/201a43a2_85dfa17f/>),
there was a pre-existing behavior pattern in at least one
older version of a media playback javascript library
(video.js) that enumerated all MediaSource object attributes,
including reading from them and obtaining an unexpected
exception on reading this new handle attribute on the
main/Window context.
Huh! Might be worthwhile to file an issue on the TAG's design
principles to warn against making getters throw exceptions or have any
other side effects for that matter.
Also uncaught by myself nor reviewers was the lack of
"flag-guarding" of this feature (it only had blink runtime
enabled features, not corresponding base::Features), so the
revert in M105 stable of the previous launch required a binary
respin.
Maybe our process should strongly-recommend/require adding a
base_feature
<https://chromium.googlesource.com/chromium/src/+/HEAD/third_party/blink/renderer/platform/RuntimeEnabledFeatures.md#generate-a-instance-from-a-blink-feature> flag
for anything that changes the API shape, as it's very hard to predict
such breakage..
The MSE spec has been updated to replace this regressing logic
<https://github.com/w3c/media-source/pull/317>, and the
corresponding Chromium implementation update, addition of
flag-guards, and relaunch to stable (in current M108
milestone) is in code-review now
<https://chromium-review.googlesource.com/c/chromium/src/+/3910706>
and is pending further LGTMs on this thread.
Please review for relaunch. I expect I'll need 3 LGTMs to
proceed with landing the relaunch CL
<https://chromium-review.googlesource.com/c/chromium/src/+/3910706>.
Thank you,
Matt
On Mon, Aug 1, 2022 at 10:59 AM 'Joe Medley' via blink-dev
<blink-dev@chromium.org> wrote:
Gang,
NOTE: Requested desktop and Android ship milestone:
105 (The 'edit estimated milestones' link from
chromestatus didn't result in such ability to edit, so
I'm adding this request explicitly here.)
I'm looking into this and will report back. Has anyone
else noticed this problem?
Joe
Joe Medley | Technical Writer, Chrome DevRel
|jmed...@google.com |816-678-7195 <tel:(816)%20678-7195>
/If an API's not documented it doesn't exist./
On Mon, Aug 1, 2022 at 8:52 AM Mike Taylor
<miketa...@chromium.org> wrote:
LGTM3
On 8/1/22 8:05 AM, Yoav Weiss wrote:
LGTM2
On Fri, Jul 29, 2022 at 12:40 AM slightlyoff via
Chromestatus
<admin+slightly...@cr-status.appspotmail.com
<mailto:admin%2bslightly...@cr-status.appspotmail.com>>
wrote:
LGTM1
--
You received this message because you are
subscribed to the Google Groups "blink-dev" group.
To unsubscribe from this group and stop receiving
emails from it, send an email to
blink-dev+unsubscr...@chromium.org.
To view this discussion on the web visit
https://groups.google.com/a/chromium.org/d/msgid/blink-dev/000000000000eea04c05e4e53819%40google.com
<https://groups.google.com/a/chromium.org/d/msgid/blink-dev/000000000000eea04c05e4e53819%40google.com?utm_medium=email&utm_source=footer>.
--
You received this message because you are subscribed
to the Google Groups "blink-dev" group.
To unsubscribe from this group and stop receiving
emails from it, send an email to
blink-dev+unsubscr...@chromium.org.
To view this discussion on the web visit
https://groups.google.com/a/chromium.org/d/msgid/blink-dev/CAL5BFfWsqYRjB%3DqCuu4UxHzEa%2B%2B2SqkDJdE8W8sUNCnS%2BwoH5w%40mail.gmail.com
<https://groups.google.com/a/chromium.org/d/msgid/blink-dev/CAL5BFfWsqYRjB%3DqCuu4UxHzEa%2B%2B2SqkDJdE8W8sUNCnS%2BwoH5w%40mail.gmail.com?utm_medium=email&utm_source=footer>.
--
You received this message because you are subscribed
to the Google Groups "blink-dev" group.
To unsubscribe from this group and stop receiving
emails from it, send an email to
blink-dev+unsubscr...@chromium.org.
To view this discussion on the web visit
https://groups.google.com/a/chromium.org/d/msgid/blink-dev/91d78e87-3241-0719-eb2a-a7e8d21afd0b%40chromium.org
<https://groups.google.com/a/chromium.org/d/msgid/blink-dev/91d78e87-3241-0719-eb2a-a7e8d21afd0b%40chromium.org?utm_medium=email&utm_source=footer>.
--
You received this message because you are subscribed to
the Google Groups "blink-dev" group.
To unsubscribe from this group and stop receiving emails
from it, send an email to blink-dev+unsubscr...@chromium.org.
To view this discussion on the web visit
https://groups.google.com/a/chromium.org/d/msgid/blink-dev/CAJUhtG8EH2E1y33qLPJr3xdp8ZT1z1T98mCQ9bJ4WeBn19BOsQ%40mail.gmail.com
<https://groups.google.com/a/chromium.org/d/msgid/blink-dev/CAJUhtG8EH2E1y33qLPJr3xdp8ZT1z1T98mCQ9bJ4WeBn19BOsQ%40mail.gmail.com?utm_medium=email&utm_source=footer>.
