WebKit Bugzilla
New
Browse
Log In
×
Sign in with GitHub
or
Remember my login
Create Account
·
Forgot Password
Forgotten password account recovery
RESOLVED FIXED
214941
Remove non-inclusive terminology from WebKit variable names and test-only SPI
https://bugs.webkit.org/show_bug.cgi?id=214941
Summary
Remove non-inclusive terminology from WebKit variable names and test-only SPI
Alex Christensen
Reported
2020-07-29 15:15:20 PDT
Remove non-inclusive terminology from WebKit variable nmes and test-only SPI
Attachments
Patch
(42.64 KB, patch)
2020-07-29 15:16 PDT
,
Alex Christensen
no flags
Details
Formatted Diff
Diff
Patch
(42.64 KB, patch)
2020-07-29 15:23 PDT
,
Alex Christensen
no flags
Details
Formatted Diff
Diff
Show Obsolete
(1)
View All
Add attachment
proposed patch, testcase, etc.
Alex Christensen
Comment 1
2020-07-29 15:16:25 PDT
Created
attachment 405510
[details]
Patch
EWS Watchlist
Comment 2
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
Tim Horton
Comment 3
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?)
Alex Christensen
Comment 4
2020-07-29 15:23:31 PDT
As seen later, they're only used by WebKitTestRunner, which we can change at the same time.
Alex Christensen
Comment 5
2020-07-29 15:23:39 PDT
Created
attachment 405512
[details]
Patch
Tim Horton
Comment 6
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!
Tim Horton
Comment 7
2020-07-29 15:25:50 PDT
Comment on
attachment 405512
[details]
Patch r+ for all but the GTK port parts
Alex Christensen
Comment 8
2020-07-29 15:30:42 PDT
Adding some GTK folks.
Geoffrey Garen
Comment 9
2020-07-29 15:43:12 PDT
Comment on
attachment 405512
[details]
Patch r=me
Alex Christensen
Comment 10
2020-07-29 15:43:54 PDT
Comment on
attachment 405512
[details]
Patch Requesting review from someone not from Apple.
Adrian Perez
Comment 11
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 :)
Michael Catanzaro
Comment 12
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.
Alex Christensen
Comment 13
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.
Carlos Garcia Campos
Comment 14
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.
Michael Catanzaro
Comment 15
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.
Adrian Perez
Comment 16
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 🤔️
Alex Christensen
Comment 17
2020-07-30 08:00:46 PDT
Comment on
attachment 405512
[details]
Patch Requesting review from someone not from Apple.
EWS
Comment 18
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]
.
Radar WebKit Bug Importer
Comment 19
2020-07-30 08:11:32 PDT
<
rdar://problem/66327965
>
Note
You need to
log in
before you can comment on or make changes to this bug.
Top of Page
Format For Printing
XML
Clone This Bug