hello! I checked in safari technology preview pretty weird I dont observe the same breaking behaviour, I am trying to investigate why that is so!
On Tuesday, June 6, 2023 at 8:49:22 PM UTC+5:30 Debadree Chatterjee wrote: > Thank you so much for finding out simon!! so then nuxt is probably not to > blame here > > On Tuesday, June 6, 2023 at 8:39:51 PM UTC+5:30 Simon Lecutiez wrote: > >> Hello, >> >> > I am still analyzing if this is a library issue or maybe its some >> specific style of writing nuxt >> >> Both websites were made by the same agency (Akaru <https://akaru.fr/>), >> so it could even be a homemade library or a single developer’s style ;) >> >> Hope it helps, >> Simon >> >> Le lundi 5 juin 2023 à 17:07:47 UTC+2, Debadree Chatterjee a écrit : >> >>> Hey! >>> >>> I have attached the difference in how they look for >>> ttps://maisonyoko.com/ <https://maisonyoko.com/> note that the >>> background video doesn't load maybe since for local testing I am raising >>> exceptions thats why its the causing issue (I am checking this) as for the >>> nuxt issue I am still analyzing if this is a library issue or maybe its >>> some specific style of writing nuxt >>> >>> As for the CL i shall then include both the runtime flag and use counter >>> in the same CL hopefully by tomorrow >>> >>> Regards, >>> Debadree >>> On Monday, June 5, 2023 at 7:47:44 PM UTC+5:30 Philip Jägenstedt wrote: >>> >>>> Hi Debadree, >>>> >>>> I also noticed the code was similar for multiple sites, I'm glad you >>>> were able to track it down to a common framework, Nuxt. I would suggest >>>> filing an issue at https://github.com/nuxt/nuxt about this problem, >>>> maybe they can fix it in the next release. Since you've confirmed that >>>> some >>>> sites won't load the video correctly, and fixing it would require >>>> upgrading >>>> the framework, it's important to have a fix out there by the time the >>>> change is done in Chrome. >>>> >>>> Since the WebKit change <https://github.com/WebKit/WebKit/pull/13500> >>>> was included in STP 171 >>>> <https://developer.apple.com/documentation/safari-technology-preview-release-notes/stp-release-171>, >>>> >>>> I tested https://maisonyoko.com/ and https://crossfitdespentes.fr/ in >>>> both Safari and STP 171, but I couldn't see any difference. Which video >>>> should I expect to not load as a result of this change? I was expecting >>>> something to be broken, and then I'd file a WebKit regression bug about it. >>>> >>>> As for whether to bake the changes into a single CL or to split it, I >>>> don't have a strong opinion. >>>> >>>> Best regards, >>>> Philip >>>> >>>> On Sun, Jun 4, 2023 at 9:05 PM Debadree Chatterjee <[email protected]> >>>> wrote: >>>> >>>>> Hey! >>>>> >>>>> So I did a more thorough testing! and it seems there is something >>>>> common in both these two sites they are using the Nuxt Framework and the >>>>> breakage in both of them is similar they have videos on the first section >>>>> of their landing page with animations on top of them, due to the breakage >>>>> it seems the video doesn't load! If you look at the call sites too the >>>>> code >>>>> looks very similar >>>>> >>>>> for https://maisonyoko.com/ >>>>> [image: Screenshot 2023-06-05 at 12.24.34 AM.png] >>>>> >>>>> for https://crossfitdespentes.fr/ >>>>> [image: Screenshot 2023-06-05 at 12.25.09 AM.png] >>>>> >>>>> Both of them nuxt >>>>> [image: Screenshot 2023-06-05 at 12.27.12 AM.png] >>>>> [image: Screenshot 2023-06-05 at 12.27.50 AM.png] >>>>> >>>>> I am not familiar with nuxt I will try to get to know about it, but it >>>>> seems like there could be some problem with nuxt, If any reader here >>>>> knows >>>>> nuxt would love to reach out! >>>>> >>>>> Thank You >>>>> Debadree >>>>> On Wednesday, May 31, 2023 at 9:38:18 PM UTC+5:30 Debadree Chatterjee >>>>> wrote: >>>>> >>>>>> Hey Philip! >>>>>> >>>>>> In my initial testing, I didn't see any observable change in site >>>>>> behavior, but I shall confirm once again! >>>>>> >>>>>> As for the kill switch and use counter should I update my existing CL >>>>>> to include these or make a new one containing the counter to just >>>>>> measure >>>>>> the effects? >>>>>> >>>>>> >>>>>> On Wednesday, May 31, 2023 at 9:28:39 PM UTC+5:30 Philip Jägenstedt >>>>>> wrote: >>>>>> >>>>>>> Hi Debadree, >>>>>>> >>>>>>> Thank you so much for your hard work on this. >>>>>>> >>>>>>> To confirm, these two sites were from the 20 listed earlier in this >>>>>>> thread, is that right? Now that we've confirmed that these two sites >>>>>>> will >>>>>>> have a different behavior, can you observe any difference on >>>>>>> https://maisonyoko.com/ or https://crossfitdespentes.fr/ with the >>>>>>> changes, but without the exception-throwing? >>>>>>> >>>>>>> Generally, finding a behavior change in 10% of tested sites is a bit >>>>>>> concerning, but if it means that only ~10% of the cases hit by the use >>>>>>> counter >>>>>>> <https://chromestatus.com/metrics/feature/timeline/popularity/4478> >>>>>>> are problematic, it could be <0.001% of sites, and we've >>>>>>> successfully made breaking changes at that level in the past. >>>>>>> >>>>>>> Since you now have the code for throwing an exception, would it be >>>>>>> straightforward to turn that into a use counter that we can land and >>>>>>> get a >>>>>>> better measure of this? I think as discussed previously in this thread, >>>>>>> we >>>>>>> should considering shipping this with a killswitch and a use counter, >>>>>>> so we >>>>>>> can both revert and check the usage if we get reports of breakage. >>>>>>> >>>>>>> Best regards, >>>>>>> Philip >>>>>>> >>>>>>> On Tue, May 30, 2023 at 5:16 PM Debadree Chatterjee < >>>>>>> [email protected]> wrote: >>>>>>> >>>>>>>> Hey Philip! >>>>>>>> >>>>>>>> Even I was surprised, turns out I was wrong about the delete >>>>>>>> function (so sorry), I have observed a breakage now, >>>>>>>> >>>>>>>> The has function output example: >>>>>>>> [image: Screenshot 2023-05-30 at 7.36.36 PM.png] >>>>>>>> >>>>>>>> >>>>>>>> The updated delete function: >>>>>>>> >>>>>>>> ```c++ >>>>>>>> void URLSearchParams::deleteAllWithNameOrTuple(const String& name, >>>>>>>> const String& value, >>>>>>>> ExceptionState& exception_state) { >>>>>>>> std::vector<int> indices_to_remove_with_name_val, >>>>>>>> indices_to_remove_with_name; >>>>>>>> for (wtf_size_t i = 0; i < params_.size(); i++) { >>>>>>>> if (params_[i].first == name && >>>>>>>> (value.IsNull() || params_[i].second == value)) { >>>>>>>> indices_to_remove_with_name_val.push_back(i); >>>>>>>> } >>>>>>>> } >>>>>>>> >>>>>>>> for (wtf_size_t i = 0; i < params_.size(); i++) { >>>>>>>> if (params_[i].first == name) { >>>>>>>> indices_to_remove_with_name.push_back(i); >>>>>>>> } >>>>>>>> } >>>>>>>> >>>>>>>> if (indices_to_remove_with_name_val.size() != >>>>>>>> indices_to_remove_with_name.size()) { >>>>>>>> DLOG(ERROR) << "indices_to_remove_with_name_val.size() != >>>>>>>> indices_to_remove_with_name.size()"; >>>>>>>> exception_state.ThrowException(1, "Divergent behavior"); >>>>>>>> return; >>>>>>>> } >>>>>>>> >>>>>>>> // match the values of indices_to_remove_with_name_val, >>>>>>>> indices_to_remove_with_name >>>>>>>> for (size_t i = 0; i < indices_to_remove_with_name_val.size(); >>>>>>>> i++) { >>>>>>>> if (indices_to_remove_with_name_val[i] != >>>>>>>> indices_to_remove_with_name[i]) { >>>>>>>> DLOG(ERROR) << "indices_to_remove_with_name_val[i] != >>>>>>>> indices_to_remove_with_name[i]"; >>>>>>>> exception_state.ThrowException(1, "Divergent behavior"); >>>>>>>> return; >>>>>>>> } >>>>>>>> } >>>>>>>> >>>>>>>> for (wtf_size_t i = 0; i < params_.size();) { >>>>>>>> if (params_[i].first == name && >>>>>>>> (value.IsNull() || params_[i].second == value)) { >>>>>>>> params_.EraseAt(i); >>>>>>>> } else { >>>>>>>> i++; >>>>>>>> } >>>>>>>> } >>>>>>>> >>>>>>>> RunUpdateSteps(); >>>>>>>> } >>>>>>>> ``` >>>>>>>> The example outputs of the delete function: >>>>>>>> [image: Screenshot 2023-05-30 at 8.39.03 PM.png] >>>>>>>> >>>>>>>> Example Breakages: >>>>>>>> In https://maisonyoko.com/ >>>>>>>> [image: Screenshot 2023-05-30 at 8.33.59 PM.png] >>>>>>>> https://crossfitdespentes.fr/ >>>>>>>> [image: Screenshot 2023-05-30 at 8.42.01 PM.png] >>>>>>>> Other than these sites didn't notice a breakage. >>>>>>>> >>>>>>>> seems like the breakages seem to be in the delete function only, so >>>>>>>> sorry once again for the mistake before. Do let me know if all this >>>>>>>> looks >>>>>>>> ok. >>>>>>>> >>>>>>>> Thank You, >>>>>>>> Debadree >>>>>>>> >>>>>>>> On Tuesday, May 30, 2023 at 5:34:18 PM UTC+5:30 Philip Jägenstedt >>>>>>>> wrote: >>>>>>>> >>>>>>>>> Hi Debadree, >>>>>>>>> >>>>>>>>> That's very promising! The code looks right to me, but just to be >>>>>>>>> sure, did you verify that the exceptions are thrown in a test case >>>>>>>>> where >>>>>>>>> the 2nd argument makes a difference? It's a bit suspicious when no >>>>>>>>> sites at >>>>>>>>> all threw the exception :) >>>>>>>>> >>>>>>>>> Best regards, >>>>>>>>> Philip >>>>>>>>> >>>>>>>>> On Tue, May 30, 2023 at 11:45 AM Debadree Chatterjee < >>>>>>>>> [email protected]> wrote: >>>>>>>>> >>>>>>>>>> Hey Everyone! >>>>>>>>>> >>>>>>>>>> Sorry for the delays I followed Philip's suggestion on testing if >>>>>>>>>> behavior diverged in these sites, I checked this by throwing >>>>>>>>>> exceptions if >>>>>>>>>> the actual return value is different if I used name only or both >>>>>>>>>> name and >>>>>>>>>> value, I am including the code for reference: >>>>>>>>>> >>>>>>>>>> bool URLSearchParams::has(const String& name, const String& >>>>>>>>>> value, ExceptionState& exception_state) const { bool >>>>>>>>>> found_match_name = false, found_match_name_value = false; for ( >>>>>>>>>> const auto& param : params_) { if (param.first == name) { >>>>>>>>>> found_match_name = true; break; } } for (const auto& param : >>>>>>>>>> params_) { if (param.first == name && (value.IsNull() || >>>>>>>>>> param.second == value)) { found_match_name_value = true; break; >>>>>>>>>> } } if (found_match_name != found_match_name_value) { >>>>>>>>>> exception_state.ThrowException(1, "Divergent behavior"); return >>>>>>>>>> false; } return found_match_name_value; } void >>>>>>>>>> URLSearchParams::deleteAllWithNameOrTuple(const String& name, >>>>>>>>>> const String& value, ExceptionState& exception_state) { for >>>>>>>>>> (wtf_size_t i = 0; i < params_.size();) { bool match_by_name = >>>>>>>>>> params_[i].first == name; bool match_by_value = !value.IsNull() >>>>>>>>>> && params_[i].second == value; if (match_by_name) { if >>>>>>>>>> (match_by_value) { params_.EraseAt(i); } else { // It would have >>>>>>>>>> been deleted if value wasnt there exception_state.ThrowException( >>>>>>>>>> 1, "Divergent behavior"); return; } } else { i++; } } >>>>>>>>>> RunUpdateSteps(); } >>>>>>>>>> >>>>>>>>>> The good news is none of the example sites broke or triggered >>>>>>>>>> this exception at all, I navigated lightly through all the sites but >>>>>>>>>> no >>>>>>>>>> exception was observed, whereas if I raised an exception whenever >>>>>>>>>> double >>>>>>>>>> variables are passed all the sites would give an exception as you >>>>>>>>>> have seen >>>>>>>>>> in the previous mails. Do let me know if this seems correct! >>>>>>>>>> >>>>>>>>>> Regards, >>>>>>>>>> Debadree >>>>>>>>>> >>>>>>>>>> On Wednesday, May 24, 2023 at 10:35:59 PM UTC+5:30 Debadree >>>>>>>>>> Chatterjee wrote: >>>>>>>>>> >>>>>>>>>>> Understood! >>>>>>>>>>> >>>>>>>>>>> I am going with the local testing approach for now, I think what >>>>>>>>>>> I will do is raise exceptions if a difference in behavior is noted >>>>>>>>>>> as >>>>>>>>>>> Philip suggested, and see how many of these example sites raise >>>>>>>>>>> them. This >>>>>>>>>>> may take a little bit of time I think but trying my best! >>>>>>>>>>> >>>>>>>>>>> Thank You! >>>>>>>>>>> >>>>>>>>>>> On Wednesday, May 24, 2023 at 9:13:04 PM UTC+5:30 Philip >>>>>>>>>>> Jägenstedt wrote: >>>>>>>>>>> >>>>>>>>>>>> If refining the use counter is easy, that would be good to do, >>>>>>>>>>>> even if we don't block shipping on getting stable data for the use >>>>>>>>>>>> counter. >>>>>>>>>>>> >>>>>>>>>>>> But I think that careful local testing is the best way to get a >>>>>>>>>>>> sense for the risk on this. If you're confident you've hit the >>>>>>>>>>>> code path on >>>>>>>>>>>> the sites in question, and nothing at all changes for the user, >>>>>>>>>>>> then I >>>>>>>>>>>> think we should try to ship this. >>>>>>>>>>>> >>>>>>>>>>>> On Mon, May 22, 2023 at 6:59 PM Debadree Chatterjee < >>>>>>>>>>>> [email protected]> wrote: >>>>>>>>>>>> >>>>>>>>>>>>> For basic testing of the sites, I saw no breaking behavior, I >>>>>>>>>>>>> did a few actions on sites like adding things to the cart, trying >>>>>>>>>>>>> to go the >>>>>>>>>>>>> login flow clicking on navigation, etc. Although I think would >>>>>>>>>>>>> need to go a >>>>>>>>>>>>> little deep on that, Should I submit a new CL for this counter >>>>>>>>>>>>> thing? or do >>>>>>>>>>>>> deeper local testing? >>>>>>>>>>>>> >>>>>>>>>>>>> On Monday, May 22, 2023 at 10:09:26 PM UTC+5:30 Philip >>>>>>>>>>>>> Jägenstedt wrote: >>>>>>>>>>>>> >>>>>>>>>>>>>> Well, this is a tricky case with no obvious answer. You've >>>>>>>>>>>>>> found one case of array.some(...), which most likely will change >>>>>>>>>>>>>> the >>>>>>>>>>>>>> behavior of the code. For the other cases where a second >>>>>>>>>>>>>> argument is passed >>>>>>>>>>>>>> is explicitly, it depends on the value whether it changes >>>>>>>>>>>>>> behavior, if it's >>>>>>>>>>>>>> the same value that was added, then it's fine. >>>>>>>>>>>>>> >>>>>>>>>>>>>> One concrete thing you could do is to refine the use counter >>>>>>>>>>>>>> to only count the cases where the 2nd argument results in has() >>>>>>>>>>>>>> returning >>>>>>>>>>>>>> false instead of true, or where delete() doesn't delete anything >>>>>>>>>>>>>> but would >>>>>>>>>>>>>> without the 2nd argument. However, I'm not sure that would be >>>>>>>>>>>>>> informative, >>>>>>>>>>>>>> if it reduces the use counter by 10x we'd still be unsure about >>>>>>>>>>>>>> how serious >>>>>>>>>>>>>> the breakage is to users. >>>>>>>>>>>>>> >>>>>>>>>>>>>> In your manual testing of these sites, were you able to >>>>>>>>>>>>>> confirm the code paths were taken, and unable to spot anything >>>>>>>>>>>>>> at all >>>>>>>>>>>>>> broken on the pages? Did you compare to how the sites work >>>>>>>>>>>>>> without the >>>>>>>>>>>>>> changes? >>>>>>>>>>>>>> >>>>>>>>>>>>>> I would say that given testing of sites that hit the code >>>>>>>>>>>>>> path, if you can't find anything at all breaking, then we should >>>>>>>>>>>>>> try to >>>>>>>>>>>>>> ship the change. >>>>>>>>>>>>>> >>>>>>>>>>>>>> On Fri, May 19, 2023 at 3:40 PM Debadree Chatterjee < >>>>>>>>>>>>>> [email protected]> wrote: >>>>>>>>>>>>>> >>>>>>>>>>>>>>> I tried navigating and clicking around the sites, but they >>>>>>>>>>>>>>> didn't seem to be breaking atleast even though this exception >>>>>>>>>>>>>>> is being >>>>>>>>>>>>>>> raised. Are there any more investigations I can do? >>>>>>>>>>>>>>> >>>>>>>>>>>>>>> On Friday, May 19, 2023 at 3:59:21 AM UTC+5:30 >>>>>>>>>>>>>>> [email protected] wrote: >>>>>>>>>>>>>>> >>>>>>>>>>>>>>>> As for having a premonition that this would be added, there >>>>>>>>>>>>>>>> is at least one post in the original Github issue saying that >>>>>>>>>>>>>>>> the poster >>>>>>>>>>>>>>>> already expected the two-argument overload to be supported ( >>>>>>>>>>>>>>>> https://github.com/whatwg/url/issues/335#issuecomment-919700370 >>>>>>>>>>>>>>>> ). >>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>> Andreu >>>>>>>>>>>>>>>> On 5/18/23 23:42, PhistucK wrote: >>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>> Most of them are just weird, really. I can only imagine >>>>>>>>>>>>>>>> they started with a .set with an empty string as a second >>>>>>>>>>>>>>>> parameter and ended up changing to .delete without >>>>>>>>>>>>>>>> deleting the second parameter. >>>>>>>>>>>>>>>> (Or they had a premonition and knew there will be a second >>>>>>>>>>>>>>>> parameter with the specific purpose you want to ship hehe) >>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>> I imagine those were outliers, I would not worry much about >>>>>>>>>>>>>>>> it (also the bound callback is a bit too convoluted to be >>>>>>>>>>>>>>>> widely used), but >>>>>>>>>>>>>>>> that is just me. :) >>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>> -- 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 [email protected]. To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/blink-dev/8187b2db-1480-4860-ac8f-94d173eae8e3n%40chromium.org.
