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/
Screenshot 2023-06-05 at 12.24.34 AM.png
for https://crossfitdespentes.fr/
Screenshot 2023-06-05 at 12.25.09 AM.png
Both of them nuxt
Screenshot 2023-06-05 at 12.27.12 AM.png
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:
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:
Screenshot 2023-05-30 at 8.39.03 PM.png
Example Breakages:
In https://maisonyoko.com/
Screenshot 2023-05-30 at 8.33.59 PM.png
https://crossfitdespentes.fr/
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/354b0282-d2c8-4ed6-9d3c-a990fda9569bn%40chromium.org
<https://groups.google.com/a/chromium.org/d/msgid/blink-dev/354b0282-d2c8-4ed6-9d3c-a990fda9569bn%40chromium.org?utm_medium=email&utm_source=footer>.
