Bug 58112

Summary: DocumentMaker::AllMarkers should not be a part of DocumentMarker::MarkerType
Product: WebKit Reporter: Hajime Morrita <morrita>
Component: HTML EditingAssignee: Hajime Morrita <morrita>
Status: RESOLVED FIXED    
Severity: Normal CC: rniwa
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: All   
OS: OS X 10.5   
Bug Depends on:    
Bug Blocks: 58111    
Attachments:
Description Flags
Patch
none
Patch rniwa: review+

Description Hajime Morrita 2011-04-07 19:18:44 PDT
DocumentMarker::MarkerType is a enumeration type. But AllMarkers are sum of all types and used as a mask.
and MarkerType is used as a enum at some part and as a mask on the other part. 
This mixture should be removed and AllMarkers should have a different type other than MarkerType.

My plan is to convert MarkerTypes (typedef of unsigned) to a class and make AllMarkers an instance of that class.
At a glance, this makes a bit over-engineering. But this must help upcoming refactoring series.
Comment 1 Hajime Morrita 2011-04-07 19:53:25 PDT
Created attachment 88758 [details]
Patch
Comment 2 Ryosuke Niwa 2011-04-12 18:08:42 PDT
Comment on attachment 88758 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=88758&action=review

> Source/WebCore/dom/DocumentMarker.h:63
> +        bool matches(MarkerType type) const { return m_mask & type; }

This should be "has" or "contains"?  "matches" sounds like equality.

> Source/WebCore/dom/DocumentMarkerController.cpp:160
> -    if (!possiblyHasMarkers(markerType))
> +    if (!possiblyHasMarkers(DocumentMarker::AllMarkers()))

I don't quite understand why this change doesn't change the behavior.

> Source/WebCore/dom/DocumentMarkerController.cpp:181
> -        if (marker.endOffset < startOffset || (marker.type != markerType && markerType != DocumentMarker::AllMarkers))
> +        if (marker.endOffset < startOffset)

Ditto.

> Source/WebCore/dom/DocumentMarkerController.cpp:537
> -    if (!possiblyHasMarkers(markerType))
> +    if (!possiblyHasMarkers(DocumentMarker::AllMarkers()))

Ditto.

> Source/WebCore/dom/DocumentMarkerController.cpp:552
> -        if (marker.startOffset >= startOffset && (markerType == DocumentMarker::AllMarkers || marker.type == markerType)) {
> +        if (marker.startOffset >= startOffset) {

Ditto.
Comment 3 Hajime Morrita 2011-04-13 10:09:35 PDT
Created attachment 89395 [details]
Patch
Comment 4 Hajime Morrita 2011-04-13 10:13:10 PDT
Hi Ryosuke, thank you for reviewing!
I updated the patch for addressing your point.

(In reply to comment #2)
> (From update of attachment 88758 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=88758&action=review
> 
> > Source/WebCore/dom/DocumentMarker.h:63
> > +        bool matches(MarkerType type) const { return m_mask & type; }
> 
> This should be "has" or "contains"?  "matches" sounds like equality.
Renamed.

> 
> > Source/WebCore/dom/DocumentMarkerController.cpp:160
> > -    if (!possiblyHasMarkers(markerType))
> > +    if (!possiblyHasMarkers(DocumentMarker::AllMarkers()))
> 
> I don't quite understand why this change doesn't change the behavior.
For shiftMarkers() and copyMarkers(), all caller sites call with default parameter for markers,
whose value is AllMarkers. So I removed the marker argument.
Actually copyMakers() and shiftaMarkers() are used for syncing markers with DOM modifications,
So limiting marker types doesn't make sense.

> 
> > Source/WebCore/dom/DocumentMarkerController.cpp:181
> > -        if (marker.endOffset < startOffset || (marker.type != markerType && markerType != DocumentMarker::AllMarkers))
> > +        if (marker.endOffset < startOffset)
> 
> Ditto.
> 
> > Source/WebCore/dom/DocumentMarkerController.cpp:537
> > -    if (!possiblyHasMarkers(markerType))
> > +    if (!possiblyHasMarkers(DocumentMarker::AllMarkers()))
> 
> Ditto.
> 
> > Source/WebCore/dom/DocumentMarkerController.cpp:552
> > -        if (marker.startOffset >= startOffset && (markerType == DocumentMarker::AllMarkers || marker.type == markerType)) {
> > +        if (marker.startOffset >= startOffset) {
> 
> Ditto.
Comment 5 Ryosuke Niwa 2011-04-14 10:48:06 PDT
(In reply to comment #4)
> > > Source/WebCore/dom/DocumentMarkerController.cpp:160
> > > -    if (!possiblyHasMarkers(markerType))
> > > +    if (!possiblyHasMarkers(DocumentMarker::AllMarkers()))
> > 
> > I don't quite understand why this change doesn't change the behavior.
> For shiftMarkers() and copyMarkers(), all caller sites call with default parameter for markers,
> whose value is AllMarkers. So I removed the marker argument.
> Actually copyMakers() and shiftaMarkers() are used for syncing markers with DOM modifications,
> So limiting marker types doesn't make sense.

I see. Thanks for the clarification.  It made my review process much easier.
Comment 6 Hajime Morrita 2011-04-15 10:57:17 PDT
Committed r83991: <http://trac.webkit.org/changeset/83991>