Bug 44383 - Encapsulate document marker management into DocumentMarkerController
Summary: Encapsulate document marker management into DocumentMarkerController
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebCore Misc. (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Daniel Bates
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2010-08-21 13:43 PDT by Daniel Bates
Modified: 2010-08-22 14:42 PDT (History)
2 users (show)

See Also:


Attachments
Patch (85.22 KB, patch)
2010-08-21 14:21 PDT, Daniel Bates
eric: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Daniel Bates 2010-08-21 13:43:02 PDT
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.
Comment 1 Daniel Bates 2010-08-21 14:21:08 PDT
Created attachment 65035 [details]
Patch

I named the new class DocumentMarkerController and the accessor markerController() (in Document.cpp). I am open to name suggestions.
Comment 2 Eric Seidel (no email) 2010-08-21 14:35:06 PDT
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.
Comment 3 Daniel Bates 2010-08-22 14:39:08 PDT
(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.
Comment 4 Daniel Bates 2010-08-22 14:42:07 PDT
Committed r65787: <http://trac.webkit.org/changeset/65787>