May 2021 - webkit-dev - lists.webkit.org

Re: [webkit-dev] Feedback on Blink's text fragment directive proposal
by David Bokan 09 Jun '21

09 Jun '21

[sending (again, sorry) from correct e-mail] I think Nick's replies <https://lists.webkit.org/pipermail/webkit-dev/2019-December/030998.html> mostly still apply, some updated answers to those questions. (1) We’re concerned about compatibility issues in a world where some > browsers support this but not all. Aware browsers will strip `:~:`, but > unaware browsers won’t. I saw that on the blink-dev ItS thread, it was > mentioned that at least one site (webmd.com) totally breaks if any > fragment ID is exposed to the page. This makes it difficult to create a > link that uses this feature but which is safe in all browsers: > - Since there is no feature detection mechanism, it’s hard for a webpage > to know whether it should issue such a link. It would have to be based on > UA string checks, which is regrettable. > - A link meant for a supporting browser can end up in a non-supporting > browser, at the very least by copy paste from the URL field, and perhaps > through other features to share a link. > We do have a feature detection <https://github.com/WICG/scroll-to-text-fragment/#feature-detection-and-futu…> mechanism for this. On the latter point, this is true but we think implementing fragment directive stripping (removing the part after and including `:~:`) is trivial even if the UA doesn't wish to implement the text-fragment feature. FWIW, we haven't seen or heard of another such example since. > (2) The portions of this Community Group report that monkey patch other > standards (HTML, URL and CSSOM View I think?) should be turned into PRs > against those specifications. Monkeypatching other specs is not a good way > to build specifications for the long term. > We still need support from another vendor to start merging the monkey patches into the real standards - if Apple's supportive I'd be happy to start on that immediately. > (3) Text fragment trumping a regular fragment ID seems a bit strange. The > more natural semantic would be that the text search starts at the fragment, > so if there are multiple matches it’s possible to scroll to a more specific > one. It’s not clear why the fragment is instead entirely ignored. > This was discussed in more detail in issue#75 <https://github.com/WICG/scroll-to-text-fragment/issues/75>; I agree with Nick's point that the disambiguation syntax is already specific enough that starting from a fragment isn't necessary. This also keeps us mostly-compatible with the TextQuoteSelector <https://www.w3.org/TR/annotation-model/#text-quote-selector> specified in WebAnnotations which I think may have benefits for interaction with annotation applications. On Mon, Aug 31, 2020 at 1:31 PM David Bokan <bokan(a)google.com> wrote: > I think Nick's replies > <https://lists.webkit.org/pipermail/webkit-dev/2019-December/030998.html> > mostly still apply, some updated answers to those questions. > > (1) We’re concerned about compatibility issues in a world where some >> browsers support this but not all. Aware browsers will strip `:~:`, but >> unaware browsers won’t. I saw that on the blink-dev ItS thread, it was >> mentioned that at least one site (webmd.com) totally breaks if any >> fragment ID is exposed to the page. This makes it difficult to create a >> link that uses this feature but which is safe in all browsers: >> - Since there is no feature detection mechanism, it’s hard for a webpage >> to know whether it should issue such a link. It would have to be based on >> UA string checks, which is regrettable. >> - A link meant for a supporting browser can end up in a non-supporting >> browser, at the very least by copy paste from the URL field, and perhaps >> through other features to share a link. >> > > We do have a feature detection > <https://github.com/WICG/scroll-to-text-fragment/#feature-detection-and-futu…> > mechanism for this. > > On the latter point, this is true but we think implementing fragment > directive stripping (removing the part after and including `:~:`) is > trivial even if the UA doesn't wish to implement the text-fragment feature. > FWIW, we haven't seen or heard of another such example since. > > >> (2) The portions of this Community Group report that monkey patch other >> standards (HTML, URL and CSSOM View I think?) should be turned into PRs >> against those specifications. Monkeypatching other specs is not a good way >> to build specifications for the long term. >> > > We still need support from another vendor to start merging the monkey > patches into the real standards - if Apple's supportive I'd be happy to > start on that immediately. > > >> (3) Text fragment trumping a regular fragment ID seems a bit strange. The >> more natural semantic would be that the text search starts at the fragment, >> so if there are multiple matches it’s possible to scroll to a more specific >> one. It’s not clear why the fragment is instead entirely ignored. >> > > This was discussed in more detail in issue#75 > <https://github.com/WICG/scroll-to-text-fragment/issues/75>; I agree with > Nick's point that the disambiguation syntax is already specific enough that > starting from a fragment isn't necessary. This also keeps us > mostly-compatible with the TextQuoteSelector > <https://www.w3.org/TR/annotation-model/#text-quote-selector> specified > in WebAnnotations which I think may have benefits for interaction with > annotation applications. > > On Mon, Aug 31, 2020 at 12:42 PM Darin Adler <darin(a)apple.com> wrote: > >> On Aug 31, 2020, at 9:33 AM, David Bokan <bokan(a)chromium.org> wrote: >> >> We've made lots of improvements to the spec, notably around issues raised >> in Mozilla's standards-position >> <https://github.com/mozilla/standards-positions/issues/194>. >> >> >> Do you think those improvements address Maciej’s concerns from last >> December? Since you didn’t say that explicitly I was wondering what your >> take on that was. >> >> — Darin >> >

