Title: [280676] trunk/Source/WebCore
- Revision
- 280676
- Author
- [email protected]
- Date
- 2021-08-04 19:46:31 -0700 (Wed, 04 Aug 2021)
Log Message
REGRESSION (r280374): ASSERTION FAILED: Completion handler should not be called more than once under WebCore::MediaControlsContextMenuProvider::contextMenuItemSelected
https://bugs.webkit.org/show_bug.cgi?id=228725
<rdar://problem/81437221>
Reviewed by Eric Carlson.
The contextmenu system used by (modern) media controls are a bit wonky in that it has to
support both macOS and iOS, which use wildly different mechanisms. The former has distinct
methods for handling when a contextmenu item is selected vs when the menu is dismissed (at
least as of r280374). The latter has a single method that handles both. Additionally, the
(modern) media controls JS expects the following from `showMediaControlsContextMenu`:
1. `showMediaControlsContextMenu` will only `return true` if the contextmenu will be shown
2. the callback provided to `showMediaControlsContextMenu` will always/only be invoked when
the contextmenu is dismissed (regardless of whether an item is selected)
3. if an item is selected, the logic for that will be handled by the `MediaControlsHost`
This patch primarily addresses #2, but also slightly adjusts the code to fix #1. It does #1
by moving the call that saves the callback further down. On iOS, #2 already works. On macOS,
it does #2 by changing from `CompletionHandler` to `Function`, allowing it to be called more
than once, with the understanding that the JS callback will not be invoked more than once.
This way, macOS can match the behavior of iOS by eagerly invoking the JS callback when a
contextmenu item is selected without waiting for the menu to actually dismiss, while still
handling the contextmenu being dismissed without an item being selected (and also not having
to worry about whether the `CompletionHandler` has already been invoked).
* Modules/mediacontrols/MediaControlsHost.h:
* Modules/mediacontrols/MediaControlsHost.cpp:
(WebCore::MediaControlsContextMenuProvider::create):
(WebCore::MediaControlsContextMenuProvider::MediaControlsContextMenuProvider):
(WebCore::MediaControlsContextMenuProvider::didDismissContextMenu):
(WebCore::MediaControlsContextMenuProvider::contextMenuCleared):
(WebCore::MediaControlsHost::showMediaControlsContextMenu):
Modified Paths
Diff
Modified: trunk/Source/WebCore/ChangeLog (280675 => 280676)
--- trunk/Source/WebCore/ChangeLog 2021-08-05 02:15:54 UTC (rev 280675)
+++ trunk/Source/WebCore/ChangeLog 2021-08-05 02:46:31 UTC (rev 280676)
@@ -1,3 +1,37 @@
+2021-08-04 Devin Rousso <[email protected]>
+
+ REGRESSION (r280374): ASSERTION FAILED: Completion handler should not be called more than once under WebCore::MediaControlsContextMenuProvider::contextMenuItemSelected
+ https://bugs.webkit.org/show_bug.cgi?id=228725
+ <rdar://problem/81437221>
+
+ Reviewed by Eric Carlson.
+
+ The contextmenu system used by (modern) media controls are a bit wonky in that it has to
+ support both macOS and iOS, which use wildly different mechanisms. The former has distinct
+ methods for handling when a contextmenu item is selected vs when the menu is dismissed (at
+ least as of r280374). The latter has a single method that handles both. Additionally, the
+ (modern) media controls JS expects the following from `showMediaControlsContextMenu`:
+ 1. `showMediaControlsContextMenu` will only `return true` if the contextmenu will be shown
+ 2. the callback provided to `showMediaControlsContextMenu` will always/only be invoked when
+ the contextmenu is dismissed (regardless of whether an item is selected)
+ 3. if an item is selected, the logic for that will be handled by the `MediaControlsHost`
+ This patch primarily addresses #2, but also slightly adjusts the code to fix #1. It does #1
+ by moving the call that saves the callback further down. On iOS, #2 already works. On macOS,
+ it does #2 by changing from `CompletionHandler` to `Function`, allowing it to be called more
+ than once, with the understanding that the JS callback will not be invoked more than once.
+ This way, macOS can match the behavior of iOS by eagerly invoking the JS callback when a
+ contextmenu item is selected without waiting for the menu to actually dismiss, while still
+ handling the contextmenu being dismissed without an item being selected (and also not having
+ to worry about whether the `CompletionHandler` has already been invoked).
+
+ * Modules/mediacontrols/MediaControlsHost.h:
+ * Modules/mediacontrols/MediaControlsHost.cpp:
+ (WebCore::MediaControlsContextMenuProvider::create):
+ (WebCore::MediaControlsContextMenuProvider::MediaControlsContextMenuProvider):
+ (WebCore::MediaControlsContextMenuProvider::didDismissContextMenu):
+ (WebCore::MediaControlsContextMenuProvider::contextMenuCleared):
+ (WebCore::MediaControlsHost::showMediaControlsContextMenu):
+
2021-08-04 Dana Estra <[email protected]>
REGRESSION (r280492): Clicking backspace on sign in fields results in deleting two characters instead of one.
Modified: trunk/Source/WebCore/Modules/mediacontrols/MediaControlsHost.cpp (280675 => 280676)
--- trunk/Source/WebCore/Modules/mediacontrols/MediaControlsHost.cpp 2021-08-05 02:15:54 UTC (rev 280675)
+++ trunk/Source/WebCore/Modules/mediacontrols/MediaControlsHost.cpp 2021-08-05 02:46:31 UTC (rev 280676)
@@ -61,7 +61,7 @@
#include "VTTCue.h"
#include "VoidCallback.h"
#include <_javascript_Core/JSCJSValueInlines.h>
-#include <wtf/CompletionHandler.h>
+#include <wtf/Function.h>
#include <wtf/JSONValues.h>
#include <wtf/Scope.h>
#include <wtf/UUID.h>
@@ -365,13 +365,13 @@
#if ENABLE(CONTEXT_MENUS) && USE(ACCESSIBILITY_CONTEXT_MENUS)
class MediaControlsContextMenuProvider final : public ContextMenuProvider {
public:
- static Ref<MediaControlsContextMenuProvider> create(Vector<ContextMenuItem>&& items, CompletionHandler<void(uint64_t)>&& callback)
+ static Ref<MediaControlsContextMenuProvider> create(Vector<ContextMenuItem>&& items, Function<void(uint64_t)>&& callback)
{
return adoptRef(*new MediaControlsContextMenuProvider(WTFMove(items), WTFMove(callback)));
}
private:
- MediaControlsContextMenuProvider(Vector<ContextMenuItem>&& items, CompletionHandler<void(uint64_t)>&& callback)
+ MediaControlsContextMenuProvider(Vector<ContextMenuItem>&& items, Function<void(uint64_t)>&& callback)
: m_items(WTFMove(items))
, m_callback(WTFMove(callback))
{
@@ -390,8 +390,10 @@
void didDismissContextMenu() override
{
- if (m_callback)
+ if (!m_didDismiss) {
+ m_didDismiss = true;
m_callback(ContextMenuItemTagNoAction);
+ }
}
void contextMenuItemSelected(ContextMenuAction action, const String&) override
@@ -401,8 +403,7 @@
void contextMenuCleared() override
{
- if (m_callback)
- m_callback(ContextMenuItemTagNoAction);
+ didDismissContextMenu();
m_items.clear();
}
@@ -412,7 +413,8 @@
}
Vector<ContextMenuItem> m_items;
- CompletionHandler<void(uint64_t)> m_callback;
+ Function<void(uint64_t)> m_callback;
+ bool m_didDismiss { false };
};
class MediaControlsContextMenuEventListener final : public EventListener {
@@ -464,13 +466,6 @@
if (m_showMediaControlsContextMenuCallback)
return false;
- m_showMediaControlsContextMenuCallback = WTFMove(callback);
-
- auto invokeCallbackAtScopeExit = makeScopeExit([&, protectedThis = makeRef(*this)] {
- if (m_showMediaControlsContextMenuCallback)
- std::exchange(m_showMediaControlsContextMenuCallback, nullptr)->handleEvent();
- });
-
if (!m_mediaElement)
return false;
@@ -655,15 +650,25 @@
ASSERT(!idMap.isEmpty());
- auto handleItemSelected = [weakMediaElement = makeWeakPtr(mediaElement), idMap = WTFMove(idMap), invokeCallbackAtScopeExit = WTFMove(invokeCallbackAtScopeExit)] (MenuItemIdentifier selectedItemID) {
+ m_showMediaControlsContextMenuCallback = WTFMove(callback);
+
+ auto handleItemSelected = [weakThis = makeWeakPtr(this), idMap = WTFMove(idMap)] (MenuItemIdentifier selectedItemID) {
+ if (!weakThis)
+ return;
+ Ref strongThis = *weakThis;
+
+ auto invokeCallbackAtScopeExit = makeScopeExit([strongThis] {
+ if (auto showMediaControlsContextMenuCallback = std::exchange(strongThis->m_showMediaControlsContextMenuCallback, nullptr))
+ showMediaControlsContextMenuCallback->handleEvent();
+ });
+
if (selectedItemID == invalidMenuItemIdentifier)
return;
- if (!weakMediaElement)
+ if (!strongThis->m_mediaElement)
return;
+ auto& mediaElement = *strongThis->m_mediaElement;
- auto& mediaElement = *weakMediaElement;
-
UserGestureIndicator gestureIndicator(ProcessingUserGesture, &mediaElement.document());
auto selectedItem = idMap.get(selectedItemID);
Modified: trunk/Source/WebCore/Modules/mediacontrols/MediaControlsHost.h (280675 => 280676)
--- trunk/Source/WebCore/Modules/mediacontrols/MediaControlsHost.h 2021-08-05 02:15:54 UTC (rev 280675)
+++ trunk/Source/WebCore/Modules/mediacontrols/MediaControlsHost.h 2021-08-05 02:46:31 UTC (rev 280676)
@@ -45,7 +45,7 @@
class TextTrackList;
class VoidCallback;
-class MediaControlsHost : public RefCounted<MediaControlsHost> {
+class MediaControlsHost final : public RefCounted<MediaControlsHost>, public CanMakeWeakPtr<MediaControlsHost> {
WTF_MAKE_FAST_ALLOCATED(MediaControlsHost);
public:
static Ref<MediaControlsHost> create(HTMLMediaElement&);
_______________________________________________
webkit-changes mailing list
[email protected]
https://lists.webkit.org/mailman/listinfo/webkit-changes
