Bug 47630 – reversion bubble in WebViews

• Component: New Bugs
• Status: RESOLVED FIXED

[Jia Pu]

Feature parity with AppKit.

<rdar://problem/8530960>

[Jia Pu]

Created attachment 72904 [details]
Proposed patch (v1)

Comment on attachment 72904 [details]
Proposed patch (v1)

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) {

Similar question here.

> WebKit/mac/WebCoreSupport/WebEditorClient.mm:888
> +        if (correctionWasRejectedOrNot)

This is confusing to read. If you use the variable as a boolean expression, you should give it a non-ambiguous name. Or you can just explicitly compare to one of the enum values.

[Jia Pu]

(In reply to comment #2)
> (From update of attachment 72904 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=72904&action=review
> 
> Generally good, but enough comments to warrant a second round.

Thanks for the review. I will make change as you suggest except two places that I have replied to your comment below.

> 
> > 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?

An SPI has been recently added to AppKit to disable user data. I suggest we hook up DumpRenderTree with said SPI in another patch. However, to make the automated test fast, we still need to be able to fake the delay.


> > WebCore/editing/CorrectionPanelInfo.h:58
> > +enum CorrectionWasRejectedOrNot { CorrectionWasNotRejected, CorrectionWasRejected };
> 
> I wonder if “Accepted” would be better than “Not Rejected”.

The reason for this more confusing naming is due to the three different ways with which the correction panel can be dismissed. 
1. Reject: when user press ESC key or click on the cross button on the panel.
2. Ignore: when user continues typing.
3. Accept: when user types whitespace or punctuation mark.
This argument is used to distinguish case 1 from case 2 and 3, hence the name. Maybe I should use a enum type with three values, even though the handling of two of those values would be the same. That would make explicit the concept I just described. Thought?

(In reply to comment #3)
> (In reply to comment #2)
> > (From update of attachment 72904 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=72904&action=review
> > 
> > Generally good, but enough comments to warrant a second round.
> 
> Thanks for the review. I will make change as you suggest except two places that I have replied to your comment below.
> 
> > 
> > > 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?
> 
> An SPI has been recently added to AppKit to disable user data. I suggest we hook up DumpRenderTree with said SPI in another patch.

Good idea.

> However, to make the automated test fast, we still need to be able to fake the delay.

I think that is also doable in the long term (as a separate change to WebCore and DumpRenderTree).

> 
> 
> > > WebCore/editing/CorrectionPanelInfo.h:58
> > > +enum CorrectionWasRejectedOrNot { CorrectionWasNotRejected, CorrectionWasRejected };
> > 
> > I wonder if “Accepted” would be better than “Not Rejected”.
> 
> The reason for this more confusing naming is due to the three different ways with which the correction panel can be dismissed. 
> 1. Reject: when user press ESC key or click on the cross button on the panel.
> 2. Ignore: when user continues typing.
> 3. Accept: when user types whitespace or punctuation mark.
> This argument is used to distinguish case 1 from case 2 and 3, hence the name. Maybe I should use a enum type with three values, even though the handling of two of those values would be the same. That would make explicit the concept I just described. Thought?

I think it’s fine as-is.

[Jia Pu]

Created attachment 72985 [details]
Proposed patch (v2)

Revised patch based on comment #2 through comment #4.

Jia, did you intentionally obsolete the newer patch?

[Jia Pu]

(In reply to comment #6)
> Jia, did you intentionally obsolete the newer patch?

Yes. Doug found a minor bug. A new patch in coming.

[Jia Pu]

Created attachment 72997 [details]
Proposed patch (v3)

This patch contains:
1. Changes based on comment #2 through comment #4.
2. A oneliner change in Editor::RespondToChangedSelection() to stop correction panel timer. This prevents reversion from showing up if the user move selection away from the end of a correction word before the timer fires.

[WebKit Commit Bot]

Comment on attachment 72997 [details]
Proposed patch (v3)

Rejecting patch 72997 from commit-queue.

Failed to run "['./WebKitTools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '--bot-id=abarth-cq-sl', 'apply-attachment', '--force-clean', '--non-interactive', 72997]" exit_code: 2
Last 500 characters of output:
#1 succeeded at 1 with fuzz 3.
patching file WebKit2/WebProcess/WebCoreSupport/WebEditorClient.h
patching file WebKit2/WebProcess/WebCoreSupport/mac/WebEditorClientMac.mm
patching file WebKit/mac/ChangeLog
Hunk #1 succeeded at 1 with fuzz 3.
patching file WebKit/mac/WebCoreSupport/WebEditorClient.h
patching file WebKit/mac/WebCoreSupport/WebEditorClient.mm

Failed to run "[u'/Users/abarth/git/webkit-queue/WebKitTools/Scripts/svn-apply', u'--reviewer', u'Dan Bernstein', u'--force']" exit_code: 1

Full output: http://queues.webkit.org/results/5316003

[Jia Pu]

It seems the xcode project file may be the one that gives the trouble. At least that's what I got when trying to merge with TOT locally.

Committed r71385 <http://trac.webkit.org/projects/webkit/changeset/71385>.