6 18

Request for Position: Virtual Keyboard Control
by Anupam Snigdha 02 Jun '21

02 Jun '21

Hi webkit-dev, This is a request for Webkit's position on Virtual Keyboard Control feature: Explainer: https://github.com/MicrosoftEdge/MSEdgeExplainers/blob/main/VirtualKeyboard… https://github.com/MicrosoftEdge/MSEdgeExplainers/blob/main/VirtualKeyboard… TAG review threads: https://github.com/w3ctag/design-reviews/issues/507 https://github.com/w3ctag/design-reviews/issues/498 TPAC discussions where we discussed about the VK API shape and events: https://www.w3.org/2020/10/TPAC/breakout-schedule.html#virtual-keyboard https://www.w3.org/2020/10/30-editing-minutes.html Demo page: https://bocupp-microsoft.github.io/VirtualKeyboard/geometrychange.html Please let us know if you have any feedback! Thanks, Anupam

1 1

Clang Thread Safety Analysis
by Chris Dumez 30 May '21

30 May '21

Clang Thread Safety Analysis WTF::Lock now supports Clang Thread Safety Analysis <https://clang.llvm.org/docs/ThreadSafetyAnalysis.html>. It is a C++ language extension which warns about potential race conditions in code, at compile time. It is the same great Lock, but with some extra help from clang to make our multi-threaded code safer by giving out compilation errors when thread unsafe code is detected. Annotations Just by using WTF::Lock, Clang will already be able to do some basic validation. However, to take full advantage of it, there are a few annotations you should know about. Note that you'll see those annotations throughout the code base already as a few of us have been busy adopting them. All these annotations are declared in wtf/ThreadSafetyAnalysis.h <http://trac.webkit.org/browser/webkit/trunk/Source/WTF/wtf/ThreadSafetyAnal…>. WTF_GUARDED_BY_LOCK() WTF_GUARDED_BY_LOCK is an attribute on data members, which declares that the data member is protected by the given lock. Read operations on the data require shared access, while write operations require exclusive access. WTF_POINTEE_GUARDED_BY_LOCK is similar, but is intended for use on pointers and smart pointers. There is no constraint on the data member itself, but the data that it points to is protected by the given lock. class Foo { Vector<String> m_strings WTF_GUARDED_BY_LOCK(m_stringsLock); Lock m_stringsLock; }; You will get a compiler error if you try to access or modify m_strings without grabbing the m_stringsLock lock first. WTF_REQUIRES_LOCK() WTF_REQUIRES_LOCK is an attribute on functions or methods, which declares that the calling thread must have exclusive access to the given locks. More than one lock may be specified. The locks must be held on entry to the function, and must still be held on exit. class Foo { void addString(String&&) WTF_REQUIRES_LOCK(m_stringsLock); Vector<String> m_strings WTF_GUARDED_BY_LOCK(m_stringsLock); Lock m_stringsLock; }; You will get a compiler error if you try to call addString() without grabbing the m_stringsLock lock first. This also allows to compiler to know that the addString() implementation can modify the m_strings data member without grabbing the lock first. Another good use case: static Lock globalCacheLock; static HashMap<String, String>& globalCache() WTF_REQUIRES_LOCK(globalCacheLock) { static NeverDestroyed<HashMap<String, String>> globalCache; return globalCache; } This will force all call sites to grab the globalCacheLock lock before calling globalCache(). Previously, we may have passed a LockHolder& as parameter to try and convey that. We have been updating code to stop passing such parameters though as it is no longer useful. WTF_ACQUIRES_LOCK() / WTF_RELEASES_LOCK() WTF_ACQUIRES_LOCK is an attribute on functions or methods declaring that the function acquires a lock, but does not release it. The given lock must not be held on entry, and will be held on exit. WTF_RELEASES_LOCK declares that the function releases the given lock. The lock must be held on entry, and will no longer be held on exit. class Foo { public: void suspend() WTF_ACQUIRES_LOCK(m_suspensionLock) { m_suspensionLock.lock(); m_isSuspended = true; } void resume() WTF_RELEASE_LOCK(m_suspensionLock) { m_isSuspended = false; m_suspensionLock.unlock(); } private: Lock m_suspensionLock; bool m_isSuspended; }; Without these annotations, you'd get a compiler error stating that you failed to unlock m_suspensionLock before returning in suspend() and that you unlocked m_suspensionLock in resume() even though it was not previously locked. WTF_RETURNS_LOCK() WTF_RETURNS_LOCK is an attribute on functions or methods, which declares that the function returns a reference to the given lock. class Foo { public: Lock& lock() WTF_RETURNS_LOCK(m_lock) { return m_lock; } private: Lock m_lock; }; WTF_EXCLUDES_LOCK() WTF_EXCLUDES_LOCK is an attribute on functions or methods, which declares that the caller must not hold the given lock. This annotation is used to prevent deadlock. Many mutex implementations are not re-entrant, so deadlock can occur if the function acquires the mutex a second time. class Foo { void addString(String&& string) WTF_EXCLUDES_LOCK(m_stringsLock) { Locker locker { m_stringsLock }; m_string.append(WTFMove(string)); } Vector<String> m_strings WTF_GUARDED_BY_LOCK(m_stringsLock); Lock m_stringsLock; }; WTF_IGNORES_THREAD_SAFETY_ANALYSIS WTF_IGNORES_THREAD_SAFETY_ANALYSIS is an attribute on functions or methods, which turns off thread safety checking for that method. It provides an escape hatch for functions which are either (1) deliberately thread-unsafe, or (2) are thread-safe, but too complicated for the analysis to understand. Reasons for (2) are be described in the Known Limitations <https://clang.llvm.org/docs/ThreadSafetyAnalysis.html#limitations>. class Foo { void unsafeIncrement() WTF_IGNORES_THREAD_SAFETY_ANALYSIS { ++m_counter; } unsigned m_counter WTF_GUARDED_BY_LOCK(m_counterLock) { 0 }; Lock m_counterLock; }; Unlike the other attributes, WTF_IGNORES_THREAD_SAFETY_ANALYSIS is not part of the interface of a function, and should thus be placed on the function definition (in the .cpp file) rather than on the function declaration (in the header). assertIsHeld() These are attributes on a function or method which asserts the calling thread already holds the given lock, for example by performing a run-time test and terminating if the lock is not held. Presence of this annotation causes the analysis to assume the lock is held after calls to the annotated function. class Foo { void waitForString() { Locker locker { m_stringsLock }; m_stringsCondition.wait(m_stringsLock, [&] { assertIsHeld(m_stringsLock); // Needed as the compiler is not able to tell we have the lock. return !m_strings.isEmpty(); }); } void populateFromProviders() { Locker locker { m_stringsLock }; forEachProvider([&](Provider& provider) { assertIsHeld(m_stringsLock); // Needed as the compiler is not able to tell we have the lock. m_strings.append(provider.pullStrings()); }); } Vector<String> m_strings WTF_GUARDED_BY_LOCK(m_stringsLock); Lock m_stringsLock; Condition m_stringsCondition; }; The most common case in WebKit is when you hold a lock in a function then have a lambda in this function that runs synchronously. Even though the lambda runs synchronously and you're holding the lock, the compiler doesn't know that. As a result, if the lambda tries to access a protected data member, you will get a compiler error unless you use assertIsHeld() at the beginning of the lambda. Limitations There are a few things Clang thread safety analysis is not able to do. You can find examples of these here <https://clang.llvm.org/docs/ThreadSafetyAnalysis.html#known-limitations>. I am also documenting below a few things that had to change in WebKit as a result. No more holdLock() / tryHoldLock() holdLock() / tryHoldLock() were not compatible with Clang Thread Safety Analysis so we removed them. Instead, the following patterns are now recommended: void foo() { // Previously: // auto locker = holdLock(m_lock); Locker locker { m_lock }; // With C++17 the type of the Locker<T> is deduced based on type of m_lock. } void bar() { /* Previously: auto locker = tryHoldLock(m_lock); if (!locker) return; */ if (!m_lock.tryLock()) return; Locker locker { AdoptLock, m_lock }; } } In my opinion, the tryHoldLock() alternative is not as nice but it enables the safety analysis and we don't use tryHoldLock()very often in the code base. In the future, I believe we should be able to make something like this work: void bar() { Locker locker { DeferLock, m_lock }; if (!locker.tryLock()) return; } Locker<Lock> is no longer movable To support Clang thread safety analysis, Locker<Lock> no longer has a move constructor. Locker<Lock> can no longer wrap a null Lock This was used for conditional locking like so: Locker locker { shouldLock ? &m_lock : nullptr }; What's next? Please adopt annotations above in new code to improve thread safety in our code base. If you run into trouble adopting them or modifying code that already uses the annotations, please feel free to ask me or Kimmo for help as we've been porting a lot of the existing code. Hopefully this email is a good reference too. It was a fairly large project to adopt Clang thread safety analysis in WebKit and some things still need work / clean up: WTF::CheckedLock is currently an alias to WTF::Lock and will go away very soon once no code is referencing it. WTF::UncheckedLock is the previous version of the lock that did not support thread safety analysis and with its previous Locker. It is still used in WebKit in some places for now because code using it was either not trivial to port or we just did not have time to update it. Please avoid avoid using WTF::UncheckedLock in new code if you can. For now, the plan is to work towards moving all code from WTF::UncheckedLock to WTF::Lock but it may take some time, discussion and possibly some Locker<Lock> changes. The main issue really has been the Locker API differences documented above. JavaScriptCore is by far the biggest user of WTF::UncheckedLock right now. Even though we adopted annotations in many places already, more code can adopt it to improve thread safety. Some code uses WTF_IGNORES_THREAD_SAFETY_ANALYSIS with FIXME comments indicating that it is likely not thread-safe. We need to review those and either make them thread-safe or clarify why it is OK as is. We will likely try and make the Locker<Lock> more flexible to support more use cases. We are a bit limited by what clang supports but I believe there is room of improvement. Credits Credits to Kimmo for introducing the checked Lock to WebKit and helping with the adoption! Big thanks to everyone who helped with reviews and feedback as well. --  Chris Dumez

