[webkit-reviews] review granted: [Bug 214109] Remove live ranges from Document.h, AlternativeTextController.h, DictionaryLookup.h, and WebPage.h : [Attachment 403986] Patch

[bugzilla-daemon at webkit.org]

Sam Weinig <sam at webkit.org> has granted Darin Adler <darin at apple.com>'s request
for review:
Bug 214109: Remove live ranges from Document.h, AlternativeTextController.h,
DictionaryLookup.h, and WebPage.h
https://bugs.webkit.org/show_bug.cgi?id=214109

Attachment 403986: Patch

https://bugs.webkit.org/attachment.cgi?id=403986&action=review




--- Comment #4 from Sam Weinig <sam at webkit.org> ---
Comment on attachment 403986
  --> https://bugs.webkit.org/attachment.cgi?id=403986
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).