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.
Created attachment 88758 [details] Patch
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.
Created attachment 89395 [details] Patch
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.
(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.
Committed r83991: <http://trac.webkit.org/changeset/83991>