Bug 221594

<rdar://problem/74129129>

Created attachment 419678 [details] Patch

Created attachment 419680 [details] Patch

Comment on attachment 419680 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=419680&action=review The WebCore changes look good to me modulo my minor comments, but someone that knows more about WKActionSheetAssistant than I do should review those changes. > Source/WebCore/Modules/mediacontrols/MediaControlsHost.cpp:327 > + > + HashMap<MediaControlsContextMenuItem::ID, Variant<RefPtr<AudioTrack>, RefPtr<TextTrack>>> idMap; Nit: something like `using MenuTrackItem = Variant<RefPtr<AudioTrack>, RefPtr<TextTrack>>` would make this easier to parse. > Source/WebCore/Modules/mediacontrols/MediaControlsHost.cpp:407 > + auto selectedItem = idMap.get(selectedItemID); Might be worth adding something like `ASSERT(selectedItem != HashTraits<MediaControlsContextMenuItem::ID>::emptyValue())` > Source/WebCore/Modules/mediacontrols/MediaControlsHost.cpp:419 > + for (auto& track : idMap.values()) { > + if (auto* textTrack = WTF::get_if<RefPtr<TextTrack>>(track)) > + (*textTrack)->setMode(TextTrack::Mode::Disabled); > + } Doesn't HTMLMediaElement::setSelectedTextTrack already take care of this? > Source/WebCore/Modules/mediacontrols/MediaControlsHost.idl:68 > + [Conditional=MEDIA_CONTROLS_CONTEXT_MENUS, EnabledBySetting=MediaControlsContextMenus] undefined showMediaControlsContextMenu(HTMLElement target, ContextMenuOptions options); Does this need `EnabledAtRuntime=ModernMediaControls`, or is that obsolete (and so should be removed everywhere)? > Source/WebCore/page/MediaControlsContextMenuItem.h:48 > +struct MediaControlsContextMenuItem { > + using ID = uint64_t; > + static constexpr ID invalidID = 0; > + > + ID id { invalidID }; > + String title; > + String icon; > + bool isChecked { false }; > + Vector<MediaControlsContextMenuItem> children; > + > + template<class Encoder> void encode(Encoder&) const; > + template<class Decoder> static Optional<MediaControlsContextMenuItem> decode(Decoder&); > +}; Isn't `children` always empty except for the last entry? It should either be Optional<> or you could define one struct for the menu and another for the items? > Source/WebKit/UIProcess/ios/WKActionSheetAssistant.mm:1077 > +- (NSArray *)currentAvailableMediaControlsContextMenuItems s/currentAvailableMediaControlsContextMenuItems/currentlyAvailableMediaControlsContextMenuItems/

Comment on attachment 419680 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=419680&action=review >> Source/WebCore/Modules/mediacontrols/MediaControlsHost.cpp:327 >> + HashMap<MediaControlsContextMenuItem::ID, Variant<RefPtr<AudioTrack>, RefPtr<TextTrack>>> idMap; > > Nit: something like `using MenuTrackItem = Variant<RefPtr<AudioTrack>, RefPtr<TextTrack>>` would make this easier to parse. ooo good idea =D >> Source/WebCore/Modules/mediacontrols/MediaControlsHost.cpp:407 >> + auto selectedItem = idMap.get(selectedItemID); > > Might be worth adding something like `ASSERT(selectedItem != HashTraits<MediaControlsContextMenuItem::ID>::emptyValue())` I think `HashTable::checkKey` already does this >> Source/WebCore/Modules/mediacontrols/MediaControlsHost.cpp:419 >> + } > > Doesn't HTMLMediaElement::setSelectedTextTrack already take care of this? I was emulating the code inside `TrackSupport.prototype.tracksPanelSelectionDidChange`. AFAICT `HTMLMediaElement::setSelectedTextTrack` doesn't do this if the given `trackToSelect` is the special "automatic" or "off" track. >> Source/WebCore/Modules/mediacontrols/MediaControlsHost.idl:68 >> + [Conditional=MEDIA_CONTROLS_CONTEXT_MENUS, EnabledBySetting=MediaControlsContextMenus] undefined showMediaControlsContextMenu(HTMLElement target, ContextMenuOptions options); > > Does this need `EnabledAtRuntime=ModernMediaControls`, or is that obsolete (and so should be removed everywhere)? Oh yeah good call. I'll add that as well :) >> Source/WebCore/page/MediaControlsContextMenuItem.h:48 >> +}; > > Isn't `children` always empty except for the last entry? It should either be Optional<> or you could define one struct for the menu and another for the items? The idea here is to be able to build a tree. If a video has both audio and text tracks, this would be used to create two submenus that each have children. A couple of the LayoutTests I wrote dump this structure so you can see it. IMO `Optional<Vector<...>>` is kinda redundant in this case because if a menu item has children it's not selectable (at the very least I think this is the case for macOS/iOS menus). Yes I could add distinct subclasses for menu vs action, but I think `children.isEmpty()` is indicator enough as to whether or not it's a menu or an action and distinct subclasses would just make this code more complicated.

