Bug 214109

Summary: Remove live ranges from Document.h, AlternativeTextController.h, DictionaryLookup.h, and WebPage.h
Product: WebKit Reporter: Darin Adler <darin>
Component: New BugsAssignee: 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 Flags
Patch
none
Patch sam: review+

Description Darin Adler 2020-07-08 16:51:38 PDT
Remove live ranges from Document.h, AlternativeTextController.h, DictionaryLookup.h, and WebPage.h
Comment 1 Darin Adler 2020-07-08 17:06:34 PDT
Created attachment 403825 [details]
Patch
Comment 2 Darin Adler 2020-07-10 12:16:37 PDT
Created attachment 403986 [details]
Patch
Comment 3 Darin Adler 2020-07-10 12:58:00 PDT
OK, looks like this new patch is on a patch to be green on all of EWS.
Comment 4 Sam Weinig 2020-07-10 16:43:07 PDT
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 5 Darin Adler 2020-07-10 16:56:52 PDT
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.
Comment 6 Darin Adler 2020-07-10 17:00:03 PDT
Committed r264247: <https://trac.webkit.org/changeset/264247>
Comment 7 Radar WebKit Bug Importer 2020-07-10 17:01:16 PDT
<rdar://problem/65372579>
Comment 8 Ryan Haddad 2020-07-13 14:13:00 PDT
Follow up fix for Catalyst in https://trac.webkit.org/changeset/264317/webkit