Bug 44383 – Encapsulate document marker management into DocumentMarkerController

• Status: RESOLVED
• Resolution: FIXED
• Component: WebCore Misc.

[Daniel Bates]

Following up on Eric Seidel's remarks on cleaning up Document.cpp in the webkit-dev email "Re: [webkit-dev] Cleaning up Document.* (was DeviceOrientation/Motion on Document rather than Page)" on 08/17/2010, we should move the marker management code in Document.cpp to its own class, say DocumentMarkerController so as to encapsulate this functionality off of the Document.

Notice, the marker management code only uses the a Document instance to store the marker map, Document::m_markers.

[Daniel Bates]

Created attachment 65035 [details]
Patch

I named the new class DocumentMarkerController and the accessor markerController() (in Document.cpp). I am open to name suggestions.

[Eric Seidel (no email)]

Comment on attachment 65035 [details]
Patch

Yay!  Thank you Dan!

WebCore/dom/Document.h:1173
 +      mutable DocumentMarkerController m_markerController;
It's better for build times if this is an OwnPtr instead.  When its a mutable member you have to include the header in Document.h.   The only reason (I know of) to make these mutable members is to avoid the pointer indirection in hot code paths.  But I don't think DocumentMarkers are hot.  This is not a deal breaker either way.

WebCore/dom/DocumentMarkerController.h:48
 +      void copyMarkers(Node *srcNode, unsigned startOffset, int length, Node *dstNode, int delta, DocumentMarker::MarkerType = DocumentMarker::AllMarkers);
Style.

WebCore/editing/Editor.cpp:1566
 +          frame()->document()->markerController()->removeMarkers(selectedRange.get(), DocumentMarker::Spelling);
I might have called the markerController() just markers().

LGTM.

[Daniel Bates]

(In reply to comment #2)
> (From update of attachment 65035 [details])
> Yay!  Thank you Dan!
> 
> WebCore/dom/Document.h:1173
>  +      mutable DocumentMarkerController m_markerController;
> It's better for build times if this is an OwnPtr instead.  When its a mutable member you have to include the header in Document.h.   The only reason (I know of) to make these mutable members is to avoid the pointer indirection in hot code paths.  But I don't think DocumentMarkers are hot.  This is not a deal breaker either way.
>

Will change.
 
> WebCore/dom/DocumentMarkerController.h:48
>  +      void copyMarkers(Node *srcNode, unsigned startOffset, int length, Node *dstNode, int delta, DocumentMarker::MarkerType = DocumentMarker::AllMarkers);
> Style.
> 

Will fix style.

> WebCore/editing/Editor.cpp:1566
>  +          frame()->document()->markerController()->removeMarkers(selectedRange.get(), DocumentMarker::Spelling);
> I might have called the markerController() just markers().
> 

Will rename.

> LGTM.

[Daniel Bates]

Committed r65787: <http://trac.webkit.org/changeset/65787>