Bug 61262

Summary: [Refactoring] DocumentMarker should be carried around as a pointer, not value.
Product: WebKit Reporter: Hajime Morrita <morrita>
Component: HTML EditingAssignee: 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 Flags
Patch tony: review+

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 1 Hajime Morrita 2011-05-23 02:06:55 PDT
Created attachment 94385 [details]
Patch
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.
Comment 4 Hajime Morrita 2011-05-27 00:56:52 PDT
Committed r87483: <http://trac.webkit.org/changeset/87483>