Bug 58112 – DocumentMaker::AllMarkers should not be a part of DocumentMarker::MarkerType

• Component: HTML Editing
• Status: RESOLVED FIXED

[Hajime Morrita]

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.

[Hajime Morrita]

Created attachment 88758 [details]
Patch

[Ryosuke Niwa]

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.

[Hajime Morrita]

Created attachment 89395 [details]
Patch

[Hajime Morrita]

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.

[Ryosuke Niwa]

(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.

[Hajime Morrita]

Committed r83991: <http://trac.webkit.org/changeset/83991>