Bug 80897

Summary: move calc*Value functions out from Length (and platform)
Product: WebKit Reporter: Joe Thomas <joethomas>
Component: CSSAssignee: Joe Thomas <joethomas>
Status: RESOLVED FIXED    
Severity: Normal CC: cc-bugs, jamesr, joethomas, kling, koivisto, macpherson, menard, mikelawther, rakuco, shanestephens, tkent, webkit.review.bot
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: All   
OS: All   
Bug Depends on:    
Bug Blocks: 27160    
Attachments:
Description Flags
ProposedPatch
koivisto: review-, koivisto: commit-queue-
Patch-updated
koivisto: review+, webkit.review.bot: commit-queue-
Patch-Rebaselined
none
Patch none

Description Joe Thomas 2012-03-12 15:12:58 PDT
This refactoring is basically to avoid the use of virtual interface. Otherwise virtual interface has to be added for avoiding the layering violation.

Relevant Discussion: https://bugs.webkit.org/show_bug.cgi?id=27160#c68
Comment 1 Joe Thomas 2012-03-12 15:14:04 PDT
Patch forthcoming
Comment 2 Joe Thomas 2012-03-14 01:06:25 PDT
Created attachment 131803 [details]
ProposedPatch
Comment 3 Antti Koivisto 2012-03-14 09:54:09 PDT
Comment on attachment 131803 [details]
ProposedPatch

View in context: https://bugs.webkit.org/attachment.cgi?id=131803&action=review

Looks generally good, a few comments.

