| Summary: | [iOS][WK2] Move tap highlight to the inverseScaleRootView | ||||||||
|---|---|---|---|---|---|---|---|---|---|
| Product: | WebKit | Reporter: | Benjamin Poulain <benjamin> | ||||||
| Component: | New Bugs | Assignee: | Benjamin Poulain <benjamin> | ||||||
| Status: | RESOLVED FIXED | ||||||||
| Severity: | Normal | CC: | bunhere, cdumez, commit-queue, darin, enrica, gyuyoung.kim, sergio | ||||||
| Priority: | P2 | ||||||||
| Version: | 528+ (Nightly build) | ||||||||
| Hardware: | Unspecified | ||||||||
| OS: | Unspecified | ||||||||
| Attachments: |
|
||||||||
|
Description
Benjamin Poulain
2014-06-27 20:39:23 PDT
Created attachment 234040 [details]
Patch
Attachment 234040 [details] did not pass style-queue:
ERROR: Source/WebKit2/UIProcess/ios/WKContentViewInteraction.mm:806: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3]
ERROR: Source/WebKit2/UIProcess/ios/WKContentViewInteraction.mm:808: Place brace on its own line for function definitions. [whitespace/braces] [4]
ERROR: Source/WebKit2/UIProcess/ios/WKContentViewInteraction.mm:808: Missing space before { [whitespace/braces] [5]
ERROR: Source/WebKit2/UIProcess/ios/WKContentViewInteraction.mm:808: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3]
ERROR: Source/WebKit2/UIProcess/ios/WKContentViewInteraction.mm:809: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3]
ERROR: Source/WebKit2/UIProcess/ios/WKContentViewInteraction.mm:810: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3]
ERROR: Source/WebKit2/UIProcess/ios/WKContentViewInteraction.mm:811: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3]
ERROR: Source/WebKit2/UIProcess/ios/WKContentViewInteraction.mm:812: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3]
Total errors found: 8 in 4 files
If any of these errors are false positives, please file a bug against check-webkit-style.
Created attachment 234041 [details]
Patch
Attachment 234041 [details] did not pass style-queue:
ERROR: Source/WebKit2/UIProcess/ios/WKContentViewInteraction.mm:806: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3]
ERROR: Source/WebKit2/UIProcess/ios/WKContentViewInteraction.mm:808: Place brace on its own line for function definitions. [whitespace/braces] [4]
ERROR: Source/WebKit2/UIProcess/ios/WKContentViewInteraction.mm:808: Missing space before { [whitespace/braces] [5]
ERROR: Source/WebKit2/UIProcess/ios/WKContentViewInteraction.mm:808: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3]
ERROR: Source/WebKit2/UIProcess/ios/WKContentViewInteraction.mm:809: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3]
ERROR: Source/WebKit2/UIProcess/ios/WKContentViewInteraction.mm:810: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3]
ERROR: Source/WebKit2/UIProcess/ios/WKContentViewInteraction.mm:811: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3]
ERROR: Source/WebKit2/UIProcess/ios/WKContentViewInteraction.mm:812: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3]
Total errors found: 8 in 4 files
If any of these errors are false positives, please file a bug against check-webkit-style.
Comment on attachment 234041 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=234041&action=review > Source/WebKit2/UIProcess/ios/WKContentViewInteraction.mm:74 > +const CGFloat UIWebViewMinimumHighlightRadius = 2.0; This constant name seems nonideal. > Source/WebKit2/UIProcess/ios/WKContentViewInteraction.mm:521 > + CGFloat selfScale = [[self layer] transform].m11; moar dot notation > Source/WebKit2/UIProcess/ios/WKContentViewInteraction.mm:559 > + [borderRadii addObject:[NSValue valueWithCGSize:CGSizeMake(_tapHighlightInformation.topLeftRadius.width() + UIWebViewMinimumHighlightRadius, _tapHighlightInformation.topLeftRadius.height() + UIWebViewMinimumHighlightRadius)]]; I bet you could come up with a much less wordy way to write these. > Source/WebKit2/UIProcess/ios/WKContentViewInteraction.mm:806 > + [[_highlightView layer] setOpacity:0]; dots etc. Committed r170600: <http://trac.webkit.org/changeset/170600> Comment on attachment 234041 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=234041&action=review > Source/WebKit2/UIProcess/ios/WKContentViewInteraction.h:190 > + return fabs(a - b) < std::numeric_limits<float>::epsilon(); This converts a - b to a double, and back to a float before comparing with epsilon, which is way more expensive than we’d like. It should use fabsf to avoid this. By the way, I believe this is only an acceptable technique for numbers in a small range of float. For large floats, this will be the same as "=="; a difference of epsilon can’t actually be represented, and for very small numbers (denormalized) this will return true for values that differ quite a bit. This might be OK for our current purposes but it’s questionable for broader use. (In reply to comment #8) > (From update of attachment 234041 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=234041&action=review > > > Source/WebKit2/UIProcess/ios/WKContentViewInteraction.h:190 > > + return fabs(a - b) < std::numeric_limits<float>::epsilon(); > > This converts a - b to a double, and back to a float before comparing with epsilon, which is way more expensive than we’d like. It should use fabsf to avoid this. > > By the way, I believe this is only an acceptable technique for numbers in a small range of float. For large floats, this will be the same as "=="; a difference of epsilon can’t actually be represented, and for very small numbers (denormalized) this will return true for values that differ quite a bit. This might be OK for our current purposes but it’s questionable for broader use. That was not my intent at all here. :( My intent was to have the comparison done with double. I was expecting the compiler to load std::numeric_limits<float>::epsilon() to double from a constant. Overall, everything should have been fairly cheap on ARM. What I am missing here is why is the left side converted to float instead of the right side "converted" to double? I thought the implicit conversion would always upconvert in this case. (In reply to comment #9) > My intent was to have the comparison done with double. So you expected subtraction to be done with float precision, then wanted the result of that subtraction to be converted to a double, then do an absolute value on the double value, then compare that with a double constant that contains the float precision epsilon? I accept that you expected that, but do not understand why it’s desirable. Why is any conversion to double helpful? If conversion to double is helpful, say for performance or precision resasons, why is the right time to convert to double *after* subtraction but *before* computing absolute value, rather than, say, before the subtraction or after the absolute value computation? > What I am missing here is why is the left side converted to float instead of the right side "converted" to double? Sorry, my mistake, naturally the left side won’t be converted to float. If we do indeed intend to intentionally convert to double, calling fabs with a float argument is an unnecessarily oblique way to do so, since it’s a common programming mistake typically made by those who do *not* intend to convert to double. (In reply to comment #10) > (In reply to comment #9) > > My intent was to have the comparison done with double. > > So you expected subtraction to be done with float precision, then wanted the result of that subtraction to be converted to a double, then do an absolute value on the double value, then compare that with a double constant that contains the float precision epsilon? > > I accept that you expected that, but do not understand why it’s desirable. Why is any conversion to double helpful? If conversion to double is helpful, say for performance or precision resasons, why is the right time to convert to double *after* subtraction but *before* computing absolute value, rather than, say, before the subtraction or after the absolute value computation? Ok, I see the mistake now, this is wrong. The arguments were supposed to be template type, abstracting the CGFloat being float or double. The reason that code was there is that the UIProcess handle its transform on double, while the WebProcess represents them as float. We must ignore small differences in scale between what is on screen and the WebProcess's page scale factor otherwise we create update cycles. I bet this code only works today because of the conversion double->float of the arguments instead of using the abs() < epsilon. (In reply to comment #11) > The arguments were supposed to be template type, abstracting the CGFloat being float or double. In that case, I suggest using the newfangled std::abs, now overloaded for many types. Rather than fabs, which only works on double. (In reply to comment #12) > (In reply to comment #11) > > The arguments were supposed to be template type, abstracting the CGFloat being float or double. > > In that case, I suggest using the newfangled std::abs, now overloaded for many types. Rather than fabs, which only works on double. Sounds good! I'll update this code. |