Bug 150354

Summary: Use modern for-loops in WebCore/editing.
Product: WebKit Reporter: Hunseop Jeong <hs85.jeong>
Component: WebCore Misc.Assignee: Hunseop Jeong <hs85.jeong>
Status: RESOLVED FIXED    
Severity: Normal CC: commit-queue, darin
Priority: P2    
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Patch
none
Patch none

Description Hunseop Jeong 2015-10-19 21:57:11 PDT
Refactor the for-loops using the ranged-for loops and auto keyword in WebCore/editing.
Comment 1 Hunseop Jeong 2015-10-19 22:07:15 PDT
Created attachment 263550 [details]
Patch
Comment 2 Darin Adler 2015-10-20 09:04:07 PDT
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.
Comment 3 Hunseop Jeong 2015-10-21 23:47:50 PDT
(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?
Comment 4 Darin Adler 2015-10-24 15:25:19 PDT
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()));
Comment 5 Hunseop Jeong 2015-10-25 21:52:53 PDT
Created attachment 264035 [details]
Patch
Comment 6 Hunseop Jeong 2015-10-25 22:07:18 PDT
(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?
Comment 7 WebKit Commit Bot 2015-10-25 23:30:07 PDT
Comment on attachment 264035 [details]
Patch

Clearing flags on attachment: 264035

Committed r191553: <http://trac.webkit.org/changeset/191553>
Comment 8 WebKit Commit Bot 2015-10-25 23:30:11 PDT
All reviewed patches have been landed.  Closing bug.