1 1

Let's use -Werror on EWS?
by Michael Catanzaro 25 May '21

25 May '21

Hi, I'd like to suggest turning on -Werror on at least the GTK and WPE EWS, to reduce the amount of time I spend cleaning up newly-introduced build warnings. ;) If we're worried about too many spurious build failures, let's at least build with a few flags to prevent the most common GCC warnings that developers using Clang will never notice: -Werror=return-type, -Werror=class-memaccess, and -Werror=pessimizing-move. These are simple to avoid if you see the warning from GCC, but very easy to miss if you develop with Clang. I guess these account for more than half of new GCC warnings introduced into WebKit. I'd also like to see -Werror=unused, since it's easy to introduce these warnings for other ports when doing anything involving conditional compilation. That might require some CMake changes, though (as we don't want to break -Wno-unused-parameter, which we use when building Source/WebKit and several directories under Tools). But really, rather than cherry-picking particular warning flags, using -Werror seems simplest to me. Problematic warnings should be disabled or suppressed. I do *not* suggest using -Werror on any non-EWS bots, since that will make gardening harder for almost no benefit. We do not want to lose test results due to a missing UNUSED_PARAM() or RELEASE_ASSERT_NOT_REACHED() somewhere. I also certainly do not suggest using it by default in CMake, which would really annoy our distributors. I would only use it in the EWS bot config. Michael

