Bug 211197 – REGRESSION (r246395): Leak of ARQuickLookPreviewItem and ARQuickLookWebKitItem in -[_WKPreviewControllerDataSource previewController:previewItemAtIndex:]

• Component: WebKit2
• Status: RESOLVED FIXED

[David Kilzer (:ddkilzer)]

Leak of ARQuickLookPreviewItem and ARQuickLookWebKitItem in -[_WKPreviewControllerDataSource previewController:previewItemAtIndex:].

Regressed in r246395 for Bug 198812.

[Radar WebKit Bug Importer]

<rdar://problem/62612483>

[David Kilzer (:ddkilzer)]

Created attachment 397985 [details]
Patch v1

[David Kilzer (:ddkilzer)]

Created attachment 397988 [details]
Patch v2

[David Kilzer (:ddkilzer)]

(In reply to David Kilzer (:ddkilzer) from comment #3)
> Created attachment 397988 [details]
> Patch v2

Fixed build error:

-    previewItem.canonicalWebPageURL = _originatingPageURL;
+    previewItem.get().canonicalWebPageURL = _originatingPageURL;

[Darin Adler]

Comment on attachment 397988 [details]
Patch v2

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

> Source/WebKit/UIProcess/Cocoa/SystemPreviewControllerCocoa.mm:113
> +    previewItem.get().canonicalWebPageURL = _originatingPageURL;

Some WebKit contributors prefer this style to sidestep the get():

    [previewItem setCanonicalWebPageURL:_originatingPageURL];

[David Kilzer (:ddkilzer)]

Comment on attachment 397988 [details]
Patch v2

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

>> Source/WebKit/UIProcess/Cocoa/SystemPreviewControllerCocoa.mm:113
>> +    previewItem.get().canonicalWebPageURL = _originatingPageURL;
> 
> Some WebKit contributors prefer this style to sidestep the get():
> 
>     [previewItem setCanonicalWebPageURL:_originatingPageURL];

Wasn't sure if new-style Objective-C setters (with .get()) were preferred over old-style setter syntax (but no .get()).

[Darin Adler]

Comment on attachment 397988 [details]
Patch v2

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

>>> Source/WebKit/UIProcess/Cocoa/SystemPreviewControllerCocoa.mm:113
>>> +    previewItem.get().canonicalWebPageURL = _originatingPageURL;
>> 
>> Some WebKit contributors prefer this style to sidestep the get():
>> 
>>     [previewItem setCanonicalWebPageURL:_originatingPageURL];
> 
> Wasn't sure if new-style Objective-C setters (with .get()) were preferred over old-style setter syntax (but no .get()).

Tim Horton was the one who explained the lack of Objective-C setters in another patch, by citing this.

[David Kilzer (:ddkilzer)]

Created attachment 397999 [details]
Patch for landing

[David Kilzer (:ddkilzer)]

Comment on attachment 397999 [details]
Patch for landing

Marking cq+ because "Patch v2" has passed all tests except one ios-wk2 test (fast/text/text-indent-inside-float.html) which this patch is not likely to have affected.

[EWS]

Committed r260922: <https://trac.webkit.org/changeset/260922>

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