Bug 77065 – Clean up CSSPrimitiveValue::computeLengthDouble().

• Component: New Bugs
• Status: RESOLVED FIXED

[Luke Macpherson]

clean up CSSPrimitiveValue::computeLengthDouble().

[Luke Macpherson]

Created attachment 124052 [details]
Patch

[Jessie Berlin]

Comment on attachment 124052 [details]
Patch

Unofficial r=me (not a reviewer yet)

[Darin Adler]

Comment on attachment 124052 [details]
Patch

Some people like to follow a style rule where they don’t modify the values of arguments. It seems that the original author of this function was doing that. In this patch we stop following that style rule, and that eliminates a local variable, but also makes the code seem a bit more oblique to me.

We now also use a hand-written maximum branch instead of the max function. That seems to slightly hurt the code clarity.

I’m not sure these changes are improvements. I don’t feel strongly the other way, so I’m not saying review- but this doesn’t seem great to me.

[Luke Macpherson]

(In reply to comment #3)
> (From update of attachment 124052 [details])
> Some people like to follow a style rule where they don’t modify the values of arguments. It seems that the original author of this function was doing that. In this patch we stop following that style rule, and that eliminates a local variable, but also makes the code seem a bit more oblique to me.

New code is better because:
1) The current code uses two variables to represent what is effectively one piece of state.
2) The state space you have to think about has doubled when analyzing the code.
3) The new code is strictly more efficient, saving a few unnecessary computations.

> We now also use a hand-written maximum branch instead of the max function. That seems to slightly hurt the code clarity.

The new code is better because:
1) The zoomedResult is now only returned from a single, obvious location.
2) It is more efficient because the multiple branches can be skipped when the zoomedResult is > 1.
3) It more clearly reflects the intent of the comment.

> I’m not sure these changes are improvements. I don’t feel strongly the other way, so I’m not saying review- but this doesn’t seem great to me.

It's a small difference, but as someone who looks at this code almost daily, I thought the cleanup was worthwhile.

[Luke Macpherson]

Created attachment 124262 [details]
Patch

[Luke Macpherson]

Just wanted to ping this patch to see if someone will review the second patch.

[Luke Macpherson]

Created attachment 129180 [details]
Patch

[Eric Seidel (no email)]

Comment on attachment 129180 [details]
Patch

It's difficult for me to tell what's going on here.  I guess "isFontRelativeLength" is true for EMS, EXS and REMS (and nothing else?)

[Luke Macpherson]

(In reply to comment #8)
> (From update of attachment 129180 [details])
> It's difficult for me to tell what's going on here.  I guess "isFontRelativeLength" is true for EMS, EXS and REMS (and nothing else?)

That is correct.

[Eric Seidel (no email)]

Comment on attachment 129180 [details]
Patch

OK.

[WebKit Review Bot]

Comment on attachment 129180 [details]
Patch

Clearing flags on attachment: 129180

Committed r109279: <http://trac.webkit.org/changeset/109279>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.