Last Comment Bug 820902 - nsAttrValue::Equals should do ASCII-case-insensitive compares
: nsAttrValue::Equals should do ASCII-case-insensitive compares
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: DOM (show other bugs)
: unspecified
: x86 Mac OS X
: -- normal (vote)
: mozilla20
Assigned To: Boris Zbarsky [:bz] (still a bit busy)
:
: Andrew Overholt [:overholt]
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2012-12-12 09:10 PST by Boris Zbarsky [:bz] (still a bit busy)
Modified: 2012-12-15 13:32 PST (History)
3 users (show)
bzbarsky: in‑testsuite+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
nsAttrValue::Equals should do ASCII-case-insensitive compares when it's doing case-insensitive compares. (4.95 KB, patch)
2012-12-12 11:34 PST, Boris Zbarsky [:bz] (still a bit busy)
jonas: review+
Details | Diff | Splinter Review
Interdiff to fix the test issue (5.27 KB, patch)
2012-12-13 19:44 PST, Boris Zbarsky [:bz] (still a bit busy)
jonas: review+
Details | Diff | Splinter Review

Description Boris Zbarsky [:bz] (still a bit busy) 2012-12-12 09:10:28 PST
Right now it does nsCaseInsensitiveStringComparator, which is all wacky.
Comment 1 Boris Zbarsky [:bz] (still a bit busy) 2012-12-12 10:59:36 PST
Testcase at http://people.mozilla.org/~jdaggett/tests/casesensitivity-classid.html
Comment 2 Boris Zbarsky [:bz] (still a bit busy) 2012-12-12 11:34:53 PST
Created attachment 691461 [details] [diff] [review]
nsAttrValue::Equals should do ASCII-case-insensitive compares when it's doing case-insensitive compares.
Comment 3 Boris Zbarsky [:bz] (still a bit busy) 2012-12-13 14:22:11 PST
https://hg.mozilla.org/integration/mozilla-inbound/rev/66f91a854410
Comment 5 Boris Zbarsky [:bz] (still a bit busy) 2012-12-13 19:07:36 PST
Looks like at least some of that test expects the Ü key to match the ü accesskey or vice versa.

I just went through all the consumers of eIgnoreCase with AttrValueIs, FindAttrValueIn, and nsAttrValue::Equals, and most of them just pass in an ASCII literal or atom to compare to.  The exceptions are:

1) nsDocument::FindImageMap.  Per spec this needs to be doing Unicode "compatibility
   caseless" matching at least in some cases, so it doesn't want the ASCII behavior
   anyway.
2) IsAccessKeyTarget in the ESM.  Per spec I think this should be case-sensitive, but I'm
   not going to mess with it here.
3) nsHTMLEditor::IsSimpleModifiableNode.    I have no idea what this is trying to do.
Comment 6 Boris Zbarsky [:bz] (still a bit busy) 2012-12-13 19:44:08 PST
Created attachment 692137 [details] [diff] [review]
Interdiff to fix the test issue
Comment 7 Jonas Sicking (:sicking) No longer reading bugmail consistently 2012-12-14 00:28:52 PST
Comment on attachment 692137 [details] [diff] [review]
Interdiff to fix the test issue

Review of attachment 692137 [details] [diff] [review]:
-----------------------------------------------------------------

I don't understand why we can't fix eIgnoreCase to do "real" ascii-case-insensitive compares instead?

In general, I'd prefer to get rid of the pseudo-ascii-case-insensitive code that we have rather than trying to migrate callers between the various ways of doing comparisons.

Do we really have a need for pseudo-ascii-case-insensitive anywhere?
Comment 8 Boris Zbarsky [:bz] (still a bit busy) 2012-12-14 00:35:44 PST
> I don't understand why we can't fix eIgnoreCase to do "real" ascii-case-insensitive
> compares instead?

I did, then when I landed that patch it failed tests that expected a full-on Unicode compare for accesskeys.  So I looked at all consumers of eIgnoreCase, and found that there are three callsites who actually want Unicode case-insensitivity, so they have to stop using eIgnoreCase.  That's what the interdiff is about.  This is in addition to the first patch, which makes eIgnoreCase do ascii-case-insensitive stuff; I thought the comments in the interdiff made that clear....

> Do we really have a need for pseudo-ascii-case-insensitive anywhere?

I'm not sure what you're asking.  What's "pseudo-ascii-case-insensitive"?
Comment 9 Jonas Sicking (:sicking) No longer reading bugmail consistently 2012-12-14 00:38:02 PST
(In reply to Boris Zbarsky (:bz) from comment #8)
> > I don't understand why we can't fix eIgnoreCase to do "real" ascii-case-insensitive
> > compares instead?
> 
> I did, then when I landed that patch it failed tests that expected a full-on
> Unicode compare for accesskeys.

Ah, makes sense.

> I'm not sure what you're asking.  What's "pseudo-ascii-case-insensitive"?

It's the crazy "ascii case insensitive plus special rules for kelvin and dotted I" that we apparently are using in some places.
Comment 10 Boris Zbarsky [:bz] (still a bit busy) 2012-12-14 00:39:58 PST
> It's the crazy "ascii case insensitive plus special rules for kelvin and dotted I" that
> we apparently are using in some places.

Ah.  Yeah, bug 820909.  I'm going to try to look at consumers of that too; just haven't had a chance to yet.  My gut feeling is likewise that no consumer actually wants that behavior.
Comment 11 Boris Zbarsky [:bz] (still a bit busy) 2012-12-14 13:13:44 PST
https://hg.mozilla.org/integration/mozilla-inbound/rev/7b9ffa4e1e64
Comment 12 Ryan VanderMeulen [:RyanVM] 2012-12-15 13:32:11 PST
https://hg.mozilla.org/mozilla-central/rev/7b9ffa4e1e64

Note You need to log in before you can comment on or make changes to this bug.