195536 2019-03-10 15:39:21 -0700 Fix some misleading function and variable names in WKContentViewInteraction.mm 2019-03-11 06:48:25 -0700 1 1 1 Unclassified WebKit WebKit2 WebKit Nightly Build Unspecified Unspecified RESOLVED FIXED DoNotImportToRadar P2 Normal --- 1 wenson_hsieh wenson_hsieh bdakin commit-queue dbates thorton webkit-bug-importer oldest_to_newest 1514595 0 wenson_hsieh 2019-03-10 15:39:21 -0700 Namely, shouldShowKeyboard and shouldZoomToRevealSelectionRect. 1514608 1 364207 wenson_hsieh 2019-03-10 16:08:12 -0700 Created attachment 364207 Patch 1514615 2 364207 thorton 2019-03-10 16:15:52 -0700 Comment on attachment 364207 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 1514616 3 364207 wenson_hsieh 2019-03-10 16:23:39 -0700 Comment on attachment 364207 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 😅 1514625 4 364207 commit-queue 2019-03-10 16:49:16 -0700 Comment on attachment 364207 Patch Clearing flags on attachment: 364207 Committed r242690: <https://trac.webkit.org/changeset/242690> 1514626 5 commit-queue 2019-03-10 16:49:17 -0700 All reviewed patches have been landed. Closing bug. 1514692 6 364207 dbates 2019-03-10 21:28:15 -0700 Comment on attachment 364207 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). 1514693 7 dbates 2019-03-10 21:37:24 -0700 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 :) 1514694 8 dbates 2019-03-10 21:39:37 -0700 (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 :) 1514705 9 wenson_hsieh 2019-03-10 23:02:09 -0700 (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). 1514715 10 thorton 2019-03-11 00:32:10 -0700 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. 1514755 11 dbates 2019-03-11 06:48:25 -0700 Okay. Two against one + 1 "platform terminology". You win. Input View it is. 364207 2019-03-10 16:08:12 -0700 2019-03-10 16:49:16 -0700 Patch bug-195536-20190310160811.patch text/plain 4567 wenson_hsieh U3VidmVyc2lvbiBSZXZpc2lvbjogMjQyNjg1CmRpZmYgLS1naXQgYS9Tb3VyY2UvV2ViS2l0L0No YW5nZUxvZyBiL1NvdXJjZS9XZWJLaXQvQ2hhbmdlTG9nCmluZGV4IDMzNTI1ZWMyMTlkZDExY2Jh Y2U0OTY2MWFhYmVlN2NlOTY1ZDgyODMuLjU4YjFiYWMyOGRkMmEyNDZmNTgyMjBkYTQxNWM5Y2Ni YmY4ZmExMDggMTAwNjQ0Ci0tLSBhL1NvdXJjZS9XZWJLaXQvQ2hhbmdlTG9nCisrKyBiL1NvdXJj ZS9XZWJLaXQvQ2hhbmdlTG9nCkBAIC0xLDMgKzEsMjYgQEAKKzIwMTktMDMtMTAgIFdlbnNvbiBI c2llaCAgPHdlbnNvbl9oc2llaEBhcHBsZS5jb20+CisKKyAgICAgICAgRml4IHNvbWUgbWlzbGVh ZGluZyBmdW5jdGlvbiBhbmQgdmFyaWFibGUgbmFtZXMgaW4gV0tDb250ZW50Vmlld0ludGVyYWN0 aW9uLm1tCisgICAgICAgIGh0dHBzOi8vYnVncy53ZWJraXQub3JnL3Nob3dfYnVnLmNnaT9pZD0x OTU1MzYKKworICAgICAgICBSZXZpZXdlZCBieSBOT0JPRFkgKE9PUFMhKS4KKworICAgICAgICAq IFVJUHJvY2Vzcy9pb3MvV0tDb250ZW50Vmlld0ludGVyYWN0aW9uLm1tOgorICAgICAgICAoc2hv dWxkRGVmZXJab29taW5nVG9TZWxlY3Rpb25XaGVuUmV2ZWFsaW5nRm9jdXNlZEVsZW1lbnQpOgor CisgICAgICAgIFJlbmFtZSBzaG91bGRab29tVG9SZXZlYWxTZWxlY3Rpb25SZWN0IHRvIHNob3Vs ZERlZmVyWm9vbWluZ1RvU2VsZWN0aW9uV2hlblJldmVhbGluZ0ZvY3VzZWRFbGVtZW50OyB0aGlz CisgICAgICAgIGZ1bmN0aW9uIGlzIHVzZWQgdG8gZGV0ZXJtaW5lIHdoZXRoZXIgd2Ugc2hvdWxk IHpvb20gdG8gdGhlIHNlbGVjdGlvbiByZWN0IHdoZW4gcmV2ZWFsaW5nIHRoZSBmb2N1c2VkIGVs ZW1lbnQsCisgICAgICAgIGFuZCB0aGVyZWZvcmUgYWZmZWN0cyB3aGV0aGVyIHdlIG5lZWQgdG8g ZGVmZXIgem9vbWluZyB1bnRpbCB3ZSBnZXQgc2VsZWN0aW9uIGluZm9ybWF0aW9uLgorCisgICAg ICAgIChyZWN0VG9SZXZlYWxXaGVuWm9vbWluZ1RvRm9jdXNlZEVsZW1lbnQpOgorICAgICAgICAo LVtXS0NvbnRlbnRWaWV3IF9lbGVtZW50RGlkRm9jdXM6dXNlcklzSW50ZXJhY3Rpbmc6Ymx1clBy ZXZpb3VzTm9kZTpjaGFuZ2luZ0FjdGl2aXR5U3RhdGU6dXNlck9iamVjdDpdKToKKworICAgICAg ICBSZW5hbWUgc2hvdWxkU2hvd0tleWJvYXJkIHRvIHNob3VsZFNob3dJbnB1dFZpZXc7IHRoaXMg Ym9vbGVhbiBpbmRpY2F0ZXMgd2hldGhlciB3ZSBzaG91bGQgc2hvdyBub3Qgb25seSB0aGUKKyAg ICAgICAga2V5Ym9hcmQsIGJ1dCBvdGhlciBVSSBmb3IgdXNlciBpbnB1dCBzdWNoIGFzIGRhdGUg YW5kIHNlbGVjdCBwaWNrZXJzLgorCisgICAgICAgICgtW1dLQ29udGVudFZpZXcgX2RpZFJlY2Vp dmVFZGl0b3JTdGF0ZVVwZGF0ZUFmdGVyRm9jdXNdKToKKyAgICAgICAgKHNob3VsZFpvb21Ub1Jl dmVhbFNlbGVjdGlvblJlY3QpOiBEZWxldGVkLgorCiAyMDE5LTAzLTA5ICBXZW5zb24gSHNpZWgg IDx3ZW5zb25faHNpZWhAYXBwbGUuY29tPgogCiAgICAgICAgIFJFR1JFU1NJT04gKHIyNDI1NTEp OiBTcG9yYWRpYyBoYW5ncyB3aGVuIHRhcHBpbmcgdG8gY2hhbmdlIHNlbGVjdGlvbiBvbiBpT1MK ZGlmZiAtLWdpdCBhL1NvdXJjZS9XZWJLaXQvVUlQcm9jZXNzL2lvcy9XS0NvbnRlbnRWaWV3SW50 ZXJhY3Rpb24ubW0gYi9Tb3VyY2UvV2ViS2l0L1VJUHJvY2Vzcy9pb3MvV0tDb250ZW50Vmlld0lu dGVyYWN0aW9uLm1tCmluZGV4IDRlZjZmNTAzM2RlNGRmMmM1ZGNlYjUxYzQ5MTk4MzcyMWNiNzIy MTAuLjg2N2NjOGFjZjkxNDFkZWFiMTExMjA1MDhhMWRlM2ExYjY1ZTY1YTIgMTAwNjQ0Ci0tLSBh L1NvdXJjZS9XZWJLaXQvVUlQcm9jZXNzL2lvcy9XS0NvbnRlbnRWaWV3SW50ZXJhY3Rpb24ubW0K KysrIGIvU291cmNlL1dlYktpdC9VSVByb2Nlc3MvaW9zL1dLQ29udGVudFZpZXdJbnRlcmFjdGlv bi5tbQpAQCAtNDcyMyw3ICs0NzIzLDcgQEAgLSAoVUlXZWJGb3JtQWNjZXNzb3J5ICopZm9ybUFj Y2Vzc29yeVZpZXcKICAgICByZXR1cm4gX2Zvcm1BY2Nlc3NvcnlWaWV3LmdldCgpOwogfQogCi1z dGF0aWMgYm9vbCBzaG91bGRab29tVG9SZXZlYWxTZWxlY3Rpb25SZWN0KFdlYktpdDo6SW5wdXRU eXBlIHR5cGUpCitzdGF0aWMgYm9vbCBzaG91bGREZWZlclpvb21pbmdUb1NlbGVjdGlvbldoZW5S ZXZlYWxpbmdGb2N1c2VkRWxlbWVudChXZWJLaXQ6OklucHV0VHlwZSB0eXBlKQogewogICAgIHN3 aXRjaCAodHlwZSkgewogICAgIGNhc2UgV2ViS2l0OjpJbnB1dFR5cGU6OkNvbnRlbnRFZGl0YWJs ZToKQEAgLTQ3NDgsNyArNDc0OCw3IEBAIHN0YXRpYyBXZWJDb3JlOjpGbG9hdFJlY3QgcmVjdFRv UmV2ZWFsV2hlblpvb21pbmdUb0ZvY3VzZWRFbGVtZW50KGNvbnN0IFdlYktpdDo6CiAgICAgaWYg KGVsZW1lbnRJbmZvLmVsZW1lbnRSZWN0LmNvbnRhaW5zKGVsZW1lbnRJbmZvLmxhc3RJbnRlcmFj dGlvbkxvY2F0aW9uKSkKICAgICAgICAgZWxlbWVudEludGVyYWN0aW9uUmVjdCA9IHsgZWxlbWVu dEluZm8ubGFzdEludGVyYWN0aW9uTG9jYXRpb24sIHsgMSwgMSB9IH07CiAKLSAgICBpZiAoIXNo b3VsZFpvb21Ub1JldmVhbFNlbGVjdGlvblJlY3QoZWxlbWVudEluZm8uZWxlbWVudFR5cGUpKQor ICAgIGlmICghc2hvdWxkRGVmZXJab29taW5nVG9TZWxlY3Rpb25XaGVuUmV2ZWFsaW5nRm9jdXNl ZEVsZW1lbnQoZWxlbWVudEluZm8uZWxlbWVudFR5cGUpKQogICAgICAgICByZXR1cm4gZWxlbWVu dEludGVyYWN0aW9uUmVjdDsKIAogICAgIGlmIChlZGl0b3JTdGF0ZS5pc01pc3NpbmdQb3N0TGF5 b3V0RGF0YSkgewpAQCAtNDg0MCw3ICs0ODQwLDcgQEAgLSAodm9pZClfZWxlbWVudERpZEZvY3Vz Oihjb25zdCBXZWJLaXQ6OkZvY3VzZWRFbGVtZW50SW5mb3JtYXRpb24mKWluZm9ybWF0aW9uIHUK ICAgICBlbHNlCiAgICAgICAgIFtzZWxmIF9zdG9wU3VwcHJlc3NpbmdTZWxlY3Rpb25Bc3Npc3Rh bnRGb3JSZWFzb246V2ViS2l0OjpGb2N1c2VkRWxlbWVudElzVG9vU21hbGxdOwogCi0gICAgQk9P TCBzaG91bGRTaG93S2V5Ym9hcmQgPSBbJl0geworICAgIEJPT0wgc2hvdWxkU2hvd0lucHV0Vmll dyA9IFsmXSB7CiAgICAgICAgIHN3aXRjaCAoc3RhcnRJbnB1dFNlc3Npb25Qb2xpY3kpIHsKICAg ICAgICAgY2FzZSBfV0tGb2N1c1N0YXJ0c0lucHV0U2Vzc2lvblBvbGljeUF1dG86CiAgICAgICAg ICAgICAvLyBUaGUgZGVmYXVsdCBiZWhhdmlvciBpcyB0byBhbGxvdyBub2RlIGFzc2lzdGFuY2Ug aWYgdGhlIHVzZXIgaXMgaW50ZXJhY3RpbmcuCkBAIC00ODg3LDcgKzQ4ODcsNyBAQCAtICh2b2lk KV9lbGVtZW50RGlkRm9jdXM6KGNvbnN0IFdlYktpdDo6Rm9jdXNlZEVsZW1lbnRJbmZvcm1hdGlv biYpaW5mb3JtYXRpb24gdQogICAgICAgICBbX2RyYXdpbmdDb29yZGluYXRvciBpbnN0YWxsSW5r UGlja2VyRm9yRHJhd2luZzppbmZvcm1hdGlvbi5lbWJlZGRlZFZpZXdJRF07CiAjZW5kaWYKIAot ICAgIGlmICghc2hvdWxkU2hvd0tleWJvYXJkKQorICAgIGlmICghc2hvdWxkU2hvd0lucHV0Vmll dykKICAgICAgICAgcmV0dXJuOwogCiAgICAgaWYgKCFpc0Fzc2lzdGFibGVJbnB1dFR5cGUoaW5m b3JtYXRpb24uZWxlbWVudFR5cGUpKQpAQCAtNDk1MSw3ICs0OTUxLDcgQEAgLSAodm9pZClfZWxl bWVudERpZEZvY3VzOihjb25zdCBXZWJLaXQ6OkZvY3VzZWRFbGVtZW50SW5mb3JtYXRpb24mKWlu Zm9ybWF0aW9uIHUKICAgICBpZiAoZWRpdGFibGVDaGFuZ2VkKQogICAgICAgICBbX3dlYlZpZXcg X3NjaGVkdWxlVmlzaWJsZUNvbnRlbnRSZWN0VXBkYXRlXTsKICAgICAKLSAgICBpZiAoIXNob3Vs ZFpvb21Ub1JldmVhbFNlbGVjdGlvblJlY3QoX2ZvY3VzZWRFbGVtZW50SW5mb3JtYXRpb24uZWxl bWVudFR5cGUpKQorICAgIGlmICghc2hvdWxkRGVmZXJab29taW5nVG9TZWxlY3Rpb25XaGVuUmV2 ZWFsaW5nRm9jdXNlZEVsZW1lbnQoX2ZvY3VzZWRFbGVtZW50SW5mb3JtYXRpb24uZWxlbWVudFR5 cGUpKQogICAgICAgICBbc2VsZiBfem9vbVRvUmV2ZWFsRm9jdXNlZEVsZW1lbnRdOwogCiAgICAg W3NlbGYgX2Vuc3VyZUZvcm1BY2Nlc3NvcnlWaWV3XTsKQEAgLTUxMDYsNyArNTEwNiw3IEBAIC0g KHZvaWQpX2RpZFJlY2VpdmVFZGl0b3JTdGF0ZVVwZGF0ZUFmdGVyRm9jdXMKIAogICAgIC8vIEZJ WE1FOiBJZiB0aGUgaW5pdGlhbCB3cml0aW5nIGRpcmVjdGlvbiBqdXN0IGNoYW5nZWQsIHdlIHNo b3VsZCB3YWl0IHVudGlsIHdlIGdldCB0aGUgbmV4dCBwb3N0LWxheW91dCBlZGl0b3Igc3RhdGUK ICAgICAvLyBiZWZvcmUgem9vbWluZyB0byByZXZlYWwgdGhlIHNlbGVjdGlvbiByZWN0LgotICAg IGlmIChzaG91bGRab29tVG9SZXZlYWxTZWxlY3Rpb25SZWN0KF9mb2N1c2VkRWxlbWVudEluZm9y bWF0aW9uLmVsZW1lbnRUeXBlKSkKKyAgICBpZiAoc2hvdWxkRGVmZXJab29taW5nVG9TZWxlY3Rp b25XaGVuUmV2ZWFsaW5nRm9jdXNlZEVsZW1lbnQoX2ZvY3VzZWRFbGVtZW50SW5mb3JtYXRpb24u ZWxlbWVudFR5cGUpKQogICAgICAgICBbc2VsZiBfem9vbVRvUmV2ZWFsRm9jdXNlZEVsZW1lbnRd OwogfQogCg==