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
Hunseop Jeong
2015-10-19 21:57:11 PDT
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. |