Bug 214109 – Remove live ranges from Document.h, AlternativeTextController.h, DictionaryLookup.h, and WebPage.h

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Darin Adler]

Remove live ranges from Document.h, AlternativeTextController.h, DictionaryLookup.h, and WebPage.h

[Darin Adler]

Created attachment 403825 [details]
Patch

[Darin Adler]

Created attachment 403986 [details]
Patch

[Darin Adler]

OK, looks like this new patch is on a patch to be green on all of EWS.

[Sam Weinig]

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).

[Darin Adler]

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.

[Darin Adler]

Committed r264247: <https://trac.webkit.org/changeset/264247>

[Radar WebKit Bug Importer]

<rdar://problem/65372579>

[Ryan Haddad]

Follow up fix for Catalyst in https://trac.webkit.org/changeset/264317/webkit