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/29a8e7ae-bc90-4d5a-9555-80ddb5666207n%40chromium.org.
