[webkit-reviews] review denied: [Bug 48078] Editing a word with misspell or autocorrection underline should remove the underline when the editing changes the word. : [Attachment 72140] Proposed patch (v3)

[bugzilla-daemon at webkit.org]

mitz at webkit.org has denied jpu at apple.com's request for review:
Bug 48078: Editing a word with misspell or autocorrection underline should
remove the underline when the editing changes the word.
https://bugs.webkit.org/show_bug.cgi?id=48078

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

------- Additional Comments from mitz at webkit.org
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;