[webkit-reviews] review granted: [Bug 54893] Remove CorrectionIndicator markers sooner. : [Attachment 83701] Proposed patch (v2)

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has granted jpu at apple.com's request for review:
Bug 54893: Remove CorrectionIndicator markers sooner.
https://bugs.webkit.org/show_bug.cgi?id=54893

Attachment 83701: Proposed patch (v2)
https://bugs.webkit.org/attachment.cgi?id=83701&action=review

------- Additional Comments from Darin Adler <darin at apple.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=83701&action=review

I’m saying review+ but I think this could use some more refinement before
landing. At least one of my comments below may be something you’ll want to do.

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

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

> Source/WebCore/dom/DocumentMarkerController.cpp:585
> +bool
DocumentMarkerController::possiblyHasMarkers(DocumentMarker::MarkerTypes types)

> +{
> +    return m_absentMarkerTypeCache & types;
> +}

Seems to me this should be an inline function. SInce it’s only called within
the file it doesn’t have to be in the header.

> Source/WebCore/dom/DocumentMarkerController.cpp:594
> +    ExceptionCode ec = 0;
> +    Node* startContainer = range->startContainer(ec);
> +    Node* endContainer = range->endContainer(ec);

There are versions of the startContainer, endContainer, startOffset, and
endOffset that do not take ExceptionCode arguments and simply assert that the
range is in a valid state. You should use those for this type of caller inside
the engine and not bother with ExceptionCode.

> Source/WebCore/dom/DocumentMarkerController.cpp:599
> +	   unsigned endOffset = node == endContainer ? range->endOffset(ec) :
INT_MAX;

This variable’s type is unsigned, but the constant here is INT_MAX. That
inconsistency doesn’t make much sense. Also, we normally prefer to use the C++
maximum functions, which are a bit wordier, but make it clearer how they relate
to types:

    numeric_limits<unsigned>::max()

> 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();

> Source/WebCore/dom/DocumentMarkerController.h:52
> +    bool possiblyHasMarkers(DocumentMarker::MarkerTypes);

Does this need to be public?

> Source/WebCore/editing/EditCommand.h:63
> +    virtual void setShouldRetainAutocorrectionIndicator(bool) {};

Extra semicolon here. Also, there’s no real reason to put the function
definition here inline. It should be non-inline in the cpp file.

> Source/WebCore/editing/Editor.cpp:1224
> +    bool autocorrectionIsApplied = shouldConsiderApplyingAutocorrection &&
applyAutocorrectionBeforeTypingIfAppropriate() ? true : false;

The "? true : false" here is not needed. That kind of idiom is needed for types
like BOOL, but not the compiler built-in type bool.

> Source/WebCore/editing/Editor.cpp:1260
> +    bool autocorrectionIsApplied =
applyAutocorrectionBeforeTypingIfAppropriate();

Maybe was instead of is in this name?

> Source/WebCore/editing/Editor.h:433
> +    // Return true if correction is applied, false otherwise.

I think was instead of is in this comment.