Bug 209883

Summary: Remove some PLATFORM(IOS_FAMILY) guards in TextFieldInputType
Product: WebKit Reporter: Wenson Hsieh <wenson_hsieh>
Component: WebCore Misc.Assignee: Wenson Hsieh <wenson_hsieh>
Status: RESOLVED FIXED    
Severity: Normal CC: bdakin, cdumez, darin, esprehn+autocc, ews-watchlist, gyuyoung.kim, japhet, megan_gardner, mifenton, thorton, webkit-bug-importer
Priority: P2 Keywords: DoNotImportToRadar
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Patch
none
Patch
none
v2
none
Patch for landing none

Wenson Hsieh
Reported 2020-04-01 15:03:21 PDT
Consult a client hook at runtime instead to keep the implementation platform-agnostic(-ish).
Attachments
Patch (10.79 KB, patch)
2020-04-01 15:09 PDT, Wenson Hsieh 		no flags
DetailsFormatted DiffDiff
Patch (10.79 KB, patch)
2020-04-01 15:18 PDT, Wenson Hsieh 		no flags
DetailsFormatted DiffDiff
v2 (5.54 KB, patch)
2020-04-01 16:05 PDT, Wenson Hsieh 		no flags
DetailsFormatted DiffDiff
Patch for landing (5.52 KB, patch)
2020-04-01 16:20 PDT, Wenson Hsieh 		no flags
DetailsFormatted DiffDiff
Show Obsolete (3) View All Add attachment proposed patch, testcase, etc.
Wenson Hsieh
Comment 1 2020-04-01 15:09:35 PDT
Created attachment 395208 [details] Patch
Wenson Hsieh
Comment 2 2020-04-01 15:18:58 PDT
Created attachment 395210 [details] Patch
Darin Adler
Comment 3 2020-04-01 15:25:04 PDT
Comment on attachment 395210 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=395210&action=review > Source/WebKit/WebProcess/WebCoreSupport/WebChromeClient.cpp:814 > +#if PLATFORM(IOS_FAMILY) > + return true; > +#else > + return false; > +#endif If this is just a platform conditional, do we really need to use the client design pattern? Or perhaps there is some way this is likely to be dynamic in the future that we are preparing for? The client system is mildly elaborate and is worthwhile mainly when we want a true separation of concerns between WebCore and WebKit layers, not just for platform differences.
Darin Adler
Comment 4 2020-04-01 15:25:34 PDT
The idea of having this be a runtime check seems fine. But calling across framework boundaries to a client seems like it might be overkill.
Wenson Hsieh
Comment 5 2020-04-01 15:27:45 PDT
(In reply to Darin Adler from comment #4) > The idea of having this be a runtime check seems fine. But calling across > framework boundaries to a client seems like it might be overkill. Fair point! Perhaps this would be cleaner? bool TextFieldInputType::shouldOnlyShowDataListDropdownButtonWhenFocusedOrEdited() const { #if PLATFORM(IOS_FAMILY) return true; #else return false; #endif } (...instead of all the client plumbing).
Darin Adler
Comment 6 2020-04-01 15:29:59 PDT
Yes, seems better.
Darin Adler
Comment 7 2020-04-01 15:31:01 PDT
Another possible consideration is that sometimes we want a way to change something like this for test purposes so we can test both code paths on both platforms.
Darin Adler
Comment 8 2020-04-01 15:31:42 PDT
One thing like this, that we established a bit of a design pattern for, is the "editing behaviors".
Wenson Hsieh
Comment 9 2020-04-01 15:48:15 PDT
(In reply to Darin Adler from comment #8) > One thing like this, that we established a bit of a design pattern for, is > the "editing behaviors". I see! This *might* be sufficiently relevant to editing that it could be refactored into an editing behavior, but I’m not sure we have a need for it quite yet (even for testing). Would you be okay with leaving it as a helper function on TextFieldInputType that returns a bool?
Wenson Hsieh
Comment 10 2020-04-01 16:05:14 PDT
Created attachment 395218 [details] v2
Darin Adler
Comment 11 2020-04-01 16:09:56 PDT
(In reply to Wenson Hsieh from comment #9) > Would you be okay with leaving it as a helper function on TextFieldInputType > that returns a bool? Yes. Can always tweak it again later.
Darin Adler
Comment 12 2020-04-01 16:10:26 PDT
(In reply to Darin Adler from comment #11) > (In reply to Wenson Hsieh from comment #9) > > Would you be okay with leaving it as a helper function on TextFieldInputType > > that returns a bool? > > Yes. Can always tweak it again later. For example, could just change it to a static member function.
Wenson Hsieh
Comment 13 2020-04-01 16:13:36 PDT
(In reply to Darin Adler from comment #12) > (In reply to Darin Adler from comment #11) > > (In reply to Wenson Hsieh from comment #9) > > > Would you be okay with leaving it as a helper function on TextFieldInputType > > > that returns a bool? > > > > Yes. Can always tweak it again later. > > For example, could just change it to a static member function. Ah, sure — since this isn’t something that varies between instances of TextFieldInputType, a static helper makes more sense. I’ll tweak the patch to make it static and go with that. Thanks for the feedback!
Wenson Hsieh
Comment 14 2020-04-01 16:20:01 PDT
Created attachment 395220 [details] Patch for landing
EWS
Comment 15 2020-04-01 17:35:08 PDT
Committed r259375: <https://trac.webkit.org/changeset/259375> All reviewed patches have been landed. Closing bug and clearing flags on attachment 395220 [details].
Note You need to log in before you can comment on or make changes to this bug.