Bug 155395 - [iOS] WKPreviewAction conforms to NSCopying but doesn’t override -copyWithZone:
Summary: [iOS] WKPreviewAction conforms to NSCopying but doesn’t override -copyWithZone:
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: WebKit Local Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Nobody
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2016-03-12 12:04 PST by mitz
Modified: 2016-03-14 15:58 PDT (History)
6 users (show)

See Also:


Attachments
Patch (1.28 KB, patch)
2016-03-14 13:31 PDT, Beth Dakin
mitz: review-
Details | Formatted Diff | Diff
Patch (1.25 KB, patch)
2016-03-14 13:38 PDT, Beth Dakin
no flags Details | Formatted Diff | Diff
Patch (1.26 KB, patch)
2016-03-14 13:49 PDT, Beth Dakin
no flags Details | Formatted Diff | Diff
Patch (1.25 KB, patch)
2016-03-14 14:01 PDT, Beth Dakin
sam: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description mitz 2016-03-12 12:04:50 PST
_WKPreviewAction (which should be renamed WKPreviewAction) is a subclass of UIPreviewAction, and therefore conforms to NSCopying. It needs to override -copyWithZone: so that when it’s copied, the the copy has the same identifier.
Comment 1 Radar WebKit Bug Importer 2016-03-14 13:30:15 PDT
<rdar://problem/25150528>
Comment 2 Beth Dakin 2016-03-14 13:31:43 PDT
Created attachment 274011 [details]
Patch
Comment 3 mitz 2016-03-14 13:33:58 PDT
Comment on attachment 274011 [details]
Patch

This makes a copy that doesn’t have the original’s title, style and handler!
Comment 4 Beth Dakin 2016-03-14 13:38:16 PDT
Created attachment 274016 [details]
Patch
Comment 5 mitz 2016-03-14 13:40:31 PDT
Comment on attachment 274016 [details]
Patch

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

> Source/WebKit2/UIProcess/API/Cocoa/WKPreviewActionItem.mm:43
> +    WKPreviewAction *action = [self copyWithZone:zone];

This looks infinitely recursive.
Comment 6 Beth Dakin 2016-03-14 13:42:40 PDT
(In reply to comment #5)
> Comment on attachment 274016 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=274016&action=review
> 
> > Source/WebKit2/UIProcess/API/Cocoa/WKPreviewActionItem.mm:43
> > +    WKPreviewAction *action = [self copyWithZone:zone];
> 
> This looks infinitely recursive.

Eep! It does.
Comment 7 Beth Dakin 2016-03-14 13:49:31 PDT
Created attachment 274021 [details]
Patch
Comment 8 Beth Dakin 2016-03-14 14:01:15 PDT
Created attachment 274024 [details]
Patch
Comment 9 Sam Weinig 2016-03-14 15:51:57 PDT
Comment on attachment 274024 [details]
Patch

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

You should remove the explicit conformance to NSCopying of WKPreviewAction since UIPreviewAction already conforms to it.

> Source/WebKit2/UIProcess/API/Cocoa/WKPreviewActionItem.mm:45
> +    WKPreviewAction *action = [super copyWithZone:zone];
> +    action->_identifier = self.identifier;
> +    return action;

Since WKPreviewAction is immutable, this can be implemented as:

    return [self retain];
Comment 10 Sam Weinig 2016-03-14 15:55:24 PDT
(In reply to comment #9)
> Comment on attachment 274024 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=274024&action=review
> 
> You should remove the explicit conformance to NSCopying of WKPreviewAction
> since UIPreviewAction already conforms to it.
> 
> > Source/WebKit2/UIProcess/API/Cocoa/WKPreviewActionItem.mm:45
> > +    WKPreviewAction *action = [super copyWithZone:zone];
> > +    action->_identifier = self.identifier;
> > +    return action;
> 
> Since WKPreviewAction is immutable, this can be implemented as:
> 
>     return [self retain];

Actually, it looks like it isn't completely immutable, so you should do your thing.
Comment 11 Beth Dakin 2016-03-14 15:58:51 PDT
Thanks Sam and Dan! http://trac.webkit.org/changeset/198166