Bug 137858 – Kill toRenderedDocumentMarker() by using tighter typing

• Component: Layout and Rendering
• Status: RESOLVED FIXED

[Chris Dumez]

Kill toRenderedDocumentMarker() by using tighter typing. This method was doing no type validation, just a static_cast<>().

[Chris Dumez]

Created attachment 240074 [details]
Patch

[Chris Dumez]

Created attachment 240075 [details]
Patch

[Chris Dumez]

Created attachment 240079 [details]
Patch

[Darin Adler]

Comment on attachment 240079 [details]
Patch

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

> Source/WebCore/dom/DocumentMarkerController.cpp:409
> +        Vector<RenderedDocumentMarker*> markers = markersFor(node);
> +        for (auto* marker : markers) {

Believe it or not, we don’t even need the local variable here. This will work:

    for (auto* marker : markersFor(node)) {

However, some other similar constructions wouldn’t work. Only the result of the entire expression on the right gets its lifetime extended in the right way. So maybe you don’t want to do this?

> Source/WebCore/dom/DocumentMarkerController.cpp:681
> +        Vector<RenderedDocumentMarker*> markers = markersFor(node);
> +        for (auto* marker : markers) {

Same thing again.

> Source/WebCore/editing/AlternativeTextController.cpp:474
> +    Vector<RenderedDocumentMarker*> markers = node->document().markers().markersFor(node);
> +    for (const auto* marker : markers) {

Same thing again. Also not sure why we bother putting the const here; doesn’t add much.

> Source/WebCore/editing/AlternativeTextController.cpp:554
> +    Vector<RenderedDocumentMarker*> correctedOnceMarkers = markers.markersInRange(rangeOfCorrection, DocumentMarker::Autocorrected);

A line like this is where other people hate, and I love, auto.

> Source/WebCore/editing/AlternativeTextController.cpp:735
>      for (size_t i = 0; i < dictationAlternativesMarkers.size(); ++i)

Could use a modern for loop here.

> Source/WebCore/rendering/InlineTextBox.h:161
> -    void paintDocumentMarker(GraphicsContext*, const FloatPoint& boxOrigin, DocumentMarker*, const RenderStyle&, const Font&, bool grammar);
> -    void paintTextMatchMarker(GraphicsContext*, const FloatPoint& boxOrigin, DocumentMarker*, const RenderStyle&, const Font&);
> +    void paintDocumentMarker(GraphicsContext*, const FloatPoint& boxOrigin, RenderedDocumentMarker*, const RenderStyle&, const Font&, bool grammar);
> +    void paintTextMatchMarker(GraphicsContext*, const FloatPoint& boxOrigin, RenderedDocumentMarker*, const RenderStyle&, const Font&);
>  
> -    void computeRectForReplacementMarker(DocumentMarker*, const RenderStyle&, const Font&);
> +    void computeRectForReplacementMarker(RenderedDocumentMarker*, const RenderStyle&, const Font&);

Seems like these should all take references, not pointers.

[Chris Dumez]

Created attachment 240096 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 240096 [details]
Patch

Clearing flags on attachment: 240096

Committed r174876: <http://trac.webkit.org/changeset/174876>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Ryosuke Niwa]

Nice!