Bug 150354 – Use modern for-loops in WebCore/editing.

• Component: WebCore Misc.
• Status: RESOLVED FIXED

[Hunseop Jeong]

Refactor the for-loops using the ranged-for loops and auto keyword in WebCore/editing.

[Hunseop Jeong]

Created attachment 263550 [details]
Patch

[Darin Adler]

Comment on attachment 263550 [details]
Patch

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

Patch is OK as is. I spotted some things we might want to come back and improve separately.

> Source/WebCore/editing/CompositeEditCommand.cpp:578
>  static void copyMarkers(const Vector<RenderedDocumentMarker*>& markerPointers, Vector<RenderedDocumentMarker>& markers)

This should have a return value, not an out argument.

> Source/WebCore/editing/CompositeEditCommand.cpp:580
> +    markers.reserveCapacity(markerPointers.size());

This should be using reserveInitialCapacity.

> Source/WebCore/editing/CompositeEditCommand.cpp:582
> +        markers.append(*markerPointer);

This should be using uncheckedAppend.

> Source/WebCore/editing/EditingStyle.cpp:103
> +    for (auto editingProperty : editingProperties) {

I’d prefer either "int" or "auto&" here, even though auto will work fine.

> Source/WebCore/editing/mac/EditorMac.mm:517
> +    if (!paths.size())

paths.isEmpty() would be better, I think.

[Hunseop Jeong]

(In reply to comment #2)
> Comment on attachment 263550 [details]
> Patch
> 

Thanks for the review.

I have a question about your comment.

> > Source/WebCore/editing/CompositeEditCommand.cpp:578
> >  static void copyMarkers(const Vector<RenderedDocumentMarker*>& markerPointers, Vector<RenderedDocumentMarker>& markers)
> 
> This should have a return value, not an out argument.
> 

I don't understand here. Do I changed the function to have a return value?
Could you give me the detail explanation?

[Darin Adler]

Comment on attachment 263550 [details]
Patch

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

>>> Source/WebCore/editing/CompositeEditCommand.cpp:578
>>>  static void copyMarkers(const Vector<RenderedDocumentMarker*>& markerPointers, Vector<RenderedDocumentMarker>& markers)
>> 
>> This should have a return value, not an out argument.
> 
> I don't understand here. Do I changed the function to have a return value?
> Could you give me the detail explanation?

Yes, the new function should be:

    static Vector<RenderedDocumentMarker> copyMarkers(const Vector<RenderedDocumentMarker*>& markerPointers)
    {
        ...
        return markers;
    }

And the call site below should be changed to:

    auto markers = copyMarkers(markerController.markersInRange(Range::create(document(), node, offset, node, offset + count).ptr(), DocumentMarker::AllMarkers()));

[Hunseop Jeong]

Created attachment 264035 [details]
Patch

[Hunseop Jeong]

(In reply to comment #4)
> Comment on attachment 263550 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=263550&action=review
> 
> >>> Source/WebCore/editing/CompositeEditCommand.cpp:578
> >>>  static void copyMarkers(const Vector<RenderedDocumentMarker*>& markerPointers, Vector<RenderedDocumentMarker>& markers)
> >> 
> >> This should have a return value, not an out argument.
> > 
> > I don't understand here. Do I changed the function to have a return value?
> > Could you give me the detail explanation?
> 
> Yes, the new function should be:
> 
>     static Vector<RenderedDocumentMarker> copyMarkers(const
> Vector<RenderedDocumentMarker*>& markerPointers)
>     {
>         ...
>         return markers;
>     }
> 
> And the call site below should be changed to:
> 
>     auto markers =
> copyMarkers(markerController.markersInRange(Range::create(document(), node,
> offset, node, offset + count).ptr(), DocumentMarker::AllMarkers()));

Thanks for the guide.
I applied your comments.
Could you check the patch again?

[WebKit Commit Bot]

Comment on attachment 264035 [details]
Patch

Clearing flags on attachment: 264035

Committed r191553: <http://trac.webkit.org/changeset/191553>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.