|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>|
|Version:||525.x (Safari 3.1)|
|Bug Depends on:|
Description Hajime Morrita 2011-05-22 23:59:25 PDT
Currently DocumentMarker is treat as a value class. But this has following problems: - It is wasteful. We are doing unnecessary copying. - It loose its entity. A living entity of DocumentMarker object is actually a RenderedDocumentMarker, a subclass of DM. It would have benefits if we could access it. Using a DocumentMarker object as a pointer solves these problems. Concretely, DocumentMarkerController::markersForNode() and markersInRange() should return a vector of pointers.
Comment 2 Tony Chang 2011-05-26 13:37:58 PDT
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?
Comment 3 Hajime Morrita 2011-05-26 23:47:52 PDT
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.