Bug 214941 – Remove non-inclusive terminology from WebKit variable names and test-only SPI

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Alex Christensen]

Remove non-inclusive terminology from WebKit variable nmes and test-only SPI

[Alex Christensen]

Created attachment 405510 [details]
Patch

[EWS Watchlist]

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

[Tim Horton]

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?)

[Alex Christensen]

As seen later, they're only used by WebKitTestRunner, which we can change at the same time.

[Alex Christensen]

Created attachment 405512 [details]
Patch

[Tim Horton]

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

[Tim Horton]

Comment on attachment 405512 [details]
Patch

r+ for all but the GTK port parts

[Alex Christensen]

Adding some GTK folks.

[Geoffrey Garen]

Comment on attachment 405512 [details]
Patch

r=me

[Alex Christensen]

Comment on attachment 405512 [details]
Patch

Requesting review from someone not from Apple.

[Adrian Perez]

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 :)

[Michael Catanzaro]

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.

[Alex Christensen]

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.

[Carlos Garcia Campos]

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

[Michael Catanzaro]

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.

[Adrian Perez]

(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 🤔️

[Alex Christensen]

Comment on attachment 405512 [details]
Patch

Requesting review from someone not from Apple.

[EWS]

Committed r265081: <https://trac.webkit.org/changeset/265081>

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

[Radar WebKit Bug Importer]

<rdar://problem/66327965>