[webkit-reviews] review granted: [Bug 209985] Stop using live ranges in DocumentMarkerController : [Attachment 395538] Patch

[bugzilla-daemon at webkit.org]

Antti Koivisto <koivisto at iki.fi> has granted Darin Adler <darin at apple.com>'s
request for review:
Bug 209985: Stop using live ranges in DocumentMarkerController
https://bugs.webkit.org/show_bug.cgi?id=209985

Attachment 395538: Patch

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




--- Comment #10 from Antti Koivisto <koivisto at iki.fi> ---
Comment on attachment 395538
  --> https://bugs.webkit.org/attachment.cgi?id=395538
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=395538&action=review

Wish the patch was more focused on what it says in the title, with unrelated
refactoring done separately.

> Source/WebCore/dom/DocumentMarker.h:105
> +	   , DictationData // DictationAlternatives
> +#if PLATFORM(IOS_FAMILY)
> +	   , Vector<String> // DictationPhraseWithAlternatives
> +	   , RetainPtr<id> // DictationResult
> +#endif
> +	   , RefPtr<Node> // DraggedContent

I'm not sure removing the types and then having to comment here what the
variants are is an improvement. Call sites end up confusing too.

> Source/WebCore/dom/DocumentMarker.h:120
> -    void clearData() { m_data = false; }
> +    void clearData() { m_data = String { }; }

You might consider using WTF::Monostate for the empty Variant case. It can be
cleaner to use a state that is separate from any actual values.

> Source/WebCore/dom/DocumentMarkerController.cpp:629
> +	   if (auto renderer = node.renderer())
> +	       renderer->repaint();

I think we still usually mark plain pointers with auto*

> Source/WebCore/dom/Node.h:206
> -    virtual bool isCharacterDataNode() const { return false; }
> +    bool isCharacterDataNode() const { return !isContainerNode() &&
(isTextNode() || virtualIsCharacterData()); }

Nice ninja optimization.

> Source/WebCore/dom/SimpleRange.h:59
> +IntersectingNodeRange intersectingNodes(const SimpleRange&);

This is nice.

> Source/WebCore/rendering/RenderObject.h:156
> -    // Convenience function for getting to the nearest enclosing box of a
RenderObject.
> +    // Nearest enclosing box.
>      WEBCORE_EXPORT RenderBox& enclosingBox() const;

Event the compressed comment doesn't really add anything (and if it did, the
right action would be to rename the function).

> Source/WebCore/rendering/RenderObject.h:172
> +    // Forbid calls to setNeedsLayout() during its lifetime.
> +    class SetLayoutNeededForbiddenScope;

"Forbid" is vague. Can the type be renamed to say "assert"? At least the
comment should mention this just enables an assert.