Bug 173997 - Add TextStream operators for Range, VisiblePosition, VisibleSelection, and ScrollAlignment
Summary: Add TextStream operators for Range, VisiblePosition, VisibleSelection, and Sc...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Megan Gardner
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2017-06-29 17:15 PDT by Megan Gardner
Modified: 2017-06-29 17:56 PDT (History)
4 users (show)

See Also:


Attachments
Patch (6.50 KB, patch)
2017-06-29 17:18 PDT, Megan Gardner
no flags Details | Formatted Diff | Diff
Patch (6.56 KB, patch)
2017-06-29 17:29 PDT, Megan Gardner
no flags Details | Formatted Diff | Diff
Patch (6.56 KB, patch)
2017-06-29 17:38 PDT, Megan Gardner
simon.fraser: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Megan Gardner 2017-06-29 17:15:31 PDT
Some WebCore Logging
Comment 1 Megan Gardner 2017-06-29 17:18:44 PDT
Created attachment 314193 [details]
Patch
Comment 2 Wenson Hsieh 2017-06-29 17:26:06 PDT
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.
Comment 3 Tim Horton 2017-06-29 17:26:30 PDT
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).
Comment 4 Megan Gardner 2017-06-29 17:29:33 PDT
Created attachment 314195 [details]
Patch
Comment 5 Simon Fraser (smfr) 2017-06-29 17:31:37 PDT
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.
Comment 6 Megan Gardner 2017-06-29 17:38:58 PDT
Created attachment 314196 [details]
Patch
Comment 7 Simon Fraser (smfr) 2017-06-29 17:41:51 PDT
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.
Comment 8 Megan Gardner 2017-06-29 17:56:19 PDT
https://trac.webkit.org/r218976