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+

Hajime Morrita
Reported 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.
Attachments
Patch (16.91 KB, patch)
2011-04-07 19:53 PDT, Hajime Morrita 		no flags
DetailsFormatted DiffDiff
Patch (16.90 KB, patch)
2011-04-13 10:09 PDT, Hajime Morrita 		rniwa: review+
DetailsFormatted DiffDiff
Show Obsolete (1) View All Add attachment proposed patch, testcase, etc.
Hajime Morrita
Comment 1 2011-04-07 19:53:25 PDT
Created attachment 88758 [details] Patch
Ryosuke Niwa
Comment 2 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.
Hajime Morrita
Comment 3 2011-04-13 10:09:35 PDT
Created attachment 89395 [details] Patch
Hajime Morrita
Comment 4 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.
Ryosuke Niwa
Comment 5 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.
Hajime Morrita
Comment 6 2011-04-15 10:57:17 PDT
Committed r83991: <http://trac.webkit.org/changeset/83991>
Note You need to log in before you can comment on or make changes to this bug.