Bug 54893 – Remove CorrectionIndicator markers sooner.

• Component: New Bugs
• Status: RESOLVED FIXED

[Jia Pu]

This is a Mac specific change to make the autocorrection behavior in WebKit to be consistent with that in AppKit.

<rdar://problem/8997524>

[Jia Pu]

Created attachment 83232 [details]
Proposed patch (v1)

[Darin Adler]

Comment on attachment 83232 [details]
Proposed patch (v1)

View in context: https://bugs.webkit.org/attachment.cgi?id=83232&action=review

> Source/WebCore/dom/DocumentMarkerController.cpp:580
> +    for (TextIterator markedText(range); !markedText.atEnd(); markedText.advance()) {
> +        RefPtr<Range> textPiece = markedText.range();
> +        unsigned startOffset = textPiece->startOffset();
> +        unsigned endOffset = textPiece->endOffset();
> +        MarkerMapVectorPair* vectorPair = m_markers.get(textPiece->startContainer());
> +        if (!vectorPair)
> +            continue;

It seems wrong to iterate a range by using the TextIterator. We can easily iterate all the nodes in a range and remove the markers found in those nodes. There’s no real reason to involve the text iterator. TextIterator does more work that unneeded and potentially expensive. The code in DocumentMarkerController::setMarkersActive is an example of how to do this.

> Source/WebCore/dom/DocumentMarkerController.h:74
> +    // This function clears the description even when the marker only partially follows in the specified range.
> +    void clearMarkerDescriptions(Range*, DocumentMarker::MarkerTypes);

You mean "partially falls" not "partially follows".

I think a better name for the function would have the word intersecting in it. So you wouldn’t need that comment so much.

> Source/WebCore/editing/Editor.cpp:1228
> +    if (text.length() == 1 && (category(text[0]) & (Punctuation_Dash | Punctuation_Open | Punctuation_Close | Punctuation_Connector | Punctuation_Other)) && !isAmbiguousBoundaryCharacter(text[0]))

I think the punctuation check here needs to be in a separate helper function. It can be in this file. But writing it out like that makes the code unnecessarily hard to read.

Also, can you use isPunct instead of category?

> Source/WebCore/editing/Editor.h:405
> +    bool m_hasCorrectionIndicatorMarkers;
> +    bool m_editingCommandShouldRemoveCorrectionIndicators;

I don’t see initial values for these in the constructor. They are needed.

Using boolean state like this is really awkward. The more of these we add the more spaghetti-like the code becomes. Is there some other way to do it?

[Jia Pu]

(In reply to comment #2)
> (From update of attachment 83232 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=83232&action=review
> 
> > Source/WebCore/dom/DocumentMarkerController.cpp:580
> > +    for (TextIterator markedText(range); !markedText.atEnd(); markedText.advance()) {
> > +        RefPtr<Range> textPiece = markedText.range();
> > +        unsigned startOffset = textPiece->startOffset();
> > +        unsigned endOffset = textPiece->endOffset();
> > +        MarkerMapVectorPair* vectorPair = m_markers.get(textPiece->startContainer());
> > +        if (!vectorPair)
> > +            continue;
> 
> It seems wrong to iterate a range by using the TextIterator. We can easily iterate all the nodes in a range and remove the markers found in those nodes. There’s no real reason to involve the text iterator. TextIterator does more work that unneeded and potentially expensive. The code in DocumentMarkerController::setMarkersActive is an example of how to do this.

I was confused by the existing usage of TextIterator in DocumentMarkerController::addMarker. I assumed that TextIterator is preferred, since it seems prevent iterating on nodes that are not really leaf text nodes. I will change this loop to iterating on node directly.


> 
> > Source/WebCore/editing/Editor.h:405
> > +    bool m_hasCorrectionIndicatorMarkers;
> > +    bool m_editingCommandShouldRemoveCorrectionIndicators;
> 
> I don’t see initial values for these in the constructor. They are needed.
> 
> Using boolean state like this is really awkward. The more of these we add the more spaghetti-like the code becomes. Is there some other way to do it?

Yes. I realize this issue. I did think about an alternative for each of these two variables. 

m_hasCorrectionIndicatorMarkers serves the purpose of caching, since we want to find out quickly if there isn't a marker with this value. I could move this sort of caching into DocumentMarkerController class.

m_editingCommandShouldRemoveCorrectionIndicator is very much a property of edit command. So we could add getter and setter for this property in EditCommand. 

Let know what you think about these alternatives.

[Jia Pu]

Created attachment 83701 [details]
Proposed patch (v2)

[Darin Adler]

Comment on attachment 83701 [details]
Proposed patch (v2)

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.

[Jia Pu]

Created attachment 84124 [details]
Proposed patch (v3)

[Jia Pu]

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

[Darin Adler]

(In reply to comment #7)
> > 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.

I think you got my suggestion backwards. The assertion would be this:

    if (!possiblyHasMarkers(markerTypes))
        return;
    ASSERT(!m_markers.isEmpty());

Would there be some case where that would not work?

[Darin Adler]

(In reply to comment #8)
> Would there be some case where that would not work?

All we would need to do would be to add code to set m_possiblyExistingMarkerTypes to 0 if m_markers becomes empty in any function that removes markers that doesn’t already update m_possiblyExistingMarkerTypes.

[Darin Adler]

Comment on attachment 84124 [details]
Proposed patch (v3)

View in context: https://bugs.webkit.org/attachment.cgi?id=84124&action=review

> Source/WebCore/dom/DocumentMarkerController.cpp:585
> +inline bool DocumentMarkerController::possiblyHasMarkers(DocumentMarker::MarkerTypes types)
> +{
> +    return m_possiblyExistingMarkerTypes & types;
> +}

Because this is an inline, I think we might need it to be at the top of the file, before any callers. Not sure. I think the EWS bot may tell us if I’m right.

> Source/WebCore/dom/DocumentMarkerController.h:86
> +    MarkerMap m_markers;
> +    // Provide a quick way to determine whether a particular marker type is absent without going through the map.
> +    DocumentMarker::MarkerTypes m_possiblyExistingMarkerTypes;
> +    bool possiblyHasMarkers(DocumentMarker::MarkerTypes);

Normally we put the data members in a separate paragraph after all the function members.

[Jia Pu]

(In reply to comment #8)
> (In reply to comment #7)
> > > 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.
> 
> I think you got my suggestion backwards. The assertion would be this:
> 
>     if (!possiblyHasMarkers(markerTypes))
>         return;
>     ASSERT(!m_markers.isEmpty());
> 
> Would there be some case where that would not work?

Ah, then we will have to do:

 if (!possiblyHasMarkers(DocumentMarker::AllTypes))
         return;
  ASSERT(!m_markers.isEmpty());

Then we will need to check again in functions that are only interested in particular types:
if (!possiblyHasMarkers(markerTypes))
         return;

[Darin Adler]

(In reply to comment #11)
> Ah, then we will have to do:
> 
>  if (!possiblyHasMarkers(DocumentMarker::AllTypes))
>          return;
>   ASSERT(!m_markers.isEmpty());
> 
> Then we will need to check again in functions that are only interested in particular types:
> if (!possiblyHasMarkers(markerTypes))
>      return;

No we wouldn’t. My original code would work fine:

    if (!possiblyHasMarkers(markerTypes))
        return;
    ASSERT(!m_markers.isEmpty());

If it possibly has a marker of any one type, then it has at least one marker. If ever got down to zero markers we would have zeroed out the entire marker type mask.

[Jia Pu]

Created attachment 84149 [details]
Proposed patch (v5)

[Jia Pu]

(In reply to comment #13)
> Created an attachment (id=84149) [details]
> Proposed patch (v5)

Updated the patch based on comment 8 to 10.

[WebKit Commit Bot]

Comment on attachment 84149 [details]
Proposed patch (v5)

Rejecting attachment 84149 [details] from commit-queue.

Failed to run "['./Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '--bot-id=cr-jail-3', 'build-..." exit_code: 2

Last 500 characters of output:
siting/canvas .
platform/mac/editing/deleting .....
platform/mac/editing/input ...............
platform/mac/editing/pasteboard ....
platform/mac/editing/selection ...
platform/mac/editing/spelling .
platform/mac/editing/spelling/removing-underline-after-accepting-autocorrection-using-punctuation.html -> failed

Exiting early after 1 failures. 13303 tests run.
379.17s total testing time

13302 test cases (99%) succeeded
1 test case (<1%) had incorrect layout
11 test cases (<1%) had stderr output

Full output: http://queues.webkit.org/results/8076515

[Jia Pu]

(In reply to comment #15)
> (From update of attachment 84149 [details])
> Rejecting attachment 84149 [details] from commit-queue.
> 
> Failed to run "['./Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '--bot-id=cr-jail-3', 'build-..." exit_code: 2
> 
> Last 500 characters of output:
> siting/canvas .
> platform/mac/editing/deleting .....
> platform/mac/editing/input ...............
> platform/mac/editing/pasteboard ....
> platform/mac/editing/selection ...
> platform/mac/editing/spelling .
> platform/mac/editing/spelling/removing-underline-after-accepting-autocorrection-using-punctuation.html -> failed
> 
> Exiting early after 1 failures. 13303 tests run.
> 379.17s total testing time
> 
> 13302 test cases (99%) succeeded
> 1 test case (<1%) had incorrect layout

Ah, the new test doesn't apply to earlier Mac OS X  versions. I will update the Skipped files.

> 11 test cases (<1%) had stderr output

Not quite sure what this is about. Is this just a side effect of the failing test case?

> 
> Full output: http://queues.webkit.org/results/8076515

[Jia Pu]

Created attachment 84240 [details]
Proposed patch (v6)

Edited Skipped files to fix testing failure. And Merged TOT.

[Darin Adler]

(In reply to comment #16)
> > 11 test cases (<1%) had stderr output
> 
> Not quite sure what this is about. Is this just a side effect of the failing test case?

We don’t consider stderr output a failure; I think that 11 tests having it is normal. There are some tests that have some stderr output, in most cases because of some underlying operating system component that reports things directly to stderr.

[WebKit Commit Bot]

Comment on attachment 84240 [details]
Proposed patch (v6)

Clearing flags on attachment: 84240

Committed r80023: <http://trac.webkit.org/changeset/80023>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.