[webkit-reviews] review denied: [Bug 44958] Add support for autocorrection UI on Mac OS X. : [Attachment 66118] Proposed patch (v3)

[bugzilla-daemon at webkit.org]

mitz at webkit.org has denied jpu at apple.com's request for review:
Bug 44958: Add support for autocorrection UI on Mac OS X.
https://bugs.webkit.org/show_bug.cgi?id=44958

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

------- Additional Comments from mitz at webkit.org
> WebCore/ChangeLog:9
> +	   No new tests. (OOPS!)
We cannot check in change log entries with the word OOPS in them. Please remove
this line. 

> WebCore/ChangeLog:11
> +	   Several new member methods are added to EditorClient for
communication between WebCore and WebKit. A new handler,
executeCancelOperation(), is added to EditorCommand.cpp so that WebCore can
intercept the ESC key event to dismiss autocorrection UI. A new DocumentMarker
value, RejectedCorrection, is added to keep track of the corrections that user
has rejected, so that it will not be suggested again later. The autocorrection
is driven by a timer. Every time the editor inserts a new letter, the timer is
reset. If the timer fires, it means neither has user entered any new letter for
current word, nor has he entered whitespace or punctuation to complete the
word. In this case, we query for autocorrection.
Please hard-wrap this at the 80-100 column mark.

> WebCore/ChangeLog:19
> +	   (WebCore::Editor::~Editor): Make sure autocorrection UI is dismiss
before destroying Editor object.
typo: dismiss -> dismissed

> WebCore/ChangeLog:21
> +	   (WebCore::Editor::markAllMisspellingsAndBadGrammarInRanges): Added
an argument to indicate whether we want to show autocorrection UI. Added code
to show autocorrection UI if it is necessary. Replaced all four boolean
arguments with enum to improve readability.
I think a single enum-based bitfield would have been even better. Please wrap
this long line.

> WebCore/editing/Editor.cpp:2373
> +	   markAllMisspellingsAndBadGrammarInRanges(MarkSpelling,
adjacentWords.toNormalizedRange().get(), markGrammar,
selectedSentence.toNormalizedRange().get(), performTextCheckingReplacements,
DoNotShowCorrectionPanel);
Might be better to use MarkGrammar instead of markGrammar here.

> WebCore/editing/Editor.cpp:2685
> +			   // We only the show correction panel on the last
word.
typo: the show -> show the

> WebCore/editing/Editor.cpp:2701
> +	       if (doReplacement) {
I think I may have misled you about this in an earlier review, but it seems
like this block could now be entered in cases where previously it wasn’t,
because the result-type check and the canEdit() and shouldInsertText() checks
aren’t applied here.

> WebCore/editing/Editor.cpp:2720
> +    if (selectionChanged && !shouldShowCorrectionPanel) {
Can you explain this change? How could selectionChanged be true if we are just
showing the correction UI?

> WebCore/editing/Editor.h:351
> +    void correctionPanelTimerFired(Timer<Editor>* timer);
Please omit the name “timer” here.

> WebCore/platform/graphics/GraphicsContext.h:276
> +	       TextCheckingInvalidLineStyle = -1,
The invalid value is not needed.

> WebCore/platform/graphics/GraphicsContext.h:284
> +	   void drawLineForTextChecking(const IntPoint&, int width,
TextCheckingLineStyle);
> +#else
>	   void drawLineForMisspellingOrBadGrammar(const IntPoint&, int width,
bool grammar);
> -
> +#endif
It’s really bad the have different GraphicsContext methods for Mac and all the
other ports. It shouldn’t be hard to blindly change all other ports to adopt
the new method.

> WebCore/rendering/InlineTextBox.cpp:785
> +#if PLATFORM(MAC)
> +static GraphicsContext::TextCheckingLineStyle
textCheckingLineStyleForMarkerType(DocumentMarker::MarkerType markerType)
> +{
> +    switch (markerType) {
> +    case DocumentMarker::Spelling:
> +	   return GraphicsContext::TextCheckingSpellingLineStyle;
> +    case DocumentMarker::Grammar:
> +	   return GraphicsContext::TextCheckingGrammarLineStyle;
> +    case DocumentMarker::Replacement:
> +	   return GraphicsContext::TextCheckingReplacementLineStyle;
> +    default:
> +	   return GraphicsContext::TextCheckingInvalidLineStyle;
> +    }
> +}
> +#endif
In the default case you should ASSERT_NOT_REACHED() and return one of the valid
values. It is a programming error to call this method with anything besides
Spelling, Geammar or Replacement.