Bug 210396 – Stop using live ranges in functions that return range of the selection

• Status: RESOLVED
• Resolution: FIXED
• Component: HTML Editing

[Darin Adler]

Stop using live ranges in functions that return range of the selection

[Darin Adler]

Created attachment 396199 [details]
Patch

[Darin Adler]

I’m guessing GTK and WPE will require some changes to compile.

[Darin Adler]

Created attachment 396200 [details]
Patch

[Darin Adler]

Created attachment 396201 [details]
Patch

[Darin Adler]

Created attachment 396227 [details]
Patch

[Darin Adler]

Created attachment 397102 [details]
Patch

[Darin Adler]

Created attachment 397103 [details]
Patch

[Darin Adler]

Created attachment 397105 [details]
Patch

[Darin Adler]

Created attachment 397158 [details]
Patch

[Darin Adler]

Created attachment 397164 [details]
Patch

[Darin Adler]

Created attachment 397298 [details]
Patch

[Darin Adler]

Created attachment 397317 [details]
Patch

[Sam Weinig]

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.

[Darin Adler]

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.

[Darin Adler]

I decided to fix the misspelling and do everything else in a separate follow-up patch.

[Darin Adler]

Committed r260725: <https://trac.webkit.org/changeset/260725>

[Radar WebKit Bug Importer]

<rdar://problem/62396029>