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

[bugzilla-daemon at webkit.org]

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


Darin Adler <darin at apple.com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
  Attachment #83701|review?                     |review+
               Flag|                            |




--- Comment #5 from Darin Adler <darin at apple.com>  2011-02-28 09:41:35 PST ---
(From update of attachment 83701)
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.

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