REGRESSION (r264101): Sharing a link attaches an image instead of the URL

• Component: WebKit2
• Status: RESOLVED FIXED

[Wenson Hsieh]

<rdar://problem/65513607>

[Wenson Hsieh]

Created attachment 404301 [details]
Patch

[Tim Horton]

Might be even better to use some WTF thing, but this is fine as a quick fix

[Wenson Hsieh]

(In reply to Tim Horton from comment #2)
> Might be even better to use some WTF thing, but this is fine as a quick fix

That’s true! I could make this something like

URL(element.imageURL).protocolIs(“data");

[Wenson Hsieh]

(In reply to Tim Horton from comment #2)
> Might be even better to use some WTF thing, but this is fine as a quick fix

That’s true! I could make this something like

URL(element.imageURL).protocolIsData();

[Devin Rousso]

Comment on attachment 404301 [details]
Patch

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

r=me

> Source/WebKit/ChangeLog:10
> +        r264101 added logic that attempts to share an activated (i.e. long-pressed) element as an image, if the image
> +        URL for the element is equal to "data" (ignoring case sensitivity). However, in the case where the image URL is

NIT: it's really comparing the URL's scheme, not the entire URL itself

> Source/WebKit/UIProcess/ios/WKActionSheetAssistant.mm:831
> -        if ([element.imageURL.scheme caseInsensitiveCompare:@"data"] == NSOrderedSame && element.image && [delegate respondsToSelector:@selector(actionSheetAssistant:shareElementWithImage:rect:)])
> +        if ([@"data" caseInsensitiveCompare:element.imageURL.scheme] == NSOrderedSame && element.image && [delegate respondsToSelector:@selector(actionSheetAssistant:shareElementWithImage:rect:)])

Is it worth explicitly checking that `element.imageURL` is truthy so that it's explicit/obvious that this codepath expects it to be non-`nil` (i.e. have `element.image && element.imageURL` at the beginning)?

> Tools/TestWebKitAPI/Tests/ios/ShareSheetTests.mm:114
> +        EXPECT_WK_STREQ("https://www.apple.com/", [[activityItems objectAtIndex:0] absoluteString]);

So how would this fail if the above code changed in the future?  Would `absoluteString` be `nil` (and some other "form" would be set instead) or would it be the dataURL?

[Wenson Hsieh]

Thanks for the reviews!

(In reply to Devin Rousso from comment #5)
> Comment on attachment 404301 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=404301&action=review
> 
> r=me
> 
> > Source/WebKit/ChangeLog:10
> > +        r264101 added logic that attempts to share an activated (i.e. long-pressed) element as an image, if the image
> > +        URL for the element is equal to "data" (ignoring case sensitivity). However, in the case where the image URL is
> 
> NIT: it's really comparing the URL's scheme, not the entire URL itself

Yep, I missed a word here ("scheme").

> 
> > Source/WebKit/UIProcess/ios/WKActionSheetAssistant.mm:831
> > -        if ([element.imageURL.scheme caseInsensitiveCompare:@"data"] == NSOrderedSame && element.image && [delegate respondsToSelector:@selector(actionSheetAssistant:shareElementWithImage:rect:)])
> > +        if ([@"data" caseInsensitiveCompare:element.imageURL.scheme] == NSOrderedSame && element.image && [delegate respondsToSelector:@selector(actionSheetAssistant:shareElementWithImage:rect:)])
> 
> Is it worth explicitly checking that `element.imageURL` is truthy so that
> it's explicit/obvious that this codepath expects it to be non-`nil` (i.e.
> have `element.image && element.imageURL` at the beginning)?

I don’t think there’s much benefit to doing so, but I’m changing this to URL(element.imageURL).protocolIsData() anyways.

> 
> > Tools/TestWebKitAPI/Tests/ios/ShareSheetTests.mm:114
> > +        EXPECT_WK_STREQ("https://www.apple.com/", [[activityItems objectAtIndex:0] absoluteString]);
> 
> So how would this fail if the above code changed in the future?  Would
> `absoluteString` be `nil` (and some other "form" would be set instead) or
> would it be the dataURL?

Depending on the nature of the failure, any of the above!

[Wenson Hsieh]

Created attachment 404308 [details]
Patch for landing

[EWS]

Committed r264383: <https://trac.webkit.org/changeset/264383>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 404308 [details].