Consult a client hook at runtime instead to keep the implementation platform-agnostic(-ish).
Created attachment 395208 [details] Patch
Created attachment 395210 [details] Patch
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.
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.
(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).
Yes, seems better.
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.
One thing like this, that we established a bit of a design pattern for, is the "editing behaviors".
(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?
Created attachment 395218 [details] v2
(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.
(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.
(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!
Created attachment 395220 [details] Patch for landing
Committed r259375: <https://trac.webkit.org/changeset/259375> All reviewed patches have been landed. Closing bug and clearing flags on attachment 395220 [details].