Summary: | Remove live ranges from Document.h, AlternativeTextController.h, DictionaryLookup.h, and WebPage.h | ||||||||
---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Darin Adler <darin> | ||||||
Component: | New Bugs | Assignee: | Darin Adler <darin> | ||||||
Status: | RESOLVED FIXED | ||||||||
Severity: | Normal | CC: | aboxhall, apinheiro, cdumez, cfleizach, dmazzoni, eric.carlson, esprehn+autocc, ews-watchlist, glenn, jcraig, jdiggs, jer.noble, kangil.han, mifenton, philipj, samuel_white, sam, sergio, webkit-bug-importer | ||||||
Priority: | P2 | Keywords: | InRadar | ||||||
Version: | WebKit Nightly Build | ||||||||
Hardware: | Unspecified | ||||||||
OS: | Unspecified | ||||||||
Attachments: |
|
Description
Darin Adler
2020-07-08 16:51:38 PDT
Created attachment 403825 [details]
Patch
Created attachment 403986 [details]
Patch
OK, looks like this new patch is on a patch to be green on all of EWS. Comment on attachment 403986 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=403986&action=review > Source/WebCore/dom/Document.h:1587 > enum ConstructionFlags { Synthesized = 1, NonRenderedPlaceholder = 1 << 1 }; > - Document(Frame*, const URL&, unsigned = DefaultDocumentClass, unsigned constructionFlags = 0); > + Document(Frame*, const URL&, DocumentClassFlags = DefaultDocumentClass, unsigned constructionFlags = 0); Probably want to convert both DocumentClassFlags and ConstructionFlags to use OptionSet at some point. > Source/WebCore/editing/AlternativeTextController.cpp:593 > // Clone the range, since the caller of may keep a refernece to the original range and modify it. > - SpellingCorrectionCommand::create(range.cloneRange(), alternative)->apply(); > + SpellingCorrectionCommand::create(createLiveRange(range), alternative)->apply(); Comment is not quite right anymore. > Source/WebCore/editing/mac/DictionaryLookup.h:60 > - WEBCORE_EXPORT static std::tuple<RefPtr<Range>, NSDictionary *> rangeForSelection(const VisibleSelection&); > - WEBCORE_EXPORT static std::tuple<RefPtr<Range>, NSDictionary *> rangeAtHitTestResult(const HitTestResult&); > + WEBCORE_EXPORT static Optional<std::tuple<SimpleRange, NSDictionary *>> rangeForSelection(const VisibleSelection&); > + WEBCORE_EXPORT static Optional<std::tuple<SimpleRange, NSDictionary *>> rangeAtHitTestResult(const HitTestResult&); > WEBCORE_EXPORT static std::tuple<NSString *, NSDictionary *> stringForPDFSelection(PDFSelection *); I think these would be easier to understand if they returned used a struct for the return value (or Optional<NewStruct> as the case may be) rather than tuple. Not new, but this is the type of std::pair/std::tuple usage I think we should avoid, where you can't tell what the values are for straight from the function name. (/me is scared to blame the file and find out I added the std::tuple use). Comment on attachment 403986 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=403986&action=review >> Source/WebCore/dom/Document.h:1587 >> + Document(Frame*, const URL&, DocumentClassFlags = DefaultDocumentClass, unsigned constructionFlags = 0); > > Probably want to convert both DocumentClassFlags and ConstructionFlags to use OptionSet at some point. Yes! >> Source/WebCore/editing/AlternativeTextController.cpp:593 >> + SpellingCorrectionCommand::create(createLiveRange(range), alternative)->apply(); > > Comment is not quite right anymore. Will fix. >> Source/WebCore/editing/mac/DictionaryLookup.h:60 >> WEBCORE_EXPORT static std::tuple<NSString *, NSDictionary *> stringForPDFSelection(PDFSelection *); > > I think these would be easier to understand if they returned used a struct for the return value (or Optional<NewStruct> as the case may be) rather than tuple. Not new, but this is the type of std::pair/std::tuple usage I think we should avoid, where you can't tell what the values are for straight from the function name. (/me is scared to blame the file and find out I added the std::tuple use). OK, I’ll consider doing that to improve this in the future. I agree that a struct would be nicer. Don’t want to do it right now. Committed r264247: <https://trac.webkit.org/changeset/264247> Follow up fix for Catalyst in https://trac.webkit.org/changeset/264317/webkit |