[Webkit-unassigned] [Bug 44958] Add support for autocorrection UI on Mac OS X.

[bugzilla-daemon at webkit.org]

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


mitz at webkit.org changed:

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




--- Comment #8 from mitz at webkit.org  2010-08-31 16:16:19 PST ---
(From update of attachment 66118)
> 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.

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