Bug 173997 – Add TextStream operators for Range, VisiblePosition, VisibleSelection, and ScrollAlignment

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

[Megan Gardner]

Some WebCore Logging

[Megan Gardner]

Created attachment 314193 [details]
Patch

[Wenson Hsieh]

Comment on attachment 314193 [details]
Patch

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

> Source/WebCore/ChangeLog:3
> +        Some WebCore Logging

Could we make this bug title a bit more descriptive, perhaps so it references VisibleSelection, RangeBoundaryPoint and ScrollAlignment?

> Source/WebCore/ChangeLog:8
> +        Just preserving logging I wrote for a bug that got punted, no tests needed.

...it might be helpful to reference the radar # here too.

[Tim Horton]

Comment on attachment 314193 [details]
Patch

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

> Source/WebCore/ChangeLog:3
> +        Some WebCore Logging

The title of the bug and the title in the changelogs needs to be more specific (perhaps "Add TextStream operators for Range, VisiblePosition, VisibleSelection, and ScrollAlignment", or maybe "Add TextStream operators for some editing classes" since it actually covers more than just those).

[Megan Gardner]

Created attachment 314195 [details]
Patch

[Simon Fraser (smfr)]

Comment on attachment 314195 [details]
Patch

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

> Source/WebCore/ChangeLog:8
> +        Just preserving logging I wrote for <rdar://problem/19005092>

This isn't interesting to future readers of this Changelog. Maybe just say "so they can be used with TextStream-based LOG_WITH_STREAM.

> Source/WebCore/rendering/ScrollAlignment.cpp:62
> +TextStream& operator<<(TextStream& ts, const ScrollAlignment::Behavior& b)

No need to pass ScrollAlignment::Behavior as a reference. It's just an enum.

> Source/WebCore/rendering/ScrollAlignment.cpp:84
> +    return ts << "ScrollAlignment: visible: " << s.m_rectVisible << " hidden: " << s.m_rectHidden << " partial: " <<s.m_rectPartial;

Missing space near the end.

[Megan Gardner]

Created attachment 314196 [details]
Patch

[Simon Fraser (smfr)]

Comment on attachment 314196 [details]
Patch

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

> Source/WebCore/ChangeLog:8
> +        Just preserving logging so they can be used with TextStream-based LOG_WITH_STREAM.

"preserving logging" for you, but that doesn't really make sense to future readers of your changelog.

> Source/WebCore/rendering/ScrollAlignment.cpp:62
> +TextStream& operator<<(TextStream& ts, const ScrollAlignment::Behavior b)

No need for const.

[Megan Gardner]

https://trac.webkit.org/r218976