LadybirdBrowser / ladybird

Truly independent web browser
https://ladybird.org
BSD 2-Clause "Simplified" License
22.36k stars 994 forks source link

LibWeb: Use correct comparison logic in `NamedNodeMap::get_attribute()` #2469

Closed tcl3 closed 2 days ago

tcl3 commented 5 days ago

Previously, we were doing a case insensitive comparison rather than converting tha attribute name to lower case then comparing.

Fixes 2 subtests in: http://wpt.live/dom/nodes/attributes.html

yyny commented 5 days ago

Adding the new condition in the if (...) statement as the value of the old compare_as_lowercase variable would still (presumably) fix the test without allocating a new string.

tcl3 commented 5 days ago

Adding the new condition in the if (...) statement as the value of the old compare_as_lowercase variable would still (presumably) fix the test without allocating a new string.

I don't think just adding the new associated_element().document().is_html_document() condition to the old code would improve the imported test, if that's the change you're suggesting?

I've split my changes into 2 commits The first ensures a lower case comparison, which improves the imported test and the second adds the is_html_document() condition, which doesn't affect the imported test, but does more closely follow the spec.

It's definitely possible to do this without allocating a new string. I thought it best to go for an implementation which more closely follows the spec text.

yyny commented 5 days ago

I don't think just adding the new associated_element().document().is_html_document() condition to the old code would improve the imported test, if that's the change you're suggesting?

Ah, yes, you are right. If attribute->name() contains uppercase, then the comparison should fail.

Using a small string allocation seems totally fine, then. I will add "Lowercased stringviews" to my idea list for my Unicode string views PR :^)

awesomekling commented 4 days ago

Is it strictly necessary to allocate the string here? Attribute lookup is frequently visible in profiles, and most documents are HTML documents.

tcl3 commented 4 days ago

Is it strictly necessary to allocate the string here? Attribute lookup is frequently visible in profiles, and most documents are HTML documents.

I've redone this in a way that makes no allocations.