Bug 209883 – Remove some PLATFORM(IOS_FAMILY) guards in TextFieldInputType

• Status: RESOLVED
• Resolution: FIXED
• Component: WebCore Misc.

[Wenson Hsieh]

Consult a client hook at runtime instead to keep the implementation platform-agnostic(-ish).

[Wenson Hsieh]

Created attachment 395208 [details]
Patch

[Wenson Hsieh]

Created attachment 395210 [details]
Patch

[Darin Adler]

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]

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]

(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]

Yes, seems better.

[Darin Adler]

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]

One thing like this, that we established a bit of a design pattern for, is the "editing behaviors".

[Wenson Hsieh]

(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]

Created attachment 395218 [details]
v2

[Darin Adler]

(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]

(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]

(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]

Created attachment 395220 [details]
Patch for landing

[EWS]

Committed r259375: <https://trac.webkit.org/changeset/259375>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 395220 [details].