We should add a more correct way to compare floating point numbers and use it in the code base. We currently use std::abs(a-b) <= std::numeric_limits<T>::epsilon() in several places. However, this is not correct for arbitrary floating point values, and will fail for values that are not close to zero.
Created attachment 241220 [details] Patch
Attachment 241220 [details] did not pass style-queue: ERROR: Source/WebKit/mac/WebView/WebHTMLView.mm:2416: An else statement can be removed when the prior "if" concludes with a return, break, continue or goto statement. [readability/control_flow] [4] Total errors found: 1 in 11 files If any of these errors are false positives, please file a bug against check-webkit-style.
Created attachment 241242 [details] Patch
Attachment 241242 [details] did not pass style-queue: ERROR: Source/WebKit/mac/WebView/WebHTMLView.mm:2416: An else statement can be removed when the prior "if" concludes with a return, break, continue or goto statement. [readability/control_flow] [4] Total errors found: 1 in 11 files If any of these errors are false positives, please file a bug against check-webkit-style.
Comment on attachment 241242 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=241242&action=review I did not review this patch for correctness. I noticed a minor nit with the citation style and had a question as to the origin of the quotation "very close with tolerance e". > Source/WTF/ChangeLog:19 > + which defines a "very close with tolerance e" relationship between u Do you know what page this quotes comes from? I was unable to find this quotation when searching through the 1981 printing of The Art of Computer Programming, Volume 2 (*) or section 4.2.2 "Accuracy of Floating Point Arithmetic" of the 1998 printing. The only reference I found of this quotation is on <http://www.boost.org/doc/libs/1_34_0/libs/test/doc/components/test_tools/floating_point_comparison.html>. This web page doesn't cite the particular page(s)/section(s) referenced from The Art of Computer Programming, Volume 2 :( You may also want to consider citing the boost.org web page in this ChangeLog entry. (*) A PDF of the 1981 printing of t <http://www.itpa.lt/~acus/Knygos/Donald.E.Knuth%20-%20The%20Art%20of%20Computer%20Programming%20I-III,%20Concrete%20Mathematics,%20The%20Tex%20Book/Addison.Wesley.Donald.E.Knuth.The.Art.of.Computer.Programming.Volume.2.pdf> > Source/WTF/wtf/MathExtras.h:409 > +// Floating point numbers comparison. > +// From Knuth D.E. The art of computer programming (vol II): > +// | u - v | / |u| <= e and | u - v | / |v| <= e > +// defines a "very close with tolerance e" relationship between u and v You may also want to consider citing the boost.org web page, <http://www.boost.org/doc/libs/1_34_0/libs/test/doc/components/test_tools/floating_point_comparison.html>, in this comment. Assuming you are referencing section 4.2.2 "Accuracy of Floating Point Arithmetic" of the The Art of Computer Programming then the MLA-citation would be of the form: Knuth, D. E. "Accuracy of Floating Point Arithmetic." The Art of Computer Programming. 3rd ed. Vol. 2. Boston: Addison-Wesley, 1998. 229-45.
Comment on attachment 241242 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=241242&action=review >> Source/WTF/ChangeLog:19 >> + which defines a "very close with tolerance e" relationship between u > > Do you know what page this quotes comes from? I was unable to find this quotation when searching through the 1981 printing of The Art of Computer Programming, Volume 2 (*) or section 4.2.2 "Accuracy of Floating Point Arithmetic" of the 1998 printing. The only reference I found of this quotation is on <http://www.boost.org/doc/libs/1_34_0/libs/test/doc/components/test_tools/floating_point_comparison.html>. This web page doesn't cite the particular page(s)/section(s) referenced from The Art of Computer Programming, Volume 2 :( You may also want to consider citing the boost.org web page in this ChangeLog entry. > > (*) A PDF of the 1981 printing of t <http://www.itpa.lt/~acus/Knygos/Donald.E.Knuth%20-%20The%20Art%20of%20Computer%20Programming%20I-III,%20Concrete%20Mathematics,%20The%20Tex%20Book/Addison.Wesley.Donald.E.Knuth.The.Art.of.Computer.Programming.Volume.2.pdf> This is on page 218 - 219. Knuth actually uses the terms "essentially equal to" / "approximately equal to". I actually quoted the boost page, which referred to Knuth's book. My proposal would be to use "is essentially equal to" (rename to function to areEssentiallyEqual()) and keep quoting Knuth. What do you think? I will also include a link to the boost page as this is the one I referred to initially. >> Source/WTF/wtf/MathExtras.h:409 >> +// defines a "very close with tolerance e" relationship between u and v > > You may also want to consider citing the boost.org web page, <http://www.boost.org/doc/libs/1_34_0/libs/test/doc/components/test_tools/floating_point_comparison.html>, in this comment. > > Assuming you are referencing section 4.2.2 "Accuracy of Floating Point Arithmetic" of the The Art of Computer Programming then the MLA-citation would be of the form: > > Knuth, D. E. "Accuracy of Floating Point Arithmetic." The Art of Computer Programming. 3rd ed. Vol. 2. Boston: Addison-Wesley, 1998. 229-45. Yes, section 4.2.2 indeed.
Created attachment 241250 [details] Patch
Thanks for the review Daniel. I have updated the patch accordingly I believe.
Attachment 241250 [details] did not pass style-queue: ERROR: Source/WebKit/mac/WebView/WebHTMLView.mm:2416: An else statement can be removed when the prior "if" concludes with a return, break, continue or goto statement. [readability/control_flow] [4] Total errors found: 1 in 11 files If any of these errors are false positives, please file a bug against check-webkit-style.
Comment on attachment 241250 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=241250&action=review This change is OK, but I am really concerned with the fragile way we wrote the code to compare double scale factors in WebKit, compensating for the fact that they are treated as floats inside WebCore. > Source/WTF/wtf/MathExtras.h:399 > + if (v < 1 && u > v * std::numeric_limits<T>::max()) Extra space before the "&&" on this line. > Source/WebKit2/UIProcess/API/Cocoa/WKWebView.mm:833 > +static inline bool withinEpsilon(float a, float b) This function, with its current name and function, is too subtle. The most important thing this function does it convert its arguments to floats, as the comment explains above; it is needed because WebCore stores these values as float, not double. Given that, I think it needs a better name. It’s not just a general purpose "within epsilon" function, but rather one that’s specifically about the "float" type. More specifically, if someone were to overload this to work for doubles as well as floats, or convert it back to a template, that would likely break this, since we’d use a double epsilon rather than a float one. > Source/WebKit2/UIProcess/CoordinatedGraphics/PageViewportController.cpp:325 > + bool currentlyScaledToFit = WTF::areEssentiallyEqual(m_pageScaleFactor, m_minimumScaleToFit, 0.0001f); In both the old code and the new, I wonder where the magic number 0.0001f comes from. > Source/WebKit2/WebProcess/WebPage/ios/WebPageIOS.mm:2217 > + return WTF::areEssentiallyEqual(a, b); We have the same problem here as in WKWebView.mm, but here we don’t even have the comment explaining why converting to float is important. It would be really easy to break this code by overloading or changing the types.
(In reply to comment #6) > [...] > This is on page 218 - 219. Knuth actually uses the terms "essentially equal > to" / "approximately equal to". I actually quoted the boost page, which > referred to Knuth's book. My proposal would be to use "is essentially equal > to" (rename to function to areEssentiallyEqual()) and keep quoting Knuth. > What do you think? Sounds reasonable. > I will also include a link to the boost page as this is > the one I referred to initially. > Good.
Comment on attachment 241250 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=241250&action=review >> Source/WebKit2/UIProcess/CoordinatedGraphics/PageViewportController.cpp:325 >> + bool currentlyScaledToFit = WTF::areEssentiallyEqual(m_pageScaleFactor, m_minimumScaleToFit, 0.0001f); > > In both the old code and the new, I wonder where the magic number 0.0001f comes from. This apparently used to be 0.001f a very long time ago, when this code was Qt-specific. It was updated to 0.0001f in https://bugs.webkit.org/attachment.cgi?id=177492&action=review to "fix flakiness" in scale related API tests. It is very likely we can use here the same code as on Mac and iOS instead of using this arbitrary precision. However, I am not sure I should be the one making this change as I don't have an easy to test the change.
Created attachment 241258 [details] Patch
Attachment 241258 [details] did not pass style-queue: ERROR: Source/WebKit/mac/WebView/WebHTMLView.mm:2416: An else statement can be removed when the prior "if" concludes with a return, break, continue or goto statement. [readability/control_flow] [4] Total errors found: 1 in 11 files If any of these errors are false positives, please file a bug against check-webkit-style.
Comment on attachment 241258 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=241258&action=review > Source/WebKit2/UIProcess/CoordinatedGraphics/PageViewportController.cpp:325 > + // FIXME: Why this arbitrary precision? We likely want to omit the third argument so that Gyuyoung, do you think this is something you could look into in a follow-up as the EFL port is exercising this code path?
Created attachment 241263 [details] Patch
Attachment 241263 [details] did not pass style-queue: ERROR: Source/WebKit/mac/WebView/WebHTMLView.mm:2416: An else statement can be removed when the prior "if" concludes with a return, break, continue or goto statement. [readability/control_flow] [4] Total errors found: 1 in 13 files If any of these errors are false positives, please file a bug against check-webkit-style.
Comment on attachment 241263 [details] Patch Clearing flags on attachment: 241263 Committed r175796: <http://trac.webkit.org/changeset/175796>
All reviewed patches have been landed. Closing bug.
"Essentually" is misspelled--should be "Essentially".
(In reply to comment #20) > "Essentually" is misspelled--should be "Essentially". Hmm :) Thankfully it is only in the Changelogs.
(In reply to comment #21) > (In reply to comment #20) > > "Essentually" is misspelled--should be "Essentially". > > Hmm :) Thankfully it is only in the Changelogs. Not sure what's the policy in this case. Should I land a patch to fix only the Changelogs?
I like the function name, areEssentiallyEqualAsFloat, that you chose for the float workaround function. Unfortunately, it looks like it was spelled incorrectly as areEssentiallEqualAsFloat in WebPageIOS.mm.
(In reply to comment #23) > I like the function name, areEssentiallyEqualAsFloat, that you chose for the > float workaround function. Unfortunately, it looks like it was spelled > incorrectly as areEssentiallEqualAsFloat in WebPageIOS.mm. Thanks for catching that and sorry about that. I fixed to typo in <http://trac.webkit.org/changeset/175797>.
(In reply to comment #22) > Should I land a patch to fix only the Changelogs? No.