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 <debadree...@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/CAARdPYeN9QDtdUi%2B14mqUq-2%3DPCUEjh9%3DuhuvEdWOnPA4pOa1A%40mail.gmail.com.