Comment on attachment 419680 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=419680&action=review >>> Source/WebCore/Modules/mediacontrols/MediaControlsHost.idl:68 >>> + [Conditional=MEDIA_CONTROLS_CONTEXT_MENUS, EnabledBySetting=MediaControlsContextMenus] undefined showMediaControlsContextMenu(HTMLElement target, ContextMenuOptions options); >> >> Does this need `EnabledAtRuntime=ModernMediaControls`, or is that obsolete (and so should be removed everywhere)? > > Oh yeah good call. I'll add that as well :) IIRC, `EnabledAtRuntime` is for runtime-enabled features (`WebCore::RuntimeEnabledFeatures`), and allows the bindings generator guards the IDL with a runtime enabled feature check: ``` if ($context->extendedAttributes->{EnabledAtRuntime}) { assert("Must specify value for EnabledAtRuntime.") if $context->extendedAttributes->{EnabledAtRuntime} eq "VALUE_IS_MISSING"; AddToImplIncludes("RuntimeEnabledFeatures.h"); my @flags = split(/&/, $context->extendedAttributes->{EnabledAtRuntime}); foreach my $flag (@flags) { push(@conjuncts, "RuntimeEnabledFeatures::sharedFeatures()." . ToMethodName($flag) . "Enabled()"); } } ``` > Source/WebKit/UIProcess/ios/WKActionSheetAssistant.mm:1041 > + NSMutableArray *array = [NSMutableArray array]; Nit - `-arrayWithCapacity:`? > Source/WebKit/WebProcess/WebPage/WebPage.cpp:7170 > + completionHandler(selectedItemID); Nit - does it work if you just pass in `WTFMove(completionHandler)` here, or is the outer completion handler needed? > Tools/WebKitTestRunner/ios/UIScriptControllerIOS.mm:1040 > + if (UIView *calloutBar = UICalloutBar.activeCalloutBar) { Nit - I need to double check, but I suspect `UICalloutBar.activeCalloutBar` is non-nil even if the callout bar is not currently shown. (The point of `activeCalloutBar`, `+[UIKeyboardImpl activeInstance]`, and the like are to avoid the cost of instantiating these singleton objects too early, usually during app launch).

Comment on attachment 419680 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=419680&action=review >> Source/WebKit/UIProcess/ios/WKActionSheetAssistant.mm:1041 >> + NSMutableArray *array = [NSMutableArray array]; > > Nit - `-arrayWithCapacity:`? This was just copypasta from above, but I'm happy to change it :) >> Source/WebKit/WebProcess/WebPage/WebPage.cpp:7170 >> + completionHandler(selectedItemID); > > Nit - does it work if you just pass in `WTFMove(completionHandler)` here, or is the outer completion handler needed? :extreme facepalm: >> Tools/WebKitTestRunner/ios/UIScriptControllerIOS.mm:1040 >> + if (UIView *calloutBar = UICalloutBar.activeCalloutBar) { > > Nit - I need to double check, but I suspect `UICalloutBar.activeCalloutBar` is non-nil even if the callout bar is not currently shown. (The point of `activeCalloutBar`, `+[UIKeyboardImpl activeInstance]`, and the like are to avoid the cost of instantiating these singleton objects too early, usually during app launch). Oh wow I completely missed that it was checking for `calloutBar.window`. Will change. Great catch!

Created attachment 419751 [details] Patch

Comment on attachment 419751 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=419751&action=review The test harness and context menu changes look good to me. I'm not familiar with the media and media-controls side of things though, so perhaps one of the media folks could take a closer look! > Source/WebKit/UIProcess/ios/WKActionSheetAssistant.mm:685 > - (void)ensureContextMenuInteraction Nit - should this become `ensureContextMenuInteractions`? (Or perhaps split this into two ensure methods, since there's no need to create and add `_dataDetectorContextMenuInteraction` when showing the media context menu). If you do decide to split this into two ensures, you could also just return the interaction from them, and then do `[self.ensureMediaControlsContextMenuInteraction _presentMenuAtLocation:_mediaControlsContextMenuTargetFrame.center()];` below. > Source/WebKit/UIProcess/ios/WKActionSheetAssistant.mm:1071 > + result[@"checked"] = [NSNumber numberWithBool:YES]; Nit - this can just be @YES.

Comment on attachment 419751 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=419751&action=review >> Source/WebKit/UIProcess/ios/WKActionSheetAssistant.mm:685 >> - (void)ensureContextMenuInteraction > > Nit - should this become `ensureContextMenuInteractions`? (Or perhaps split this into two ensure methods, since there's no need to create and add `_dataDetectorContextMenuInteraction` when showing the media context menu). > > If you do decide to split this into two ensures, you could also just return the interaction from them, and then do `[self.ensureMediaControlsContextMenuInteraction _presentMenuAtLocation:_mediaControlsContextMenuTargetFrame.center()];` below. oh yeah good idea

Created attachment 419777 [details] Patch also split `removeContextMenuInteraction` and move it outside of the lambda so that if a second request to show the contextmenu comes in while the first one is dismissing then it's not removed

Committed r272630: <https://commits.webkit.org/r272630> All reviewed patches have been landed. Closing bug and clearing flags on attachment 419777 [details].

unreviewed build fix r272631: <https://commits.webkit.org/r272631>