[Webkit-unassigned] [Bug 48078] Editing a word with misspell or autocorrection underline should remove the underline when the editing changes the word.

[bugzilla-daemon at webkit.org]

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


mitz at webkit.org changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
  Attachment #72140|review?                     |review-
               Flag|                            |




--- Comment #9 from mitz at webkit.org  2010-10-27 23:57:02 PST ---
(From update of attachment 72140)
View in context: https://bugs.webkit.org/attachment.cgi?id=72140&action=review

r- because of config.h and the problem in hasMarker().

> WebCore/config.h:176
> +// Some platforms provide UI for suggesting autocorrection.
> +#define SUPPORT_AUTOCORRECTION_PANEL 0
> +// Some platforms use spelling and autocorrection markers to provide visual cue.
> +// On such platform, if word with marker is edited, we need to remove the marker.
> +#define REMOVE_MARKERS_UPON_EDITING 0
> +
>  #if PLATFORM(MAC)
>  // New theme
>  #define WTF_USE_NEW_THEME 1
> +#if !defined(BUILDING_ON_TIGER) && !defined(BUILDING_ON_LEOPARD) && !defined(BUILDING_ON_SNOW_LEOPARD)
> +#define SUPPORT_AUTOCORRECTION_PANEL 1
> +#define REMOVE_MARKERS_UPON_EDITING 1
> +#endif // !defined(BUILDING_ON_TIGER) && !defined(BUILDING_ON_LEOPARD) && !defined(BUILDING_ON_SNOW_LEOPARD)
>  #endif // PLATFORM(MAC)

config.h isn’t the right place for defining these things. These are only relevant to one or two files in editing and should probably be defined in some header in editing (that’s included by those files).

> WebCore/dom/DocumentMarkerController.cpp:329
> +    if (iterator == m_markers.end())
> +        return;
> +    removeMarkersFromMarkerMapVectorPair(node, iterator->second, markerType);

In this case, the “early return” actually makes the function longer, since there’s only one statement after the return.

> WebCore/dom/DocumentMarkerController.cpp:535
> +bool DocumentMarkerController::hasMarker(Range *range, const Vector<DocumentMarker::MarkerType>& markerTypes)

The space should go on the other side of the star.

> WebCore/dom/DocumentMarkerController.cpp:546
> +    Node* startContainer = range->startContainer(ec);
> +    if (ec)
> +        return false;
> +    Node* endContainer = range->endContainer(ec);
> +    if (ec)
> +        return false;

I wonder if we can guarantee that the range has start and end containers and replace those returns with assertions.

> WebCore/dom/DocumentMarkerController.cpp:553
> +            for (Vector<DocumentMarker>::const_iterator markerIterator = markers.begin(); markerIterator != end; ++markerIterator) {

markerIterator is a good name, but we often use 'it' as the name of a loop iterator.

> WebCore/dom/DocumentMarkerController.cpp:555
> +                if (foundIndex != notFound && markerIterator->endOffset > static_cast<unsigned>(range->startOffset()) && markerIterator->startOffset < static_cast<unsigned>(range->endOffset()))

This test is wrong. If node is the startContainer, then you don’t care about the endOffset unless the startContainer is also the endContainer.

> WebCore/dom/DocumentMarkerController.cpp:560
> +            Vector<DocumentMarker> markers = markersForNode(node);
> +            Vector<DocumentMarker>::const_iterator end = markers.end();

Instead of repeating these two statements in the if and else blocks, you could just move them outside.

Perhaps it would be simpler to organize everything in a single for loop and test for the edge cases inside the loop.

> WebCore/editing/Editor.cpp:2977
> +    Vector<DocumentMarker::MarkerType> markerTypeToRemove;

This should be called markerTypesToRemove and initialized with a size of 2:
    Vector<DocumentMarker::MarkerType> markerTypesToRemove(2);
then instead of appending you can assign to the 0th and 1st elements.
Better yet, make this a static and make a helper function to initialize this once.

    const Vector<DocumentMarker::MarkerType>& markerTypesToRemove = createMarkerTypesToRemove();

Another idea is to use a bit mask instead of a vector. Checking if the marker type is in the vector is slower than testing a bit mask.

> WebCore/editing/Editor.cpp:2984
> +    Vector<RangeMarkerPair> markerToRemove;

This should be called markersToRemove. To avoid allocation on the heap, you can give it a small inline capacity, e.g.
    Vector<RangeMarkerPair, 16> markerToRemove;

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