[webkit-reviews] review denied: [Bug 47630] reversion bubble in WebViews : [Attachment 72904] Proposed patch (v1)
bugzilla-daemon at webkit.org
bugzilla-daemon at webkit.org
Thu Nov 4 11:22:44 PDT 2010
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.
More information about the webkit-reviews
mailing list