Bug 195536 – Fix some misleading function and variable names in WKContentViewInteraction.mm

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKit2

[Wenson Hsieh]

Namely, shouldShowKeyboard and shouldZoomToRevealSelectionRect.

[Wenson Hsieh]

Created attachment 364207 [details]
Patch

[Tim Horton]

Comment on attachment 364207 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=364207&action=review

> Source/WebKit/UIProcess/ios/WKContentViewInteraction.mm:-4726
> -static bool shouldZoomToRevealSelectionRect(WebKit::InputType type)

“Misleading” is an understatement

[Wenson Hsieh]

Comment on attachment 364207 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=364207&action=review

>> Source/WebKit/UIProcess/ios/WKContentViewInteraction.mm:-4726
>> -static bool shouldZoomToRevealSelectionRect(WebKit::InputType type)
> 
> “Misleading” is an understatement

😅

[WebKit Commit Bot]

Comment on attachment 364207 [details]
Patch

Clearing flags on attachment: 364207

Committed r242690: <https://trac.webkit.org/changeset/242690>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Daniel Bates]

Comment on attachment 364207 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=364207&action=review

> Source/WebKit/UIProcess/ios/WKContentViewInteraction.mm:4843
> +    BOOL shouldShowInputView = [&] {

I see where you’re going with this and maybe you’re correct to do it. Certainly this new name is more technically correct, but I can’t stop myself from
feeling it is also the more ambiguous, more meaningless, more abstracted name. Just makes me think of bad software engineering where you abstract a concept so far that you’re talking widgets, aspects, factories and beans and the only technically correct name for the thing is SimpleBeanFactoryAwareAspectInstanceFactory. So, this is a long way of saying that shouldShowKeyboard is more down to earth and comprehendable even though it may not be 100% technically correct. It’s like 99% correct I think (have to read this code again), but yeah 1% wrong cause we may be showing the candidate bar or the “input view” (or whatever they call that thing above the candidate bar).

[Daniel Bates]

What I am typing on now is a keyboard, but more abstractly an input device, even more abstractly an interface device. Yeah, not quite technically correct; a human interface device. Throw in that it’s USB compliant and boom USB HID! That’s how it’s done Wenson :)

[Daniel Bates]

(In reply to Daniel Bates from comment #7)
> What I am typing on now is a keyboard, but more abstractly an input device,
> even more abstractly an interface device. Yeah, not quite technically
> correct; a human interface device. Throw in that it’s USB compliant and boom
> USB HID! That’s how it’s done Wenson :)

What was I talking about again? Oh, right, keyboards. This bug is fun :)

[Wenson Hsieh]

(In reply to Daniel Bates from comment #8)
> (In reply to Daniel Bates from comment #7)
> > What I am typing on now is a keyboard, but more abstractly an input device,
> > even more abstractly an interface device. Yeah, not quite technically
> > correct; a human interface device. Throw in that it’s USB compliant and boom
> > USB HID! That’s how it’s done Wenson :)
> 
> What was I talking about again? Oh, right, keyboards. This bug is fun :)

Perhaps I missed something from the above few comments; did you propose a concrete alternative to input view?

1. The notion of a keyboard is completely absent on watchOS. As such, I chose "input view" because it's a general enough term to describe all scenarios on all platforms that exercise this logic.

2. Furthermore, in this function, we explicitly draw a distinction between elements that show a keyboard (in which case we call -_showKeyboard) vs. elements that do not. These are, according to the switch case:

WebKit::InputType::Select:
WebKit::InputType::DateTimeLocal:
WebKit::InputType::Time:
WebKit::InputType::Month:
WebKit::InputType::Date:
WebKit::InputType::Drawing:
WebKit::InputType::Color

(...arguably, we should make watchOS /not/ go down this codepath and call -_showKeyboard as well, but...I think that will be cleanup for another day).

[Tim Horton]

Agree with Wenson; this function is specifically about the presentation (or lack thereof) of a set of things that includes — but is not limited to — keyboards, and which are known collectively as "input views" in the platform API.

[Daniel Bates]

Okay. Two against one + 1 "platform terminology". You win. Input View it is.