Bug 80897 – move calc*Value functions out from Length (and platform)

• Component: CSS
• Status: RESOLVED FIXED

[Joe Thomas]

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

[Joe Thomas]

Patch forthcoming

[Joe Thomas]

Created attachment 131803 [details]
ProposedPatch

[Antti Koivisto]

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"

[Joe Thomas]

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?

[Joe Thomas]

(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.

[Antti Koivisto]

Comment on attachment 131986 [details]
Patch-updated

Looks good, r=me

[Joe Thomas]

Can someone please help me to land the patch? Thanks

[Joe Thomas]

looks like commit-queue has stopped running. It is at commit #15 for last 6 hrs.

[Luke Macpherson]

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.

[Andreas Kling]

Comment on attachment 131986 [details]
Patch-updated

cq- per Luke's comment.

[Joe Thomas]

(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?

[Antti Koivisto]

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.

[Antti Koivisto]

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.

[Luke Macpherson]

(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.

[Antti Koivisto]

I would suggest we land this as-is and bikeshed later. Hyatt ok'd this already as well. Kling?

[Antti Koivisto]

...unless someone actually has good name suggestions right now.

[Luke Macpherson]

(In reply to comment #16)
> ...unless someone actually has good name suggestions right now.

Suggestions already provided. See comment #14.

[Andreas Kling]

Comment on attachment 131986 [details]
Patch-updated

Sure, let's bikeshed another day.

[WebKit Review Bot]

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

[Joe Thomas]

I think it is failed due to merge-conflict. I will do a rebase and submit the patch.

[Joe Thomas]

Created attachment 132441 [details]
Patch-Rebaselined

Patch updated to trunk.

[Joe Thomas]

(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

[Antti Koivisto]

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?

[Joe Thomas]

(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.

[Joe Thomas]

Created attachment 132462 [details]
Patch

Functions renamed.

[Antti Koivisto]

Comment on attachment 132462 [details]
Patch

r=me, thanks

[WebKit Review Bot]

Comment on attachment 132462 [details]
Patch

Clearing flags on attachment: 132462

Committed r111126: <http://trac.webkit.org/changeset/111126>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Joe Thomas]

(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.