Bug 102405 – Replace some trivial WebKitSystemInterface calls with direct SPI calls

• Component: New Bugs
• Status: RESOLVED FIXED

[Robert Sesek]

Replace some trivial WebKitSystemInterface calls with direct SPI calls

[Robert Sesek]

Created attachment 174471 [details]
Patch

Comment on attachment 174471 [details]
Patch

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

> Source/WebCore/platform/mac/SystemPrivateInterface.h:39
> +// Forward-declaring symbols is preferred to using the WebKitSystemIterface.

Why do you say this?

[WebKit Review Bot]

Attachment 174471 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCor..." exit_code: 1
Source/WebCore/platform/mac/SystemPrivateInterface.h:42:  _NSDrawCarbonThemeBezel is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
Source/WebCore/platform/mac/SystemPrivateInterface.h:43:  _NSDrawCarbonThemeListBox is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
Total errors found: 2 in 9 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Robert Sesek]

This is really an RFC to see what people think of this approach of reducing reliance on the WebKitSystemInterface (at least where code is shared between Chromium and Mac). In this patch, I centralized all the forward declares for the SPI into a single file, but I could also just forward-declare at each callsite.

Another option would be to keep the wkFunction() pattern, but provide an implementation in code, rather than just linking in the opaque static library.

[Robert Sesek]

