Refactor the for-loops using the ranged-for loops and auto keyword in WebCore/editing.
Created attachment 263550 [details] Patch
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.
(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 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()));
Created attachment 264035 [details] Patch
(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 on attachment 264035 [details] Patch Clearing flags on attachment: 264035 Committed r191553: <http://trac.webkit.org/changeset/191553>
All reviewed patches have been landed. Closing bug.