Bug 214941

Summary: Remove non-inclusive terminology from WebKit variable names and test-only SPI
Product: WebKit Reporter: Alex Christensen <achristensen>
Component: New BugsAssignee: Alex Christensen <achristensen>
Status: RESOLVED FIXED    
Severity: Normal CC: aperez, berto, bugs-noreply, caitp, calvaris, cgarcia, ews-watchlist, ggaren, gustavo, mcatanzaro, rwlbuis, thorton, webkit-bug-importer
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on:    
Bug Blocks: 213092    
Attachments:
Description Flags
Patch
none
Patch none

Description Alex Christensen 2020-07-29 15:15:20 PDT
Remove non-inclusive terminology from WebKit variable nmes and test-only SPI
Comment 1 Alex Christensen 2020-07-29 15:16:25 PDT
Created attachment 405510 [details]
Patch
Comment 2 EWS Watchlist 2020-07-29 15:17:11 PDT
Thanks for the patch. If this patch contains new public API please make sure it follows the guidelines for new WebKit2 GTK+ API. See http://trac.webkit.org/wiki/WebKitGTK/AddingNewWebKit2API
Comment 3 Tim Horton 2020-07-29 15:22:48 PDT
Comment on attachment 405510 [details]
Patch

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

> Source/WebKit/ChangeLog:3
> +        Remove non-inclusive terminology from WebKit variable nmes and test-only SPI

nmes

> Source/WebKit/UIProcess/API/glib/WebKitUserContent.cpp:156
>   * @level: A #WebKitUserStyleLevel

Need to check with GTK folks about changing their headerdoc.

> Source/WebKit/WebProcess/InjectedBundle/API/c/WKBundlePrivate.h:58
> -WK_EXPORT void WKBundleAddOriginAccessWhitelistEntry(WKBundleRef bundle, WKStringRef, WKStringRef, WKStringRef, bool);
> -WK_EXPORT void WKBundleRemoveOriginAccessWhitelistEntry(WKBundleRef bundle, WKStringRef, WKStringRef, WKStringRef, bool);
> -WK_EXPORT void WKBundleResetOriginAccessWhitelists(WKBundleRef bundle);
> +WK_EXPORT void WKBundleAddOriginAccessAllowListEntry(WKBundleRef bundle, WKStringRef, WKStringRef, WKStringRef, bool);
> +WK_EXPORT void WKBundleRemoveOriginAccessAllowListEntry(WKBundleRef bundle, WKStringRef, WKStringRef, WKStringRef, bool);
> +WK_EXPORT void WKBundleResetOriginAccessAllowLists(WKBundleRef bundle);

