Bug 201864 – Remove the "Show Link Previews" and "Hide Link Previews" action menus in the preview platter

• Component: New Bugs
• Status: RESOLVED FIXED

[Dean Jackson]

Remove the "Show Link Previews" and "Hide Link Previews" action menus in the preview platter

[Dean Jackson]

<rdar://55190038>

[Dean Jackson]

Created attachment 378948 [details]
Patch

[Dean Jackson]

Created attachment 378949 [details]
Patch

[Simon Fraser (smfr)]

Comment on attachment 378949 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=378949&action=review

> Source/WebKit/ChangeLog:5
> +        https://bugs.webkit.org/show_bug.cgi?id=201864
> +

Missing radar number.

> Tools/TestWebKitAPI/Tests/WebKitCocoa/ContextMenus.mm:262
> +        return [UIViewController new];

autorelease?

[Dean Jackson]

Committed r249950: <https://trac.webkit.org/changeset/249950>

[Aakash Jain]

(In reply to Dean Jackson from comment #5)
> Committed r249950: <https://trac.webkit.org/changeset/249950>
This seems to have broken the iOS build, as also indicated the EWS.

[Aakash Jain]

This seems to be the relevant build error:

/Volumes/Data/worker/iOS-12-Build-EWS/build/Source/WebKit/UIProcess/API/Cocoa/_WKElementAction.mm:167:10: error: variable 'handler' is used uninitialized whenever switch case is taken [-Werror,-Wsometimes-uninitialized]
    case _WKElementActionToggleShowLinkPreviews:
         ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
/Volumes/Data/worker/iOS-12-Build-EWS/build/Source/WebKit/UIProcess/API/Cocoa/_WKElementAction.mm:175:92: note: uninitialized use occurs here
    return [[[self alloc] _initWithTitle:(customTitle ? customTitle : title) actionHandler:handler type:type assistant:assistant] autorelease];
                                                                                           ^~~~~~~
/Volumes/Data/worker/iOS-12-Build-EWS/build/Source/WebKit/UIProcess/API/Cocoa/_WKElementAction.mm:125:36: note: did you mean to use __block 'handler'?
    WKElementActionHandlerInternal handler;
                                   ^
                                   __block 
1 error generated.

[Truitt Savell]

Reverted r249950 for reason:

Broke the iOS build.

Committed r249956: <https://trac.webkit.org/changeset/249956>

[Dean Jackson]

Committed r250015: <https://trac.webkit.org/changeset/250015>

[David Kilzer (:ddkilzer)]

(In reply to Dean Jackson from comment #9)
> Committed r250015: <https://trac.webkit.org/changeset/250015>

The build fix was this:

diff --git a/Source/WebKit/UIProcess/API/Cocoa/_WKElementAction.mm b/Source/WebKit/UIProcess/API/Cocoa/_WKElementAction.
mm
index 4b5ff1bec28..31ec821c9b7 100644
--- a/Source/WebKit/UIProcess/API/Cocoa/_WKElementAction.mm
+++ b/Source/WebKit/UIProcess/API/Cocoa/_WKElementAction.mm
@@ -121,8 +121,8 @@ + (instancetype)_elementActionWithType:(_WKElementActionType)type title:(NSStrin
 
 + (instancetype)_elementActionWithType:(_WKElementActionType)type customTitle:(NSString *)customTitle assistant:(WKActionSheetAssistant *)assistant
 {
-    NSString *title;
-    WKElementActionHandlerInternal handler;
+    NSString *title = @"";
+    WKElementActionHandlerInternal handler = nil;
     switch (type) {
     case _WKElementActionTypeCopy:
         title = WEB_UI_STRING_KEY("Copy", "Copy (ActionSheet)", "Title for Copy Link or Image action button");
@@ -164,17 +164,9 @@ + (instancetype)_elementActionWithType:(_WKElementActionType)type customTitle:(N
             [assistant.delegate actionSheetAssistant:assistant shareElementWithURL:actionInfo.URL ?: actionInfo.imageURL rect:actionInfo.boundingRect];
         };
         break;
-    case _WKElementActionToggleShowLinkPreviews: {
-        bool showingLinkPreviews = true;
-        if (NSNumber *value = [[NSUserDefaults standardUserDefaults] objectForKey:webkitShowLinkPreviewsPreferenceKey])
-            showingLinkPreviews = value.boolValue;
-
-        title = showingLinkPreviews ? WEB_UI_STRING("Hide Link Previews", "Title for Hide Link Previews action button") : WEB_UI_STRING("Show Link Previews", "Title for Show Link Previews action button");
-        handler = ^(WKActionSheetAssistant *, _WKActivatedElementInfo *) {
-            [[NSUserDefaults standardUserDefaults] setBool:!showingLinkPreviews forKey:webkitShowLinkPreviewsPreferenceKey];
-        };
+    case _WKElementActionToggleShowLinkPreviews:
+        // This action must still exist for compatibility, but doesn't do anything.
         break;
-    }
     default:
         [NSException raise:NSInvalidArgumentException format:@"There is no standard web element action of type %ld.", (long)type];
         return nil;

Wouldn't it be better to do this instead so `handler` is not nil (which would cause a crash if `handler` was invoked without a nullptr check--as it is in this class), and future case statements that don't set these variables get a warning/build failure?

diff --git a/Source/WebKit/UIProcess/API/Cocoa/_WKElementAction.mm b/Source/WebKit/UIProcess/API/Cocoa/_WKElementAction.mm
index 31ec821c9b7..b4b701408f5 100644
--- a/Source/WebKit/UIProcess/API/Cocoa/_WKElementAction.mm
+++ b/Source/WebKit/UIProcess/API/Cocoa/_WKElementAction.mm
@@ -121,8 +121,8 @@ + (instancetype)_elementActionWithType:(_WKElementActionType)type title:(NSStrin
 
 + (instancetype)_elementActionWithType:(_WKElementActionType)type customTitle:(NSString *)customTitle assistant:(WKActionSheetAssistant *)assistant
 {
-    NSString *title = @"";
-    WKElementActionHandlerInternal handler = nil;
+    NSString *title;
+    WKElementActionHandlerInternal handler;
     switch (type) {
     case _WKElementActionTypeCopy:
         title = WEB_UI_STRING_KEY("Copy", "Copy (ActionSheet)", "Title for Copy Link or Image action button");
@@ -166,6 +166,8 @@ + (instancetype)_elementActionWithType:(_WKElementActionType)type customTitle:(N
         break;
     case _WKElementActionToggleShowLinkPreviews:
         // This action must still exist for compatibility, but doesn't do anything.
+        title = @"";
+        handler = ^(WKActionSheetAssistant *assistant, _WKActivatedElementInfo *actionInfo) { };
         break;
     default:
         [NSException raise:NSInvalidArgumentException format:@"There is no standard web element action of type %ld.", (long)type];