Hey! So I tested by stopping raising the exceptions (and executing our new normal behaviour) and the behavior is still the same for https://maisonyoko.com/ do you see the same for Safari?
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 <debad...@gmail.com> > 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 <debad...@gmail.com> >>>> 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 < >>>>>> debad...@gmail.com> 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 < >>>>>>>>> debad...@gmail.com> 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 < >>>>>>>>>>> debad...@gmail.com> 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 >>>>>>>>>>>> abot...@igalia.com 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 blink-dev+unsubscr...@chromium.org. To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/blink-dev/125536f4-e8ee-4582-a578-6030981789a4n%40chromium.org.
