LGTM3 On Thu, Sep 29, 2022 at 3:07 PM Mike Taylor <miketa...@chromium.org> wrote:
> LGTM2 > > On 9/29/22 12:14 AM, Yoav Weiss wrote: > > 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 <(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> 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> >>>> . >>>> >>> > -- > 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/ea179069-c57f-0b83-0377-dfe394e892fc%40chromium.org > <https://groups.google.com/a/chromium.org/d/msgid/blink-dev/ea179069-c57f-0b83-0377-dfe394e892fc%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/CAOMQ%2Bw-%3DqDcGCxKrOCi7s6-x2bkD7Yr80pQ17tEoykLiunR-Eg%40mail.gmail.com.
