Bug 77065

Summary: Clean up CSSPrimitiveValue::computeLengthDouble().
Product: WebKit Reporter: Luke Macpherson <macpherson>
Component: New BugsAssignee: Luke Macpherson <macpherson>
Status: RESOLVED FIXED    
Severity: Normal CC: eric, kling, macpherson, menard, webkit.review.bot
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Patch
none
Patch
none
Patch none

Description Luke Macpherson 2012-01-25 18:17:39 PST
clean up CSSPrimitiveValue::computeLengthDouble().
Comment 1 Luke Macpherson 2012-01-25 18:19:26 PST
Created attachment 124052 [details]
Patch
Comment 2 Jessie Berlin 2012-01-26 09:01:11 PST
Comment on attachment 124052 [details]
Patch

Unofficial r=me (not a reviewer yet)
Comment 3 Darin Adler 2012-01-26 11:06:02 PST
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.
Comment 4 Luke Macpherson 2012-01-26 21:16:28 PST
(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.
Comment 5 Luke Macpherson 2012-01-26 22:24:19 PST
Created attachment 124262 [details]
Patch
Comment 6 Luke Macpherson 2012-02-27 19:14:25 PST
Just wanted to ping this patch to see if someone will review the second patch.
Comment 7 Luke Macpherson 2012-02-27 20:44:43 PST
Created attachment 129180 [details]
Patch
Comment 8 Eric Seidel (no email) 2012-02-29 00:33:27 PST
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?)
Comment 9 Luke Macpherson 2012-02-29 14:28:10 PST
(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 10 Eric Seidel (no email) 2012-02-29 14:33:46 PST
Comment on attachment 129180 [details]
Patch

OK.
Comment 11 WebKit Review Bot 2012-02-29 16:12:08 PST
Comment on attachment 129180 [details]
Patch

Clearing flags on attachment: 129180

Committed r109279: <http://trac.webkit.org/changeset/109279>
Comment 12 WebKit Review Bot 2012-02-29 16:12:13 PST
All reviewed patches have been landed.  Closing bug.