Bug 134424 – [iOS][WK2] Move tap highlight to the inverseScaleRootView

• Component: New Bugs
• Status: RESOLVED FIXED

[Benjamin Poulain]

[iOS][WK2] Move tap highlight to the inverseScaleRootView

[Benjamin Poulain]

Created attachment 234040 [details]
Patch

[WebKit Commit Bot]

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.

[Benjamin Poulain]

Created attachment 234041 [details]
Patch

[Benjamin Poulain]

<rdar://problem/17480880>

[WebKit Commit Bot]

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.

[Tim Horton]

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.

[Benjamin Poulain]

Committed r170600: <http://trac.webkit.org/changeset/170600>

[Darin Adler]

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.

[Benjamin Poulain]

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

[Darin Adler]

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

[Benjamin Poulain]

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

[Darin Adler]

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

[Benjamin Poulain]

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