[Webkit-unassigned] [Bug 54893] Remove CorrectionIndicator markers sooner.

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=54893





--- Comment #7 from jpu at apple.com  2011-02-28 14:42:25 PST ---
(In reply to comment #5)
> (From update of attachment 83701 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=83701&action=review
> 
> > Source/WebCore/dom/DocumentMarkerController.cpp:42
> > +    :m_absentMarkerTypeCache(0)
> 
> There should be a space after this colon to match our usual coding style.
> 
> I think m_absentMarkerTypeCache is a confusing name. I understand how this is a cache in the broadest sense, but I don’t think the word cache is helpful in the name. And since this word has a bit set for each marker that is present, I think it’s strange to use the words “absent marker” in its name. I understand that’s trying to allude to the fact that it’s reliable for checking the absence of a marker, but not for checking the presence: false positives are possible but not false negatives. But using absent in the name to indicate that is unclear.
> 
> Taking a queue from your function name, I think that m_markerTypesPossiblyPresent might be a better name. There are probably even better ones.

I renamed it m_possiblyExistingMarkerTypes. Let me know if you prefer the name you proposed.

> 
> > Source/WebCore/dom/DocumentMarkerController.cpp:549
> > -    if (m_markers.isEmpty())
> > +    if (m_markers.isEmpty() || !possiblyHasMarkers(markerTypes))
> >          return false;
> 
> I think it’s better to check possiblyHasMarkers first. 

Changed as you suggested.

> In fact, we can probably make the isEmpty check an assertion if we make sure to clear the types any time we empty out the markers. We’re probably already really close to that.

Not sure we can do this. Since possiblyHasMarkers() only check on specified marker types. m_markers can still be non-empty when possiblyHasMarkers() returns false.

> 
> > Source/WebCore/dom/DocumentMarkerController.cpp:618
> > +            marker.description = "";
> 
> This is a slightly inefficient way to set a marker’s description to an empty string. These are more efficient:
> 
>     marker.description = emptyAtom;
> 
> Or if we want a null string instead of empty string:
> 
>     marker.description = String();

Changed to using String(). Also changed relevant code to use String::isNull() instead of String::length() to do checking.

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.