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/5946a23f-9f0a-4060-b260-d603ff503a08n%40chromium.org.