If these are renameable (thus unused), shouldn't we just delete them??? (or maybe they're only used by Safari and you're going to land a change there in sync? but then what about ToT-WebKit-against-older-Safari?)
Comment 4 Alex Christensen 2020-07-29 15:23:31 PDT
As seen later, they're only used by WebKitTestRunner, which we can change at the same time.
Comment 5 Alex Christensen 2020-07-29 15:23:39 PDT
Created attachment 405512 [details]
Patch
Comment 6 Tim Horton 2020-07-29 15:25:31 PDT
(In reply to Alex Christensen from comment #4)
> As seen later, they're only used by WebKitTestRunner, which we can change at
> the same time.

Fair!
Comment 7 Tim Horton 2020-07-29 15:25:50 PDT
Comment on attachment 405512 [details]
Patch

r+ for all but the GTK port parts
Comment 8 Alex Christensen 2020-07-29 15:30:42 PDT
Adding some GTK folks.
Comment 9 Geoffrey Garen 2020-07-29 15:43:12 PDT
Comment on attachment 405512 [details]
Patch

r=me
Comment 10 Alex Christensen 2020-07-29 15:43:54 PDT
Comment on attachment 405512 [details]
Patch

Requesting review from someone not from Apple.
Comment 11 Adrian Perez 2020-07-29 16:00:35 PDT
Comment on attachment 405512 [details]
Patch

The WPE/GTK bits look fine to me: only parameter names are changed,
which means the API (and ABI) remains the same—that's perfect :)
Comment 12 Michael Catanzaro 2020-07-29 16:08:14 PDT
Comment on attachment 405512 [details]
Patch

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

> Source/WebKit/UIProcess/API/glib/WebKitUserContent.cpp:158
> - * @whitelist: (array zero-terminated=1) (allow-none): A whitelist of URI patterns or %NULL
> - * @blacklist: (array zero-terminated=1) (allow-none): A blacklist of URI patterns or %NULL
> + * @allow_list: (array zero-terminated=1) (allow-none): An allow_list of URI patterns or %NULL
> + * @block_list: (array zero-terminated=1) (allow-none): A block_list of URI patterns or %NULL

Hm, this is actually public API. Changing parameter names is not an API break in C or C++, but it is for introspection. E.g. this could be called from python; if the python code is using positional parameters, it will break. In practice, I bet no apps are doing that, so it's probably fine... but if I'm wrong, anything that is will definitely break.

I guess we can try it anyway and revert if someone complains we broke their app. This API is relatively new, and it's relatively unlikely that apps are using it. Or we could take the safer approach and omit the changes to WebKitUserContent.cpp and the two public WebKitUserContent.h headers. From the title of this issue, I guess that was the intent anyway. CC other GTK devs for opinions on whether it's worth the risk.
Comment 13 Alex Christensen 2020-07-29 17:49:34 PDT
This bug is a part of bug 213092 which implies we hope to remove all references to the terms "whitelist" and "blacklist" and many other projects are doing the same.  See https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=49decddd39e5f6132ccd7d9fdc3d7c470b0061bb

Since the GTK API is relatively new, that probably means that if anyone is using it with strange python bindings, it's a project that is still maintained.  Since the ABI is unbroken, I would assume it's low risk.
Comment 14 Carlos Garcia Campos 2020-07-29 22:57:26 PDT
(In reply to Alex Christensen from comment #13)
> This bug is a part of bug 213092 which implies we hope to remove all
> references to the terms "whitelist" and "blacklist" and many other projects
> are doing the same.  See
> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/
> ?id=49decddd39e5f6132ccd7d9fdc3d7c470b0061bb
> 
> Since the GTK API is relatively new, that probably means that if anyone is
> using it with strange python bindings, it's a project that is still
> maintained.  Since the ABI is unbroken, I would assume it's low risk.

User content api is not relatively new, but I'm fine with the change anyway, it's very unlikely it can break any app.
Comment 15 Michael Catanzaro 2020-07-30 05:59:39 PDT
Yeah sorry, I was thinking this was the content filters API but it's actually the user stylesheets and user JS API.

Calling WebKitGTK APIs from scripting languages is actually very common on Linux, but the first python app I checked (eolie) is not using these functions at all, and they are not very commonly-used APIs, so after thinking on it for a night I agree the risk is relatively low.
Comment 16 Adrian Perez 2020-07-30 06:59:07 PDT
(In reply to Carlos Garcia Campos from comment #14)
> (In reply to Alex Christensen from comment #13)
> > This bug is a part of bug 213092 which implies we hope to remove all
> > references to the terms "whitelist" and "blacklist" and many other projects
> > are doing the same.  See
> > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/
> > ?id=49decddd39e5f6132ccd7d9fdc3d7c470b0061bb
> > 
> > Since the GTK API is relatively new, that probably means that if anyone is
> > using it with strange python bindings, it's a project that is still
> > maintained.  Since the ABI is unbroken, I would assume it's low risk.
> 
> User content api is not relatively new, but I'm fine with the change anyway,
> it's very unlikely it can break any app.

Also most languages which use the functions through GObject-Introspection
do not support named parameters on function calls (only their positions in
the call matter), and those will be fine with the change. I think the main
exception is Python where you can explicitly name parameters in a function
call expression:

  def do_something(a, b): # define a function.
      # ...
  do_something(b=1, a=2)  # call it naming the parameters explicitly.
  do_something(2, 1)      # this does the same as the previous call.

I wonder how many Python applications use WebKitGTK *and* specify parameter
names for the few functions changed by this patch. Definitely not Eolie, and
that's probably the biggest Python application I am aware of 🤔️
Comment 17 Alex Christensen 2020-07-30 08:00:46 PDT
Comment on attachment 405512 [details]
Patch

Requesting review from someone not from Apple.
Comment 18 EWS 2020-07-30 08:10:44 PDT
Committed r265081: <https://trac.webkit.org/changeset/265081>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 405512 [details].
Comment 19 Radar WebKit Bug Importer 2020-07-30 08:11:32 PDT
<rdar://problem/66327965>