(In reply to comment #2)
> (From update of attachment 174471 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=174471&action=review
> 
> > Source/WebCore/platform/mac/SystemPrivateInterface.h:39
> > +// Forward-declaring symbols is preferred to using the WebKitSystemIterface.
> 
> Why do you say this?

There's an on-going discussion. This bug is to discuss if that's actually true, but I wrote the patch assuming it is.

(In reply to comment #3)
> Attachment 174471 [details] did not pass style-queue:
> 
> Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCor..." exit_code: 1
> Source/WebCore/platform/mac/SystemPrivateInterface.h:42:  _NSDrawCarbonThemeBezel is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
> Source/WebCore/platform/mac/SystemPrivateInterface.h:43:  _NSDrawCarbonThemeListBox is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
> Total errors found: 2 in 9 files
> 
> 
> If any of these errors are false positives, please file a bug against check-webkit-style.

I will pending the result of this discussion (if we want to move forward with doing this).

[Alexey Proskuryakov]

I don't think that we have reached consensus yet. Redeclaring SPIs has an extremely unfortunate consequence - if you get it wrong, or if the SPI changes later, compiler won't tell you about this, and the outcome will be a difficult to diagnose bug.

It's something we've been doing for a long time with some SPIs (it mostly depended on personal preference whether to put something in WKSI or re-declare it), and the problem did happen before.

Additionally, please note that these are not APIs, so they may not exist in past or future OS releases. This needs to be handled gracefully at runtime by builds that are intended to have a single binary for multiple OS versions.

[Robert Sesek]

(In reply to comment #6)
> I don't think that we have reached consensus yet. Redeclaring SPIs has an extremely unfortunate consequence - if you get it wrong, or if the SPI changes later, compiler won't tell you about this, and the outcome will be a difficult to diagnose bug.

OK. I'll relax the comment in the .h and maybe converting to direct SPI is something we only do in code shared by Chromium and Mac.

> It's something we've been doing for a long time with some SPIs (it mostly depended on personal preference whether to put something in WKSI or re-declare it), and the problem did happen before.
> 
> Additionally, please note that these are not APIs, so they may not exist in past or future OS releases. This needs to be handled gracefully at runtime by builds that are intended to have a single binary for multiple OS versions.

Yes, and Chromium is one such example of having a single binary that runs across multiple versions. Maybe we should then keep the wkFunction() pattern, perhaps in a SPI:: namespace, to keep the runtime checks away from the direct callsite (or do we want the checks at the callsite?).

[Maciej Stachowiak]

Comment on attachment 174471 [details]
Patch

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

I like the direction of moving from WKSI use to SPI. Though, it might be worth a discussion on webkit-dev because I'm not sure everyone with an interest in this topic was on the private email thread about it.

r- because this specific patch has some errors in it, as explained in line-by-line comments.

> Source/WebCore/platform/graphics/mac/FontCacheMac.mm:72
> +    CGFontSetShouldUseMulticache();

This will break the build on future OS versions, can you skip this one for now? I can deal with it based on WKSI source.

> Source/WebCore/platform/mac/SystemPrivateInterface.h:35
> +// This file contains non-public functions in Mac system frameworks. These are

In the cases where WebKit already declares non-public symbols itself, we usually do it near the point of use, i.e. top of the same file. I am not sure it is a good direction to have one giant file of these, as each such individual symbol should be imported with caution. In particular, commenting the point of use in this file already makes it dependent on the point of use, but the association of point of use can bitrot.

>>> Source/WebCore/platform/mac/SystemPrivateInterface.h:39
>>> +// Forward-declaring symbols is preferred to using the WebKitSystemIterface.
>> 
>> Why do you say this?
> 
> There's an on-going discussion. This bug is to discuss if that's actually true, but I wrote the patch assuming it is.
> 
> (In reply to comment #3)

I don't think this comment is useful. "Is preferred" gives the reader no information because it does not say who prefers it or why. I would suggest dropping it.

>> Source/WebCore/platform/mac/SystemPrivateInterface.h:42
>> +void _NSDrawCarbonThemeBezel(NSRect, BOOL enabled);
> 
> _NSDrawCarbonThemeBezel is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]

This declaration of _NSDrawCarbonThemeBezel is incorrect and the point of use is incorrect two (wrong parameters). If it works it is only by accident.

>> Source/WebCore/platform/mac/SystemPrivateInterface.h:43
>> +void _NSDrawCarbonThemeListBox(NSRect, BOOL enabled);
> 
> _NSDrawCarbonThemeListBox is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]

This declaration of _NSDrawCarbonThemeListBox is incorrect and the point of use is incorrect two (wrong parameters). If it works it is only by accident.

> Source/WebCore/rendering/RenderThemeMacShared.mm:726
> +        _NSDrawCarbonThemeBezel(r, isEnabled(o) && !isReadOnlyControl(o));

This call is wrong, as mentioned previously.

> Source/WebCore/rendering/RenderThemeMacShared.mm:761
> +    _NSDrawCarbonThemeListBox(r, isEnabled(o) && !isReadOnlyControl(o));

This call is wrong, as mentioned previously.

[Maciej Stachowiak]

(In reply to comment #6)
> I don't think that we have reached consensus yet. Redeclaring SPIs has an extremely unfortunate consequence - if you get it wrong, or if the SPI changes later, compiler won't tell you about this, and the outcome will be a difficult to diagnose bug.
> 
> It's something we've been doing for a long time with some SPIs (it mostly depended on personal preference whether to put something in WKSI or re-declare it), and the problem did happen before.
> 
> Additionally, please note that these are not APIs, so they may not exist in past or future OS releases. This needs to be handled gracefully at runtime by builds that are intended to have a single binary for multiple OS versions.

I'll just note for the record that, as the person who created WebKitSystemInterface, I personally would like to eliminate it entirely. I don't think the benefits over time have exceeded the costs, and many of the hypothetical reasons we wanted it in the first place have not really panned out.

[Robert Sesek]

Comment on attachment 174471 [details]
Patch

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

>> Source/WebCore/platform/graphics/mac/FontCacheMac.mm:72
>> +    CGFontSetShouldUseMulticache();
> 
> This will break the build on future OS versions, can you skip this one for now? I can deal with it based on WKSI source.

Reverted

>> Source/WebCore/platform/mac/SystemPrivateInterface.h:35
>> +// This file contains non-public functions in Mac system frameworks. These are
> 
> In the cases where WebKit already declares non-public symbols itself, we usually do it near the point of use, i.e. top of the same file. I am not sure it is a good direction to have one giant file of these, as each such individual symbol should be imported with caution. In particular, commenting the point of use in this file already makes it dependent on the point of use, but the association of point of use can bitrot.

OK. I've removed this file and now just declare in the files in which the functions are used.

>>> Source/WebCore/platform/mac/SystemPrivateInterface.h:42
>>> +void _NSDrawCarbonThemeBezel(NSRect, BOOL enabled);
>> 
>> _NSDrawCarbonThemeBezel is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
> 
> This declaration of _NSDrawCarbonThemeBezel is incorrect and the point of use is incorrect two (wrong parameters). If it works it is only by accident.

Thanks for pointing that out. I should have checked the disassembly rather than assuming the signatures for the WK functions are the same. Updated, and below.

[Robert Sesek]

Created attachment 178265 [details]
Patch,v2

[Brent Fulgham]

I propose that we close this bug, since the main reason for it is no longer relevant now that the Blink project has forked.

[Mark Rowe (bdash)]

Comment on attachment 178265 [details]
Patch,v2

Marking as r- since the change as-is won't compile. It's also not clear that this is the direction we're interested in heading.

[Brent Fulgham]

Comment on attachment 178265 [details]
Patch,v2

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

> Source/WebCore/platform/graphics/mac/ComplexTextControllerCoreText.mm:49
> +

This is now in CoreTextSPI.h, etc.

[Brent Fulgham]

This was handled in a series of other commits. E.g., r175379 <https://trac.webkit.org/changeset/175379>.