Summary: | Stop using live ranges in functions that return range of the selection | ||||||||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Darin Adler <darin> | ||||||||||||||||||||||||
Component: | HTML Editing | Assignee: | Darin Adler <darin> | ||||||||||||||||||||||||
Status: | RESOLVED FIXED | ||||||||||||||||||||||||||
Severity: | Normal | CC: | aboxhall, apinheiro, cdumez, cfleizach, dmazzoni, eric.carlson, esprehn+autocc, ews-watchlist, glenn, gyuyoung.kim, jcraig, jdiggs, jer.noble, kangil.han, kondapallykalyan, mifenton, pdr, philipj, samuel_white, sam, sergio, webkit-bug-importer, wenson_hsieh | ||||||||||||||||||||||||
Priority: | P2 | Keywords: | InRadar | ||||||||||||||||||||||||
Version: | WebKit Nightly Build | ||||||||||||||||||||||||||
Hardware: | All | ||||||||||||||||||||||||||
OS: | All | ||||||||||||||||||||||||||
Attachments: |
|
Description
Darin Adler
2020-04-11 16:52:08 PDT
Created attachment 396199 [details]
Patch
I’m guessing GTK and WPE will require some changes to compile. Created attachment 396200 [details]
Patch
Created attachment 396201 [details]
Patch
Created attachment 396227 [details]
Patch
Created attachment 397102 [details]
Patch
Created attachment 397103 [details]
Patch
Created attachment 397105 [details]
Patch
Created attachment 397158 [details]
Patch
Created attachment 397164 [details]
Patch
Created attachment 397298 [details]
Patch
Created attachment 397317 [details]
Patch
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 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. I decided to fix the misspelling and do everything else in a separate follow-up patch. Committed r260725: <https://trac.webkit.org/changeset/260725> |