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
Patch forthcoming
Created attachment 131803 [details] ProposedPatch
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"
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?
(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 on attachment 131986 [details] Patch-updated Looks good, r=me
Can someone please help me to land the patch? Thanks
looks like commit-queue has stopped running. It is at commit #15 for last 6 hrs.
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 on attachment 131986 [details] Patch-updated cq- per Luke's comment.
(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?
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.
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.
(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.
I would suggest we land this as-is and bikeshed later. Hyatt ok'd this already as well. Kling?
...unless someone actually has good name suggestions right now.
(In reply to comment #16) > ...unless someone actually has good name suggestions right now. Suggestions already provided. See comment #14.
Comment on attachment 131986 [details] Patch-updated Sure, let's bikeshed another day.
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
I think it is failed due to merge-conflict. I will do a rebase and submit the patch.
Created attachment 132441 [details] Patch-Rebaselined Patch updated to trunk.
(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
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?
(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.
Created attachment 132462 [details] Patch Functions renamed.
Comment on attachment 132462 [details] Patch r=me, thanks
Comment on attachment 132462 [details] Patch Clearing flags on attachment: 132462 Committed r111126: <http://trac.webkit.org/changeset/111126>
All reviewed patches have been landed. Closing bug.
(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.