> Source/WebCore/css/LengthFunctions.h:3
> +/*
> + * Copyright (c) 2012 Motorola Mobility, Inc. All rights reserved.
> + *

Since this is copy code you should maintain the original license and copyrights from Length.h and just add a new (c) line.

> Source/WebCore/css/LengthFunctions.h:36
> +int calculateValue(Length, int maxValue, bool roundPercentages = false);
> +int calculateMinValue(Length, int maxValue, bool roundPercentages = false);
> +float calculateFloatValue(Length, float maxValue);
> +float calculateFloatValue(Length, int maxValue);

It would be better to put everything to header as the current implementations are inlined in Length.h. Making them regular functions might not affect performance but it would be safer to do that change separately.

Less generic names would be nice though I don't have good suggestions.

calculateMinValue, minValue, maxValue should be renamed with "minimum" and "maximum"
Comment 4 Joe Thomas 2012-03-14 22:16:16 PDT
Created attachment 131986 [details]
Patch-updated

While sorting Xcode Project file after adding LengthFunctions.h using sort-Xcode-project-file script, it also sorted some of the previously misplaced entries and they are included in this patch. Should I create a separate patch for that?
Comment 5 Joe Thomas 2012-03-14 22:17:49 PDT
(In reply to comment #3)
> (From update of attachment 131803 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=131803&action=review
> 
> Looks generally good, a few comments.
> 
> > Source/WebCore/css/LengthFunctions.h:3
> > +/*
> > + * Copyright (c) 2012 Motorola Mobility, Inc. All rights reserved.
> > + *
> 
> Since this is copy code you should maintain the original license and copyrights from Length.h and just add a new (c) line.

Done.

> 
> > Source/WebCore/css/LengthFunctions.h:36
> > +int calculateValue(Length, int maxValue, bool roundPercentages = false);
> > +int calculateMinValue(Length, int maxValue, bool roundPercentages = false);
> > +float calculateFloatValue(Length, float maxValue);
> > +float calculateFloatValue(Length, int maxValue);
> 
> It would be better to put everything to header as the current implementations are inlined in Length.h. Making them regular functions might not affect performance but it would be safer to do that change separately.
>

Done.
 
> Less generic names would be nice though I don't have good suggestions.
> 
> calculateMinValue, minValue, maxValue should be renamed with "minimum" and "maximum"

Done.
Comment 6 Antti Koivisto 2012-03-15 03:39:16 PDT
Comment on attachment 131986 [details]
Patch-updated

Looks good, r=me
Comment 7 Joe Thomas 2012-03-15 06:44:30 PDT
Can someone please help me to land the patch? Thanks
Comment 8 Joe Thomas 2012-03-15 14:44:33 PDT
looks like commit-queue has stopped running. It is at commit #15 for last 6 hrs.
Comment 9 Luke Macpherson 2012-03-15 16:01:26 PDT
Could you please address Antti's comment:
"Less generic names would be nice though I don't have good suggestions." before committing this code?
These names are too generic to sit in the WebCore namespace in my opinion.

Ultimately I think that these functions belong in a new class eg. CSSLength, that has an instance of platform/Length used for storage.
Comment 10 Andreas Kling 2012-03-15 16:12:46 PDT
Comment on attachment 131986 [details]
Patch-updated

cq- per Luke's comment.
Comment 11 Joe Thomas 2012-03-15 16:23:26 PDT
(In reply to comment #9)
> Could you please address Antti's comment:
> "Less generic names would be nice though I don't have good suggestions." before committing this code?
> These names are too generic to sit in the WebCore namespace in my opinion.
> 
> Ultimately I think that these functions belong in a new class eg. CSSLength, that has an instance of platform/Length used for storage.

I could not think of any better names, so I just renamed 'min' and 'max' to 'minimum' and 'maximum' as per Antti's suggestion.

How about 'calculateLengthValue' & 'calculateLengthMinimumValue' OR 'calculateCSSLengthValue' & 'calculateCSSLengthMinimumValue'? I think it is getting too lengthy. 

Do you have any other better names?
Comment 12 Antti Koivisto 2012-03-15 16:40:28 PDT
It you look at the full function signature it is not a very generic at all (compare to operator== in global namespace). If you have suggestions please present them. 

Adding a pointless class is as suggested by Luke is clearly not a good option.
Comment 13 Antti Koivisto 2012-03-15 16:47:17 PDT
My "generic name" comment was about the fact that there is no indication what sort of value is being calculated. The current code has the same problem.
Comment 14 Luke Macpherson 2012-03-15 16:48:56 PDT
(In reply to comment #13)
> My "generic name" comment was about the fact that there is no indication what sort of value is being calculated. The current code has the same problem.

I'm ok with calculateMinimumLength / calculateLength etc (the word "value" is almost meaningless here), but a better alternative might be to introduce a C++ namespace around these static functions, and then make the names even more generic.
Comment 15 Antti Koivisto 2012-03-15 16:53:40 PDT
I would suggest we land this as-is and bikeshed later. Hyatt ok'd this already as well. Kling?
Comment 16 Antti Koivisto 2012-03-15 16:55:26 PDT
...unless someone actually has good name suggestions right now.
Comment 17 Luke Macpherson 2012-03-15 17:17:32 PDT
(In reply to comment #16)
> ...unless someone actually has good name suggestions right now.

Suggestions already provided. See comment #14.
Comment 18 Andreas Kling 2012-03-16 13:31:24 PDT
Comment on attachment 131986 [details]
Patch-updated

Sure, let's bikeshed another day.
Comment 19 WebKit Review Bot 2012-03-16 13:55:54 PDT
Comment on attachment 131986 [details]
Patch-updated

Rejecting attachment 131986 [details] from commit-queue.

Failed to run "['/mnt/git/webkit-commit-queue/Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '-..." exit_code: 2

Last 500 characters of output:
enderStyle.h
patching file Source/WebCore/rendering/svg/RenderSVGRoot.cpp
patching file Source/WebCore/svg/SVGSVGElement.cpp
patching file Source/WebCore/svg/graphics/SVGImage.cpp
patching file Source/WebKit2/ChangeLog
Hunk #1 succeeded at 1 with fuzz 3.
patching file Source/WebKit2/WebProcess/WebCoreSupport/win/WebPopupMenuWin.cpp

Failed to run "[u'/mnt/git/webkit-commit-queue/Tools/Scripts/svn-apply', u'--force', u'--reviewer', u'Antti Koiv..." exit_code: 1 cwd: /mnt/git/webkit-commit-queue/

Full output: http://queues.webkit.org/results/11966373
Comment 20 Joe Thomas 2012-03-16 14:14:07 PDT
I think it is failed due to merge-conflict. I will do a rebase and submit the patch.
Comment 21 Joe Thomas 2012-03-16 20:56:00 PDT
Created attachment 132441 [details]
Patch-Rebaselined

Patch updated to trunk.
Comment 22 Joe Thomas 2012-03-16 22:26:11 PDT
(In reply to comment #4)
> Created an attachment (id=131986) [details]
> Patch-updated
> 
> While sorting Xcode Project file after adding LengthFunctions.h using sort-Xcode-project-file script, it also sorted some of the previously misplaced entries and they are included in this patch. Should I create a separate patch for that?

Bug 81439 is created to sort the previously misplaced entries in Xcode project
Comment 23 Antti Koivisto 2012-03-17 05:08:47 PDT
Hmm, valueForLength(), miminumValueForLength(), floatValueForLength() might be better names. They drop the superfluous "calculate" in favor of mentioning the type operated on. What do you think?
Comment 24 Joe Thomas 2012-03-17 08:23:11 PDT
(In reply to comment #23)
> Hmm, valueForLength(), miminumValueForLength(), floatValueForLength() might be better names. They drop the superfluous "calculate" in favor of mentioning the type operated on. What do you think?

Thanks for the suggestion. These names are definitely better. I will create another patch.
Comment 25 Joe Thomas 2012-03-17 08:44:15 PDT
Created attachment 132462 [details]
Patch

Functions renamed.
Comment 26 Antti Koivisto 2012-03-17 12:18:01 PDT
Comment on attachment 132462 [details]
Patch

r=me, thanks
Comment 27 WebKit Review Bot 2012-03-17 12:54:25 PDT
Comment on attachment 132462 [details]
Patch

Clearing flags on attachment: 132462

Committed r111126: <http://trac.webkit.org/changeset/111126>
Comment 28 WebKit Review Bot 2012-03-17 12:54:32 PDT
All reviewed patches have been landed.  Closing bug.
Comment 29 Joe Thomas 2012-03-20 22:42:50 PDT
(In reply to comment #3)

> 
> > Source/WebCore/css/LengthFunctions.h:36
> > +int calculateValue(Length, int maxValue, bool roundPercentages = false);
> > +int calculateMinValue(Length, int maxValue, bool roundPercentages = false);
> > +float calculateFloatValue(Length, float maxValue);
> > +float calculateFloatValue(Length, int maxValue);
> 
> It would be better to put everything to header as the current implementations are inlined in Length.h. Making them regular functions might not affect performance but it would be safer to do that change separately.
> 

Bug 81733 is created to make these functions regular.