[webkit-reviews] review denied: [Bug 102405] Replace some trivial WebKitSystemInterface calls with direct SPI calls : [Attachment 174471] Patch

[bugzilla-daemon at webkit.org]

Maciej Stachowiak <mjs at apple.com> has denied Robert Sesek
<rsesek at chromium.org>'s request for review:
Bug 102405: Replace some trivial WebKitSystemInterface calls with direct SPI
calls
https://bugs.webkit.org/show_bug.cgi?id=102405

Attachment 174471: Patch
https://bugs.webkit.org/attachment.cgi?id=174471&action=review

------- Additional Comments from Maciej Stachowiak <mjs at apple.com>
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.