2 6

Re: [webkit-dev] Position Request on Window Controls Overlay
by Diego Gonzalez 21 May '21

21 May '21

Hola webkit-dev, This is a request for Webkit's position on Window Controls Overlay for installed Web Applications. Explainer: https://github.com/WICG/window-controls-overlay/blob/main/explainer.md web.dev post: https://web.dev/window-controls-overlay/ Summary: Allows a developer to customize part of the title bar that is not covered by the window controls overlay (web app menu, maximize/restore, minimize, close). It consists of 4 parts: * (JS) New object on the window.navigator.windowControlsOverlay. * (CSS) New Environmental variables: (titlebar-area-{x/y/width/height}). * (CSS) standardisation of -webkit-app-region:drag and -webkit-app-region: no-drag. * (Manifest) addition of "display-override" member Contact email: luigonza at microsoft.com TAG review: https://github.com/w3ctag/design-reviews/issues/481 Very keen on hearing your ideas. Regards, Diego González-Zúñiga PM, Microsoft Edge [cid:image001.png@01D74E64.8F1D7ED0]

1 0

Is anyone building with USE(DIRECT2D)?
by Sam Weinig 20 May '21

20 May '21

Hi WebKit-dev, Is anyone still using attempting to build using USE(DIRECT2D) enabled? Recently, I updated files to “keep” it working, but in some I have seen things that obviously wouldn’t compile and think it is probably unmaintained. So is anyone using it? - Sam

