clean up CSSPrimitiveValue::computeLengthDouble().
Created attachment 124052 [details] Patch
Comment on attachment 124052 [details] Patch Unofficial r=me (not a reviewer yet)
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.
(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.
Created attachment 124262 [details] Patch
Just wanted to ping this patch to see if someone will review the second patch.
Created attachment 129180 [details] Patch
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?)
(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.
Comment on attachment 129180 [details] Patch OK.
Comment on attachment 129180 [details] Patch Clearing flags on attachment: 129180 Committed r109279: <http://trac.webkit.org/changeset/109279>
All reviewed patches have been landed. Closing bug.