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.
