Hi, there I'm now taking over ownership of this I2S from jiacheng@.
Regarding the metrics, I found that we did a previous measurement here: https://bugs.chromium.org/p/chromium/issues/detail?id=1065667#c30 It appears that the rough conclusion from 3 years ago was that most characters, excluding space and asterisk, are probably fine to change to match the spec. Given this, my plan is as follows 1. Fix and ship, except for space and asterisk, but with a kill switch. 2. Re-visit on space and asterisk char issues later. I'll update the chrome status to reflect that. According to https://bugs.chromium.org/p/chromium/issues/detail?id=1248196#c4, it seems the previous measurement had a noticeable performance impact. On Tue, Mar 14, 2023 at 11:04 PM Chris Harrelson <chris...@chromium.org> wrote: > LGTM3 > > On Tue, Mar 14, 2023, 6:22 AM Yoav Weiss <yoavwe...@chromium.org> wrote: > >> LGTM2. Please make sure the use counters are exposed to UKM >> <https://source.chromium.org/chromium/chromium/src/+/main:components/page_load_metrics/browser/observers/use_counter/ukm_features.cc?q=use%20counters%20ukm&ss=chromium> >> . >> >> On Tue, Mar 14, 2023 at 11:09 AM Philip Jägenstedt <foo...@chromium.org> >> wrote: >> >>> LGTM1 to ship this change with a feature flag which we can use as a kill >>> switch. Adding use counters so that we can get examples of breakage *if* >>> it happens would be great too, if it's not too much overhead. >>> >>> On Tue, Mar 14, 2023 at 11:04 AM Philip Jägenstedt <foo...@chromium.org> >>> wrote: >>> >>>> Hi Eli, >>>> >>>> Adding to what Jiacheng said, I've tested isValidHost('example!.com') >>>> with your new code, and it doesn't give the same result in Chrome and >>>> Safari. >>>> >>>> However, the url.host setter doesn't throw for invalid hosts, instead >>>> it does nothing. You could start your helper like this: >>>> >>>> let url; >>>> try { >>>> url = new globalThis.URL('https://' + host); >>>> } catch (e) { >>>> return false; >>>> } >>>> >>>> But isValidHost('example!.com') is still not going to get the same >>>> result in Chrome and Safari, because new URL('https://example!.com') >>>> doesn't throw in either, but Chrome currently will escape the host as >>>> 'example%21.com' while Safari will leave it as 'example!.com'. >>>> >>>> If you want something that is guaranteed to work exactly the same in >>>> all browsers before and after these changes, I think your best bet is to >>>> avoid the URL constructor/API entirely, >>>> >>>> On Tue, Mar 14, 2023 at 7:27 AM Jiacheng Guo <g...@google.com> wrote: >>>> >>>>> Hi Eli, >>>>> >>>>> The implementation is not fully in line with the spec. >>>>> Though hosts are not percent encoded by the browser. The browser will >>>>> percent decode host strings. Thus 'test%22.com' is a valid host >>>>> string. >>>>> If all the browsers are spec compliant when parsing the hosts. I >>>>> believe simply setting url.host and checking for errors will work. (This >>>>> is >>>>> not the case now) >>>>> >>>>> Jiacheng Guo >>>>> >>>>> On Tue, Mar 14, 2023 at 3:05 PM Eli Grey <m...@eligrey.com> wrote: >>>>> >>>>>> I've updated my isValidHost() util to support this change. Could >>>>>> someone please have another look and let me know if my implementation now >>>>>> aligns well with the spec? >>>>>> >>>>>> >>>>>> >>>>>> On Monday, March 13, 2023 at 6:26:18 PM UTC-7 dom...@chromium.org >>>>>> wrote: >>>>>> >>>>>>> On Mon, Mar 13, 2023 at 6:46 PM Philip Jägenstedt < >>>>>>> foo...@chromium.org> wrote: >>>>>>> >>>>>>>> To simplify and keep this moving, I've filed >>>>>>>> https://github.com/mozilla/standards-positions/issues/759 as an >>>>>>>> umbrella issue for anything URL in Interop 2023. >>>>>>>> >>>>>>>> My view is that we can't improve our risk assessment of this by >>>>>>>> much with metrics, because we can't distinguish between harmless and >>>>>>>> serious breakage. Instead what we should do is take some comfort in the >>>>>>>> fact that the behavior is already shipping in Safari, try to ship it >>>>>>>> and >>>>>>>> revert at the first sign of trouble to evaluate. In other words, I'll >>>>>>>> happily LGTM this, but I'll send this round of feedback first, in case >>>>>>>> Yoav >>>>>>>> disagrees with that. >>>>>>>> >>>>>>>> Some additional replies inline: >>>>>>>> >>>>>>>> On Mon, Mar 13, 2023 at 5:30 AM Yoav Weiss <yoav...@chromium.org> >>>>>>>> wrote: >>>>>>>> >>>>>>>>> Thanks for working on interop! :) >>>>>>>>> >>>>>>>> >>>>>>>> Indeed, I'm very grateful and happy to see this work! >>>>>>>> >>>>>>>> Can you please explain what would be the impact of this change and >>>>>>>>> provide examples of cases that are currently working and would stop >>>>>>>>> working >>>>>>>>> after this change is landed? >>>>>>>>> Web developers are asking questions on this thread, and it'd be >>>>>>>>> good to have an explainer that answers such questions. >>>>>>>>> >>>>>>>> >>>>>>>> I've replied to Eli. >>>>>>>> >>>>>>>> More generally, since this is a change in the nitty gritty details, >>>>>>>> my concrete advice for web developers would be to test what Safari >>>>>>>> currently does and assume that's what Chrome is going to start doing. >>>>>>>> If >>>>>>>> one doesn't have access to Safari, then >>>>>>>> https://www.browserstack.com/screenshots can be used for one-off >>>>>>>> tests, as long as the test result appears on the page. >>>>>>>> >>>>>>>> The other difference to Safari that's easy to test for is when >>>>>>>> exceptions are thrown. `new URL('https://example|.com')` returns a >>>>>>>> URL escaped as "https://example%7C.com"; in Chrome, but throws >>>>>>>> TypeError in Safari. >>>>>>>> >>>>>>> >>>>>>> Developers can also use the whatwg-url Node.js package >>>>>>> <https://github.com/jsdom/whatwg-url>, including the live URL viewer >>>>>>> <https://jsdom.github.io/whatwg-url/>. It is kept 1:1 with the URL >>>>>>> Standard and so reflects the behavior that all browsers will be aiming >>>>>>> toward as part of Interop 2023 (and that Safari is already compliant >>>>>>> with). >>>>>>> Examples: >>>>>>> >>>>>>> - Parsing https://example|.com >>>>>>> >>>>>>> <https://jsdom.github.io/whatwg-url/#url=aHR0cHM6Ly9leGFtcGxlfC5jb20=&base=YWJvdXQ6Ymxhbms=> >>>>>>> - Testing Eli's isValidHost usage of the host setter >>>>>>> <https://runkit.com/embed/lwn8l7w4yyj5> >>>>>>> >>>>>>> >>>>>>> >>>>>>>> >>>>>>>> Do we have use counters for content that would start throwing once >>>>>>>>> this change lands? >>>>>>>>> >>>>>>>> >>>>>>>> I'll let Jiacheng answer, but if the answer is no, I'm skeptical >>>>>>>> that adding use counters will meaningfully help us judge the risk of >>>>>>>> this. >>>>>>>> Breaking it down: >>>>>>>> >>>>>>>> - Many invalid URLs already throw exceptions, which may be >>>>>>>> caught. Knowing that new exceptions will be thrown in X% of page >>>>>>>> views will >>>>>>>> not help know how often those are caught and the web app still >>>>>>>> behaves >>>>>>>> correctly. >>>>>>>> - Changes in serialization are akin to changing a return value. >>>>>>>> We can't instrument the downstream effects of that and determine if >>>>>>>> the >>>>>>>> difference led to a different outcome. >>>>>>>> >>>>>>>> Can you provide a link to the tests? >>>>>>>>> >>>>>>>> >>>>>>>> >>>>>>>> https://wpt.fyi/results/url?label=experimental&label=master&product=chrome&product=firefox&product=safari&aligned&view=interop&q=label%3Ainterop-2023-url >>>>>>>> >>>>>>>> There's no way to link to exactly the subtests that will be >>>>>>>> affected, but "Parsing: <http://example example.com> against < >>>>>>>> http://other.com/>" in url-constructor.any.html is one of them. >>>>>>>> >>>>>>>> Best regards, >>>>>>>> Philip >>>>>>>> >>>>>>>> -- >>>>>>>> >>>>>>> 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+...@chromium.org. >>>>>>>> >>>>>>> To view this discussion on the web visit >>>>>>>> https://groups.google.com/a/chromium.org/d/msgid/blink-dev/CAARdPYdKrtMS10VJxvzKntoXMBOEaA1doTYqpQ1W4%2BX1q40-bg%40mail.gmail.com >>>>>>>> <https://groups.google.com/a/chromium.org/d/msgid/blink-dev/CAARdPYdKrtMS10VJxvzKntoXMBOEaA1doTYqpQ1W4%2BX1q40-bg%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/CAL5BFfX7OX4nY-Xyb-JtTer3qUk8iLCorKNYNM%2BVFostCzYhKw%40mail.gmail.com >> <https://groups.google.com/a/chromium.org/d/msgid/blink-dev/CAL5BFfX7OX4nY-Xyb-JtTer3qUk8iLCorKNYNM%2BVFostCzYhKw%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/CAOMQ%2Bw-txxAerRT7rpqU7Ac9mCu2t%2BK8OwVhP3rPwVF3VPVnvw%40mail.gmail.com > <https://groups.google.com/a/chromium.org/d/msgid/blink-dev/CAOMQ%2Bw-txxAerRT7rpqU7Ac9mCu2t%2BK8OwVhP3rPwVF3VPVnvw%40mail.gmail.com?utm_medium=email&utm_source=footer> > . > -- Hayato -- 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/CAFpjS_1992%2Brq_QRJ2tr5L-_GoQZe19U28GtEEmOV3K3o554hQ%40mail.gmail.com.
