Bug 210396 - Stop using live ranges in functions that return range of the selection
Summary: Stop using live ranges in functions that return range of the selection
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: HTML Editing (show other bugs)
Version: WebKit Nightly Build
Hardware: All All
: P2 Normal
Assignee: Darin Adler
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2020-04-11 16:52 PDT by Darin Adler
Modified: 2020-04-26 08:34 PDT (History)
23 users (show)

See Also:

Attachments
Patch (171.62 KB, patch)
2020-04-11 17:39 PDT, Darin Adler 		no flags Details | Formatted Diff | Diff
Patch (174.48 KB, patch)
2020-04-11 17:46 PDT, Darin Adler 		no flags Details | Formatted Diff | Diff
Patch (174.76 KB, patch)
2020-04-11 17:51 PDT, Darin Adler 		no flags Details | Formatted Diff | Diff
Patch (174.80 KB, patch)
2020-04-12 10:23 PDT, Darin Adler 		no flags Details | Formatted Diff | Diff
Patch (175.66 KB, patch)
2020-04-21 12:04 PDT, Darin Adler 		no flags Details | Formatted Diff | Diff
Patch (175.64 KB, patch)
2020-04-21 12:07 PDT, Darin Adler 		no flags Details | Formatted Diff | Diff
Patch (175.62 KB, patch)
2020-04-21 12:23 PDT, Darin Adler 		no flags Details | Formatted Diff | Diff
Patch (180.54 KB, patch)
2020-04-21 18:48 PDT, Darin Adler 		no flags Details | Formatted Diff | Diff
Patch (179.98 KB, patch)
2020-04-21 21:32 PDT, Darin Adler 		no flags Details | Formatted Diff | Diff
Patch (180.19 KB, patch)
2020-04-22 18:16 PDT, Darin Adler 		no flags Details | Formatted Diff | Diff
Patch (180.03 KB, patch)
2020-04-22 22:05 PDT, Darin Adler 		sam: review+
Details | Formatted Diff | Diff
Show Obsolete (10) View All Add an attachment (proposed patch, testcase, etc.)

Note You need to log in before you can comment on or make changes to this bug.
Description Darin Adler 2020-04-11 16:52:08 PDT 
Stop using live ranges in functions that return range of the selection
Comment 1 Darin Adler 2020-04-11 17:39:29 PDT Comment hidden (obsolete)
Comment 2 Darin Adler 2020-04-11 17:40:04 PDT Comment hidden (obsolete)
Comment 3 Darin Adler 2020-04-11 17:46:16 PDT Comment hidden (obsolete)
Comment 4 Darin Adler 2020-04-11 17:51:54 PDT Comment hidden (obsolete)
Comment 5 Darin Adler 2020-04-12 10:23:30 PDT Comment hidden (obsolete)
Comment 6 Darin Adler 2020-04-21 12:04:29 PDT Comment hidden (obsolete)
Comment 7 Darin Adler 2020-04-21 12:07:00 PDT Comment hidden (obsolete)
Comment 8 Darin Adler 2020-04-21 12:23:32 PDT Comment hidden (obsolete)
Comment 9 Darin Adler 2020-04-21 18:48:32 PDT Comment hidden (obsolete)
Comment 10 Darin Adler 2020-04-21 21:32:22 PDT Comment hidden (obsolete)
Comment 11 Darin Adler 2020-04-22 18:16:18 PDT Comment hidden (obsolete)
Comment 12 Darin Adler 2020-04-22 22:05:08 PDT 
Created attachment 397317 [details]
Patch
Comment 13 Sam Weinig 2020-04-25 18:41:55 PDT 
Comment on attachment 397317 [details]
Patch

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

So many good and juicy follow ups here. Very nice.

> Source/WebCore/editing/cocoa/HTMLConverter.mm:422
>      if (documentAttributes)
> -        *documentAttributes = [[_documentAttrs retain] autorelease];
> +        *documentAttributes = _documentAttrs;

Makes me wonder if this family of functions should always return a std::pair<RetainPtr<NSAttributedString>, RetainPtr<NSDictionary>>, rather than making this optional. Will trade off a branch for a retain. Doesn't really matter, but the out parameter has always bugged me and now that we can use structured bindings, the pair is not a mess anymore.
Comment 14 Darin Adler 2020-04-26 08:03:22 PDT 
Comment on attachment 397317 [details]
Patch

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

> Source/WebCore/ChangeLog:12
> +          against tree changes while iteratoring; it will now always stop at the

misspelled iterating

> Source/WebCore/editing/cocoa/EditorCocoa.mm:111
> +    return range ? attributedString(*range) : adoptNS([[NSAttributedString alloc] initWithString:@""]);

I think init instead of initWithString: here.

>> Source/WebCore/editing/cocoa/HTMLConverter.mm:422
>> +        *documentAttributes = _documentAttrs;
> 
> Makes me wonder if this family of functions should always return a std::pair<RetainPtr<NSAttributedString>, RetainPtr<NSDictionary>>, rather than making this optional. Will trade off a branch for a retain. Doesn't really matter, but the out parameter has always bugged me and now that we can use structured bindings, the pair is not a mess anymore.

And I suppose there is no savings here when the caller doesn’t ask for the attributes. They are simply computed and dumped on the floor. I think I’d use a named struct rather than a tuple  or pair though. While the string is self explanatory for the type NSAttributedString I don’t think that NSDictionary for “document attributes” is clear without a name.

> Source/WebKitLegacy/mac/WebView/WebHTMLRepresentation.mm:272
> +        return [[[NSAttributedString alloc] initWithString:@""] autorelease];

I think init instead of initWithString: here.

> Source/WebKitLegacy/mac/WebView/WebHTMLView.mm:7017
> +        return [[[NSAttributedString alloc] initWithString:@""] autorelease];

I think init instead of initWithString: here.

> Source/WebKitLegacy/mac/WebView/WebHTMLView.mm:7026
> +        return [[[NSAttributedString alloc] initWithString:@""] autorelease];

I think init instead of initWithString: here.
Comment 15 Darin Adler 2020-04-26 08:31:40 PDT 
I decided to fix the misspelling and do everything else in a separate follow-up patch.
Comment 16 Darin Adler 2020-04-26 08:33:11 PDT 
Committed r260725: <https://trac.webkit.org/changeset/260725>
Comment 17 Radar WebKit Bug Importer 2020-04-26 08:34:15 PDT 
<rdar://problem/62396029>