[webkit-reviews] review denied: [Bug 47630] reversion bubble in WebViews : [Attachment 72904] Proposed patch (v1)

[bugzilla-daemon at webkit.org]

mitz at webkit.org has denied jpu at apple.com's request for review:
Bug 47630: reversion bubble in WebViews
https://bugs.webkit.org/show_bug.cgi?id=47630

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

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