Bug 195536 - Fix some misleading function and variable names in WKContentViewInteraction.mm
Summary: Fix some misleading function and variable names in WKContentViewInteraction.mm
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit2 (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Wenson Hsieh
URL:
Keywords: DoNotImportToRadar
Depends on:
Blocks:
 
Reported: 2019-03-10 15:39 PDT by Wenson Hsieh
Modified: 2019-03-11 06:48 PDT (History)
5 users (show)

See Also:


Attachments
Patch (4.46 KB, patch)
2019-03-10 16:08 PDT, Wenson Hsieh
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Wenson Hsieh 2019-03-10 15:39:21 PDT
Namely, shouldShowKeyboard and shouldZoomToRevealSelectionRect.
Comment 1 Wenson Hsieh 2019-03-10 16:08:12 PDT
Created attachment 364207 [details]
Patch
Comment 2 Tim Horton 2019-03-10 16:15:52 PDT
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
Comment 3 Wenson Hsieh 2019-03-10 16:23:39 PDT
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

😅
Comment 4 WebKit Commit Bot 2019-03-10 16:49:16 PDT
Comment on attachment 364207 [details]
Patch

Clearing flags on attachment: 364207

Committed r242690: <https://trac.webkit.org/changeset/242690>
Comment 5 WebKit Commit Bot 2019-03-10 16:49:17 PDT
All reviewed patches have been landed.  Closing bug.
Comment 6 Daniel Bates 2019-03-10 21:28:15 PDT
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).
Comment 7 Daniel Bates 2019-03-10 21:37:24 PDT
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 :)
Comment 8 Daniel Bates 2019-03-10 21:39:37 PDT
(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 :)
Comment 9 Wenson Hsieh 2019-03-10 23:02:09 PDT
(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).
Comment 10 Tim Horton 2019-03-11 00:32:10 PDT
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.
Comment 11 Daniel Bates 2019-03-11 06:48:25 PDT
Okay. Two against one + 1 "platform terminology". You win. Input View it is.