[Webkit-unassigned] [Bug 47630] reversion bubble in WebViews

Thu Nov 4 11:22:45 PDT 2010

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

mitz at webkit.org changed:

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

--- Comment #2 from mitz at webkit.org  2010-11-04 11:22:45 PST ---
(From update of attachment 72904)
View in context: https://bugs.webkit.org/attachment.cgi?id=72904&action=review

Generally good, but enough comments to warrant a second round.

> WebCore/ChangeLog:18
> +        1. On Mac OS X, the result of spell checking is partly determined by past user usage. We can't
> +           realiably generating test cases until we can disable user custom data during spell checking.

Is there a way to set up the environment for DumpRenderTree such that it won’t be affected by user data?

> WebCore/ChangeLog:21
> +        This patch is to add reversion to correction panel. It consistis of following major code changes:

Typo: “consistis”

> WebCore/ChangeLog:37
> +          the new selection is a carret selection at end of a previously corrected word.

Typo: “carret”.

> WebCore/WebCore.vcproj/WebCore.vcproj:45856
> +			    RelativePath="..\editing\CorrectionPanelInfo.h"
> +			    >

You used spaces instead of tabs for some of the indentation here.

> WebCore/editing/CorrectionPanelInfo.h:38
> +#endif /* #if PLATFORM(MAC) && !defined(BUILDING_ON_TIGER) && !defined(BUILDING_ON_LEOPARD) && !defined(BUILDING_ON_SNOW_LEOPARD) */

Use // for the trailing comment.

> WebCore/editing/CorrectionPanelInfo.h:49
> +    enum {
> +        PanelTypeCorrection = 0,
> +        PanelTypeReversion
> +    };
> +    typedef unsigned PanelType;

This is not a bit mask, so you should just name the enum and avoid the typedef.

> WebCore/editing/CorrectionPanelInfo.h:58
> +enum CorrectionWasRejectedOrNot { CorrectionWasNotRejected, CorrectionWasRejected };

I wonder if “Accepted” would be better than “Not Rejected”.

> WebCore/editing/Editor.cpp:500
> +    if (currentSelection.isCaret() && currentSelection != oldSelection) {
> +        VisiblePosition selectionPosition = currentSelection.start();
> +        VisiblePosition endPositionOfWord = endOfWord(selectionPosition, LeftWordIfOnBoundary);
> +        if (selectionPosition == endPositionOfWord) {
> +            Position position = endPositionOfWord.deepEquivalent();
> +            if (position.anchorType() == Position::PositionIsOffsetInAnchor) {
> +                Node* node = position.containerNode();
> +                int endOffset = position.offsetInContainerNode();
> +                Vector<DocumentMarker> markers = node->document()->markers()->markersForNode(node);
> +                size_t markerCount = markers.size();
> +                for (size_t i = 0; i < markerCount; ++i) {
> +                    const DocumentMarker& marker = markers[i];
> +                    if (marker.type == DocumentMarker::CorrectionIndicator && static_cast<int>(marker.endOffset) == endOffset) {
> +                        RefPtr<Range> wordRange = Range::create(frame()->document(), node, marker.startOffset, node, marker.endOffset);
> +                        String currentWord = plainText(wordRange.get());
> +                        if (currentWord.length() > 0 && marker.description.length() > 0) {
> +                            m_correctionPanelInfo.m_rangeToBeReplaced = wordRange;
> +                            m_correctionPanelInfo.m_replacementString = marker.description;
> +                            startCorrectionPanelTimer(CorrectionPanelInfo::PanelTypeReversion);
> +                        }
> +                        break;
> +                    }
> +                }
> +            }
> +        }
> +    }

You could write this using early returns to avoid this deep indentation.

> WebCore/editing/Editor.cpp:2490
> +            return client()->dismissCorrectionPanel(CorrectionWasRejected);

No need for “return” here.

> WebCore/editing/Editor.cpp:2511
> +            return client()->dismissCorrectionPanel(correctionWasRejectedOrNot);

Ditto.

You can also change this method and handleCancelOperation() to have early returns instead of indenting the entire body of the method.

> WebCore/platform/graphics/mac/GraphicsContextMac.mm:27
> +#import "CorrectionPanelInfo.h"

It’s a layering violation to include in platform/ files headers from other parts of WebCore. I suggest that you just leave this file as-is (with the BUILDING_ON checks).

> WebKit/mac/WebCoreSupport/WebEditorClient.mm:873
> +    NSCorrectionBubbleType correctionPanelType = panelType == WebCore::CorrectionPanelInfo::PanelTypeCorrection
> +                                                 ? NSCorrectionBubbleTypeCorrection
> +                                                 : NSCorrectionBubbleTypeReversion;

Please just write this expression in a single line or two lines with normal 4-space indentation for the second line. You should probably name this “correctionBubbleType” or just “bubbleType”.

> WebKit/mac/WebCoreSupport/WebEditorClient.mm:875
> +        if (!acceptedString && correctionPanelType == WebCore::CorrectionPanelInfo::PanelTypeCorrection) {

You’re comparing a variable of type NSCorrectionBubbleType to a WebCore enum. Didn’t you mean to compare to NSCorrectionBubbleTypeCorrection?

> WebKit/mac/WebCoreSupport/WebEditorClient.mm:878
> +        } else if (acceptedString && correctionPanelType == WebCore::CorrectionPanelInfo::PanelTypeReversion) {