Summary: | [Refactoring] DocumentMarker should be carried around as a pointer, not value. | ||||||
---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Hajime Morrita <morrita> | ||||
Component: | HTML Editing | Assignee: | Hajime Morrita <morrita> | ||||
Status: | RESOLVED FIXED | ||||||
Severity: | Normal | CC: | jiapu.mail | ||||
Priority: | P2 | ||||||
Version: | 525.x (Safari 3.1) | ||||||
Hardware: | Unspecified | ||||||
OS: | Unspecified | ||||||
Bug Depends on: | |||||||
Bug Blocks: | 61261 | ||||||
Attachments: |
|
Description
Hajime Morrita
2011-05-22 23:59:25 PDT
Created attachment 94385 [details]
Patch
Comment on attachment 94385 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=94385&action=review Is it possible for us to put OwnPtr in the vector to make the ownership > Source/WebCore/dom/DocumentMarkerController.cpp:317 > +Vector<DocumentMarker*> DocumentMarkerController::markersFor(Node* node) Would this work as a Vector<const DocumentMarker*>? > Source/WebCore/rendering/InlineTextBox.cpp:1092 > + Vector<DocumentMarker*>::iterator markerIt = markers.begin(); Nit: Could this be a const_iterator? Hi Tony, thank you for giving a review! (In reply to comment #2) > (From update of attachment 94385 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=94385&action=review > > Is it possible for us to put OwnPtr in the vector to make the ownership For the return value, no it isn't. The markers are owned by DocumentMarkerController. For DocumentMarkerController::m_markers, I'm not sure. I'll try it in another patch. > > > Source/WebCore/dom/DocumentMarkerController.cpp:317 > > +Vector<DocumentMarker*> DocumentMarkerController::markersFor(Node* node) No, because we want to call some setter of returned markers. (That is a motivation of this change.) > > Would this work as a Vector<const DocumentMarker*>? > > > Source/WebCore/rendering/InlineTextBox.cpp:1092 > > + Vector<DocumentMarker*>::iterator markerIt = markers.begin(); > > Nit: Could this be a const_iterator? Sure. I'll fix this before landing. Committed r87483: <http://trac.webkit.org/changeset/87483> |