Bug 61262 – [Refactoring] DocumentMarker should be carried around as a pointer, not value.

• Status: RESOLVED
• Resolution: FIXED
• Component: HTML Editing

[Hajime Morrita]

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.

[Hajime Morrita]

Created attachment 94385 [details]
Patch

[Tony Chang]

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?

[Hajime Morrita]

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.

[Hajime Morrita]

Committed r87483: <http://trac.webkit.org/changeset/87483>