1 0

Request for position: HTTP 103 Early Hints
by Kenichi Ishibashi 19 May '21

19 May '21

Hi webkit-dev, I'm requesting a position on HTTP 103 Early Hints. Servers can send Early Hints before sending the final response so that it can give clients some hints like preload requests. Specification Title: HTTP 103 Early Hints Specification URL: https://datatracker.ietf.org/doc/html/rfc8297 Chromium's design doc: https://docs.google.com/document/d/1gCh_CnfrJq_VL7aGoq6skc7sn4yn5pKsM0gkHe5… ChromeStatus.com entry: https://chromestatus.com/feature/5207422375297024 TAG design review request: https://github.com/w3ctag/design-reviews/issues/638 Mozilla Request for Position: https://github.com/mozilla/standards-positions/issues/530 Thanks,

1 0

Request for position: Allow Navigator.registerProtocolHandler to register FTP related schemes.
by Asanka Herath 17 May '21

17 May '21

Hi folks, I sent out an 'intent to implement and ship' for adding FTP related schemes to the list of safelisted schemes for use with registerProtocolHandler. I'd like to solicit your opinion on this change. Thanks, - Asanka Summary Add "ftp", "ftps", and "sftp" to the list of protocols available for registration via registerProtocolHandler(). FTP is now deprecated and will imminently be removed from the codebase. Allowing websites to register themselves as handlers for ftp:// URLs makes it possible to build easy to use gateways for legacy ftp sites. In addition we propose adding related protocols "ftps" and "sftp" to the list. Google Chrome/Chromium was not handling these URL schemes. Blink componentBlink>HTML>CustomHandlers <https://bugs.chromium.org/p/chromium/issues/list?q=component:Blink%3EHTML%3…> Search tagsftp <https://www.chromestatus.com/features#tags:ftp>, registerprotocolhandler <https://www.chromestatus.com/features#tags:registerprotocolhandler> TAG review TAG review statusNot applicable Risks Interoperability and Compatibility Gecko: Worth prototyping ( https://github.com/mozilla/standards-positions/issues/513) Suggested filtering out credentials from outgoing FTP links. WebKit: N/A Safari does not support `registerProtocolHandler`. Web developers: Positive ( https://bugs.chromium.org/p/chromium/issues/detail?id=333943#c57) Security Same as other schemes in registerProtocolHandler. Since ftp was a protocol that used to be supported by the browser, internal ftp links might exist that assume in-browser support in assessing their security risks. A custom protocol handler will necessarily expose ftp URLs to the handler. Registering one may violate administrator assumptions about the URLs not leaving their organization when users click on them. Is this feature fully tested by web-platform-tests <https://chromium.googlesource.com/chromium/src/+/master/docs/testing/web_pl…> ?No Flag name Tracking bughttps://crbug.com/1199027 Link to entry on the Chrome Platform Status https://www.chromestatus.com/feature/5762628536238080 Links to previous Intent discussionsIntent to Ship: https://groups.google.com/a/chromium.org/g/blink-dev/c/ABhlioapE0E/m/du8Jv9…

1 0

Request for Position on :has() pseudo class
by Byungwoo Lee 13 May '21

13 May '21

Hello WebKit devs, This is a request for getting position on the :has() pseudo class. The :has() pseudo class is a selector that specifies elements which has at least one element that matches the relative selector passed as an argument. (https://www.w3.org/TR/selectors-4/#relational) Unlike other selectors, it gives a way to apply a style rule to preceding elements (preceding siblings / ancestors / preceding siblings of ancestors) of a certain elements. This difference is attractive to web developers, but also it generates lots of concerns mainly about performance and complexity. We thought that, clarifying those concerns would be helpful for the discussion. So we started to check feasibility on blink, and were able to get some meaningful and reasonable results from this step. Based on the results, we are going to move forward by prototyping. You can get the details about the current status in here. * Explainer: https://github.com/Igalia/explainers/tree/main/css/has * Test summary: https://css-has.glitch.me/ Regards, Byungwoo

1 0

Request for Position on Delegated Ink Trails
by Mario Bianucci 12 May '21

12 May '21

Hi webkit-dev, Apologies for the noise, but bringing this back up to the top of people's inboxes. This is a request for WebKit's position on Delegated Ink Trails. It is currently in progress behind a flag in Chromium. The spec is in progress, but until it is available, here is a link to the WICG explainer: https://github.com/WICG/ink-enhancement Thanks, Mario Bianucci

2 1