Bug 55202 – Add missing length units to CSS unit category function

• Component: New Bugs
• Status: NEW

[Mike Lawther]

Add missing length units to CSS unit category function

[Mike Lawther]

Created attachment 83768 [details]
Patch

[Andreas Kling]

Comment on attachment 83768 [details]
Patch

How do we test this?

[Mike Lawther]

Normally I'd say a unit test :)

After a chat on IRC with jamesr, I now think that the units are deliberately not there. The output of this function is used to determine whether units are convertible, eg picas->px (ie ULength->ULength) is OK, but khz->px (UFrequency->ULength) is not. 

Looking at the asserts in on line 559 and 563 of CSSPrimitiveValue.cpp, it's currently illegal to call unitCategory() with (em|ex|rem). I reckon this is because there is no simple conversion for them, since (ex|em|rem) depends on font metrics?

I had thought that unitCategory() was a generic function, similar to isUnitTypeLength(), but it appears to have the more specific purpose of identifying categories where the units can be converted to a canonical type (eg px for ULength).

If this is the case, then I reckon that CSS_EMS, CSS_EXS and CSS_REMS should be added to the default case of the switch so they explicitly return UOther, to make it clearer that this is intended, and not a simple omission.

Thoughts?

[Ojan Vafai]

(In reply to comment #3)
> If this is the case, then I reckon that CSS_EMS, CSS_EXS and CSS_REMS should be added to the default case of the switch so they explicitly return UOther, to make it clearer that this is intended, and not a simple omission.

I'd rather see asserts at the top of unitCategory explicitly checking for these types. For now, I'll r- this patch since it seems the existing code is correct, albeit confusing. (right?)

[Ojan Vafai]

Comment on attachment 83768 [details]
Patch

r- per Mike's comment above.

[Mike Lawther]

Yes - my newfound understanding of the current code is that it is correct, but confusing.

(aside: I don't think you want an assert, since it is legal to call unitCategory with these types, and the intent is that they come back UOther).

[Ojan Vafai]

(In reply to comment #6)
> Yes - my newfound understanding of the current code is that it is correct, but confusing.
> 
> (aside: I don't think you want an assert, since it is legal to call unitCategory with these types, and the intent is that they come back UOther).

It's not really legal though since the only two callers of this function assert that it returns something other than a UOther. So to me it's not unlike doing the following (which is in many places in WebKit):

ASSERT(foo);
// foo shouldn't be null here, but early-return to make sure we don't crash if it is.
if (!foo)
  return;

I guess I find it weird to have a default case for the switch but then have a number of cases that do the same thing. Why have a default at all?