Bug 206459 – Add Legacy WebKit SPI and WebKit IPI to show and hide placeholder

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

[Daniel Bates]

Add SPI to allow a client to show and hide the placeholder in a form control.

[Radar WebKit Bug Importer]

<rdar://problem/58700534>

[Daniel Bates]

Created attachment 388112 [details]
For the bots - missing test results

[Daniel Bates]

Created attachment 388114 [details]
For the bots - missing test results

[Daniel Bates]

Created attachment 388115 [details]
For the bots - missing test results

[Daniel Bates]

Created attachment 388313 [details]
Patch and layout test

[Daniel Bates]

Created attachment 388321 [details]
Patch and layout test

Expose SPI in legacy WebKit as well for some legacy clients.

[Wenson Hsieh]

Comment on attachment 388321 [details]
Patch and layout test

View in context: https://bugs.webkit.org/attachment.cgi?id=388321&action=review

> Source/WebKit/ChangeLog:10
> +        Add Modern WebKit SPI to allow a client to control whether the placeholder can be shown or
> +        not when a form control is empty. This is for aesthetics.

It doesn't look like this patch actually adds "Modern WebKit SPI to allow a client to control whether the placeholder can be shown…". Was this intended for a followup?

> Source/WebCore/html/HTMLTextFormControlElement.cpp:178
> +    m_canShowPlaceholder = canShowPlaceholder;

Nit - is it necessary to updatePlaceholderVisibility(); in the case where m_canShowPlaceholder didn't change?

[Daniel Bates]

Modern WebKit IPI! Not SPI and I don't need SPI either. I'll fix up the English.

[Daniel Bates]

Comment on attachment 388321 [details]
Patch and layout test

View in context: https://bugs.webkit.org/attachment.cgi?id=388321&action=review

Thanks for the review!

>> Source/WebKit/ChangeLog:10
>> +        not when a form control is empty. This is for aesthetics.
> 
> It doesn't look like this patch actually adds "Modern WebKit SPI to allow a client to control whether the placeholder can be shown…". Was this intended for a followup?

IPI

>> Source/WebCore/html/HTMLTextFormControlElement.cpp:178
>> +    m_canShowPlaceholder = canShowPlaceholder;
> 
> Nit - is it necessary to updatePlaceholderVisibility(); in the case where m_canShowPlaceholder didn't change?

That function has the smarts... didn't feel a branch to avoid the call would speed things up much. So I didn't do it

[Wenson Hsieh]

Comment on attachment 388321 [details]
Patch and layout test

View in context: https://bugs.webkit.org/attachment.cgi?id=388321&action=review

>>> Source/WebCore/html/HTMLTextFormControlElement.cpp:178
>>> +    m_canShowPlaceholder = canShowPlaceholder;
>> 
>> Nit - is it necessary to updatePlaceholderVisibility(); in the case where m_canShowPlaceholder didn't change?
> 
> That function has the smarts... didn't feel a branch to avoid the call would speed things up much. So I didn't do it

Sounds good — thanks for clarifying!

[Daniel Bates]

Created attachment 388356 [details]
To land

[Daniel Bates]

Comment on attachment 388356 [details]
To land

Clearing flags on attachment: 388356

Committed r254886: <https://trac.webkit.org/changeset/254886>

[Daniel Bates]

All reviewed patches have been landed.  Closing bug.