Bug 127015 - Support WebSelections in WK2 on iOS
Summary: Support WebSelections in WK2 on iOS
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit2 (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P1 Normal
Assignee: Enrica Casucci
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2014-01-14 15:43 PST by Enrica Casucci
Modified: 2014-02-21 13:51 PST (History)
3 users (show)

See Also:


Attachments
Patch (44.25 KB, patch)
2014-01-14 16:01 PST, Enrica Casucci
no flags Details | Formatted Diff | Diff
Patch2 (47.29 KB, patch)
2014-01-15 13:00 PST, Enrica Casucci
benjamin: review+
Details | Formatted Diff | Diff
Build fix (1.48 KB, patch)
2014-01-17 09:24 PST, Enrica Casucci
mitz: review+
Details | Formatted Diff | Diff
Patch3 (16.41 KB, patch)
2014-01-21 14:25 PST, Enrica Casucci
no flags Details | Formatted Diff | Diff
Patch4 (14.83 KB, patch)
2014-01-22 18:26 PST, Enrica Casucci
benjamin: review+
Details | Formatted Diff | Diff
block selection part1 (11.08 KB, patch)
2014-02-10 18:01 PST, Enrica Casucci
benjamin: review+
Details | Formatted Diff | Diff
block selection part2 (39.70 KB, patch)
2014-02-20 17:15 PST, Enrica Casucci
no flags Details | Formatted Diff | Diff
Block selection part2 (42.80 KB, patch)
2014-02-21 09:46 PST, Enrica Casucci
no flags Details | Formatted Diff | Diff
Block selection part2 (42.73 KB, patch)
2014-02-21 09:53 PST, Enrica Casucci
benjamin: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Enrica Casucci 2014-01-14 15:43:13 PST
This bug tracks the work required to support selections in non editable content on iOS.
Comment 1 Enrica Casucci 2014-01-14 16:01:00 PST
Created attachment 221212 [details]
Patch
Comment 2 Benjamin Poulain 2014-01-14 16:28:43 PST
Comment on attachment 221212 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=221212&action=review

> Source/WebKit2/ChangeLog:12
> +        This is the first step towards adding support for selections in
> +        non editable content on iOS for WK2.
> +        In particular, this patch adds the basic plumbing to decide which
> +        gesture recognizers are enabled and empty stubs for the gestures
> +        that we'll need to support.

I think you should add an explanation about the sequence of events, and how it differs when the assistant is up or not.
It is far from trivial, the more descriptive the better.

> Source/WebKit2/Shared/PositionInformation.cpp:42
> +    encoder << point;
> +    encoder << isSameAssistedNode;
> +    encoder << wantsHighlight;
> +    encoder << clickableElementName;
> +    encoder << url;
> +    encoder << selectionRects;

I believe you can just use the SimpleArgumentCoder to encode/decode here.

> Source/WebKit2/UIProcess/API/ios/WKInteractionView.mm:168
> +    PositionInformation _positionInfo;
> +    BOOL _hasValidPositionInfo;

Let's not shorten the name.

> Source/WebKit2/UIProcess/API/ios/WKInteractionView.mm:225
> +    [_doubleTapGestureRecognizer.get() setDelegate:nil];
>      [_highlightLongPressGestureRecognizer.get() setDelegate:nil];
> -    [_textSelectionAssistant release];
> +    [_longPressGestureRecognizer.get() setDelegate:nil];
> +    [_twoFingerPanGestureRecognizer.get() setDelegate:nil];

I believe you can remove the .get() now.

> Source/WebKit2/UIProcess/API/ios/WKInteractionView.mm:380
> +    // don't allow the highlight to be prevented by a selection gesture. press-and-hold on a link should highlight the link, not select it

Missing uppercase and period to form sentences.

> Source/WebKit2/UIProcess/API/ios/WKInteractionView.mm:389
> +#define IS_SAME_PAIR(a,b,x,y) ((((a) == (x)) && ((b) == (y))) || (((b) == (x)) && ((a) == (y))))

Let's use a static inline function instead of a Macro, that way we get type checking.

> Source/WebKit2/UIProcess/API/ios/WKInteractionView.mm:419
> +    if (_positionInfo.clickableElementName == String("IMG"))
> +        return @selector(_showImageSheet);
> +    else if (_positionInfo.clickableElementName == String("A"))

String -> ASCIILiteral.

> Source/WebKit2/UIProcess/API/ios/WKInteractionView.mm:437
> +            // If the assisted node is the same return NO.

"return NO" -> ", prevent the default gesture from starting."

> Source/WebKit2/UIProcess/API/ios/WKInteractionView.mm:438
> +            _page->getPositionInformation(IntPoint(point), _positionInfo);

I would prefer roundedIntPoint(point).

> Source/WebKit2/UIProcess/API/ios/WKInteractionView.mm:449
> +            // Return NO is there is no node.
> +            // Return YES if it is a node that wants highlight or if there is an action for it.

I think instead of Return NO/YES, you should say the desired action. The reader can see the return value bellow but it is hard to know why you are preventing the gesture from starting.

> Source/WebKit2/UIProcess/API/ios/WKInteractionView.mm:459
> +            _page->requestPositionInformation(IntPoint(point));

I would prefer roundedIntPoint(point).

> Source/WebKit2/UIProcess/API/ios/WKInteractionView.mm:471
> +        if (!_hasValidPositionInfo) {
> +            _page->getPositionInformation(IntPoint(point), _positionInfo);
> +            _hasValidPositionInfo = YES;
> +        }

Use ensurePositionInfoIsUpToDate instead?

> Source/WebKit2/UIProcess/API/ios/WKInteractionView.mm:485
> +        // FIXME: Add implementation

notImplemented().

> Source/WebKit2/UIProcess/API/ios/WKInteractionView.mm:509
> +    return _positionInfo.clickableElementName != String("img") && _positionInfo.clickableElementName != String("a") && !_positionInfo.selectionRects.isEmpty();

String -> ASCIILiteral.

> Source/WebKit2/UIProcess/API/ios/WKInteractionView.mm:553
> +        case UIGestureRecognizerStateBegan:
> +            // FIXME: add implementation
> +            break;
> +        case UIGestureRecognizerStateEnded:
> +            // FIXME: add implementation
> +            break;
> +        case UIGestureRecognizerStateCancelled:
> +            // FIXME: add implementation
> +            break;
> +        default:
> +            break;

Indent.
Comment 3 Sam Weinig 2014-01-14 18:24:41 PST
Comment on attachment 221212 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=221212&action=review

> Source/WebKit2/Shared/PositionInformation.h:37
> +struct PositionInformation {

This name seems a bit generic to me.

> Source/WebKit2/Shared/PositionInformation.h:45
> +    bool isSameAssistedNode;

Is same as what?
Comment 4 Enrica Casucci 2014-01-15 13:00:41 PST
Created attachment 221300 [details]
Patch2
Comment 5 Benjamin Poulain 2014-01-15 15:05:17 PST
Comment on attachment 221300 [details]
Patch2

View in context: https://bugs.webkit.org/attachment.cgi?id=221300&action=review

> Source/WebKit2/ChangeLog:4
> +        Support WebSelections in WK2 on iOS.
> +        https://bugs.webkit.org/show_bug.cgi?id=127015

Awesome changelog!

> Source/WebKit2/ChangeLog:11
> +        gesture recognizers are enabled and empty stubs for the gestures

and add empty stubs?

> Source/WebKit2/ChangeLog:37
> +            We are then able tp make an informed decision about whether

Typo: tp -> to

> Source/WebKit2/Shared/PositionInformation.h:37
> +struct PositionInformation {

InteractionInformationAtPosition maybe?

> Source/WebKit2/Shared/PositionInformation.h:40
> +        , wantsHighlight(true)

Shouldn't this be false by default?

Is it still needed?

> Source/WebKit2/Shared/PositionInformation.h:44
> +    WebCore::IntPoint point;

Maybe we should pass a floating point and round in the WebProcess?
I have the dream some day we will handle input on floating points position :)

(Not important for landing this).

> Source/WebKit2/Shared/PositionInformation.h:45
> +    bool isSameAssistedNode;

Maybe "nodeAtPositionIsSameAssistedNode"

> Source/WebKit2/UIProcess/API/ios/WKInteractionView.mm:39
> +#import <UIKit/UIGestureRecognizer_Private.h>
>  #import <UIKit/UIFont_Private.h>

Include order.

> Source/WebKit2/UIProcess/API/ios/WKInteractionView.mm:424
> +    if (_positionInformation.clickableElementName == String(ASCIILiteral("IMG")))
> +        return @selector(_showImageSheet);
> +    else if (_positionInformation.clickableElementName == String(ASCIILiteral("A")))
> +        return @selector(_showLinkSheet);

Did you need the String() with ASCIILiteral()? If so, I should fix operator==.

> Source/WebKit2/WebProcess/WebPage/ios/WebPageIOS.mm:817
> +        const HTMLElement* element = toHTMLElement(hitNode);
> +        if (!element)

Is this right? IIRC, toHTMLElement will just static_cast. On the other hand, hitNode is already check for nullity above, and could be a SVN element.

> Source/WebKit2/WebProcess/WebPage/ios/WebPageIOS.mm:834
> +        if (element->hasLocalName(HTMLNames::appletTag) || element->hasLocalName(HTMLNames::embedTag) || element->hasLocalName(HTMLNames::objectTag))
> +            info.wantsHighlight = false;
> +        else if (element->renderer() && element->renderer()->isImage()) {
> +            URL url = toRenderImage(element->renderer())->cachedImage()->url();
> +            if (!url.string().isNull()) {
> +                info.wantsHighlight = true;
> +                info.url = url.string();
> +            }
> +        } else if (element->isLink()) {
> +            info.wantsHighlight = true;
> +            info.url = element->getAttribute(HTMLNames::hrefAttr).string();
> +        } else if (isHTMLSelectElement(hitNode))
> +            info.wantsHighlight = true;
> +        else
> +            info.wantsHighlight = hitNode->isContentEditable();

Why not set wantsHighlight to yet in all of those cases?

> Source/WebKit2/WebProcess/WebPage/ios/WebPageIOS.mm:853
> +

You can remove the empty line.
Comment 6 Enrica Casucci 2014-01-15 16:49:18 PST
Part one landed 162103.
Comment 7 Enrica Casucci 2014-01-17 09:24:49 PST
Created attachment 221474 [details]
Build fix
Comment 8 Enrica Casucci 2014-01-17 09:30:08 PST
Landed build fix 162204.
Comment 9 Enrica Casucci 2014-01-21 14:25:54 PST
Created attachment 221787 [details]
Patch3
Comment 10 Benjamin Poulain 2014-01-22 18:08:03 PST
Comment on attachment 221787 [details]
Patch3

View in context: https://bugs.webkit.org/attachment.cgi?id=221787&action=review

> Source/WebKit2/Shared/ios/WKGestureTypes.h:57
>      WKSelectionTouchEnded,
>      WKSelectionTouchEndedMovingForward,
>      WKSelectionTouchEndedMovingBackward,
> -    WKSelectionTouchEndedNotMoving
> +    WKSelectionTouchEndedNotMoving,
> +    WKWebSelectionTouchStarted,
> +    WKWebSelectionTouchMoved,
> +    WKWebSelectionTouchEnded,

What about prefixing with WKEditableContentSelection and WKNonEditableContentSelection?

The UIKit names we use are pretty bad.

> Source/WebKit2/UIProcess/API/ios/WKInteractionView.mm:712
> +    BOOL hasWebSelection = _webSelectionAssistant != nil && !CGRectIsEmpty(_webSelectionAssistant.get().selectionFrame);

_webSelectionAssistant != nil -> _webSelectionAssistant or !!_webSelectionAssistant

> Source/WebKit2/WebProcess/WebPage/ios/WebPageIOS.mm:386
> +    IntPoint constrainedPoint = (m_assistedNode) ? constrainPoint(point, &frame, m_assistedNode.get()) : point;

No need for the parenthesis around m_assistedNode.
Comment 11 Enrica Casucci 2014-01-22 18:26:51 PST
Created attachment 221933 [details]
Patch4
Comment 12 Enrica Casucci 2014-02-10 18:01:28 PST
Created attachment 223781 [details]
block selection part1
Comment 13 Benjamin Poulain 2014-02-10 19:39:52 PST
Comment on attachment 223781 [details]
block selection part1

View in context: https://bugs.webkit.org/attachment.cgi?id=223781&action=review

> Source/WebKit2/Shared/ios/WKGestureTypes.h:77
> +typedef enum {
> +    WKNone = 0,
> +    WKWordIsNearTap = 1,
> +    WKIsBlockSelection = 2
> +} WKSelectionFlags;
> +

If this is only used by Objective-C++ and C++, you could use a typed enum instead.

If those values are exclusive, I would use the name SelectionType instead of SelectionFlags.

> Source/WebKit2/UIProcess/API/ios/WKInteractionView.mm:1077
> +    {

The bracket should be on the previous line.

> Source/WebKit2/WebProcess/WebPage/WebPage.h:1068
> +    const int blockSelectionStartWidth = 100;
> +    const int blockSelectionStartHeight = 100;

Does this have to be defined on the header? Could it be moved to the implementation?

> Source/WebKit2/WebProcess/WebPage/ios/WebPageIOS.mm:430
> +        if (!range)
> +            return range;

I would use "return nullptr". The code might be a little misleading otherwise.

> Source/WebKit2/WebProcess/WebPage/ios/WebPageIOS.mm:438
> +            if (boundingRect.isEmpty())

You could use instead "i == 0", which can be optimized by the compiler.

> Source/WebKit2/WebProcess/WebPage/ios/WebPageIOS.mm:443
> +        if (boundingRect.width() > m_blockSelectionDesiredSize.width() && boundingRect.height() > m_blockSelectionDesiredSize.height())

The same condition is repeated bellow. I think a nice little static inline function naming this would be good.

> Source/WebKit2/WebProcess/WebPage/ios/WebPageIOS.mm:450
> +    if (!currentNode->isElementNode())
> +        currentNode = currentNode->parentElement();

Now this is confusing me.
If we had a text node above, we now take its parent element? Is there a missing return?
Comment 14 Benjamin Poulain 2014-02-10 19:40:39 PST
This looks sane to me. Maybe Ryosuke should double check the selection code?
Comment 15 Enrica Casucci 2014-02-10 21:58:52 PST
Comment on attachment 223781 [details]
block selection part1

View in context: https://bugs.webkit.org/attachment.cgi?id=223781&action=review

>> Source/WebKit2/Shared/ios/WKGestureTypes.h:77
>> +
> 
> If this is only used by Objective-C++ and C++, you could use a typed enum instead.
> 
> If those values are exclusive, I would use the name SelectionType instead of SelectionFlags.

For now they are, but the idea was that when adding more they could be combined. All the other enums in this file are not. I'll change them all at once in another patch.

>> Source/WebKit2/WebProcess/WebPage/WebPage.h:1068
>> +    const int blockSelectionStartHeight = 100;
> 
> Does this have to be defined on the header? Could it be moved to the implementation?

I'll do that.

>> Source/WebKit2/WebProcess/WebPage/ios/WebPageIOS.mm:450
>> +        currentNode = currentNode->parentElement();
> 
> Now this is confusing me.
> If we had a text node above, we now take its parent element? Is there a missing return?

Yes, we had a text node, but it did not meet the size requirements. We are then looking for the enclosing block to see if we want to turn it into a block selection.
Comment 16 Enrica Casucci 2014-02-20 17:15:58 PST
Created attachment 224810 [details]
block selection part2
Comment 17 WebKit Commit Bot 2014-02-20 17:17:13 PST
Attachment 224810 [details] did not pass style-queue:


ERROR: Source/WebKit2/WebProcess/WebPage/WebPage.h:441:  The parameter name "point" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/WebKit2/WebProcess/WebPage/WebPage.h:743:  The parameter name "handlePosition" adds no information, so it should be removed.  [readability/parameter_name] [5]
Total errors found: 2 in 18 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 18 Ryosuke Niwa 2014-02-20 18:54:21 PST
Comment on attachment 224810 [details]
block selection part2

View in context: https://bugs.webkit.org/attachment.cgi?id=224810&action=review

> Source/WebKit2/WebProcess/WebPage/ios/WebPageIOS.mm:748
> +    return ((widthRatio > minMagnitudeRatio && xOriginShiftRatio < maxDisplacementRatio) &&
> +            (heightRatio > minMagnitudeRatio && yOriginShiftRatio < maxDisplacementRatio));

Nit: && should be at the beginning of the second line.
Also, it should be indented by 4 spaces instead of being aligned.

> Source/WebKit2/WebProcess/WebPage/ios/WebPageIOS.mm:758
> +    return (first->commonAncestorContainer(ec)->ownerDocument() == second->commonAncestorContainer(ec)->ownerDocument() &&
> +            first->compareBoundaryPoints(Range::START_TO_START, second, ASSERT_NO_EXCEPTION) <= 0 &&
> +            first->compareBoundaryPoints(Range::END_TO_END, second, ASSERT_NO_EXCEPTION) >= 0);

Ditto.

> Source/WebKit2/WebProcess/WebPage/ios/WebPageIOS.mm:991
> +        ExceptionCode ec;
> +        Node* node = currentRange->commonAncestorContainer(ec);

We should probably use ASSERT_NO_EXCEPTION here (probably always has it as the default argument).
Comment 19 Benjamin Poulain 2014-02-20 19:02:56 PST
Comment on attachment 224810 [details]
block selection part2

View in context: https://bugs.webkit.org/attachment.cgi?id=224810&action=review

Quick first round.

> Source/WebKit2/WebProcess/WebPage/ios/WebPageIOS.mm:500
> +    IntRect boundingRect;

This looks unused.

> Source/WebKit2/WebProcess/WebPage/ios/WebPageIOS.mm:501
> +    ExceptionCode ec;

ASSERT_NO_EXCEPTION or IGNORE_EXCEPTION instead?

> Source/WebKit2/WebProcess/WebPage/ios/WebPageIOS.mm:629
> +        if (static_cast<WKGestureRecognizerState>(gestureState) == WKGestureRecognizerStateEnded && flags & WKIsBlockSelection)

Maybe time to remove all the static_cast and do the conversion at the beginning of the function?

> Source/WebKit2/WebProcess/WebPage/ios/WebPageIOS.mm:721
> +static const int MaxHitTests = 10;

The first letter of the constant is uppercase.

> Source/WebKit2/WebProcess/WebPage/ios/WebPageIOS.mm:753
> +    ExceptionCode ec;

IGNORE_EXCEPTION or ASSERT_NO_EXCEPTION instead?

> Source/WebKit2/WebProcess/WebPage/ios/WebPageIOS.mm:793
> +    static const CGFloat maxDistance = 1000;

CGFloat -> float.

> Source/WebKit2/WebProcess/WebPage/ios/WebPageIOS.mm:836
> +        BOOL better = !(rectsEssentiallyTheSame(copyRect, currentBox, .05));

BOOL -> bool.

The name "better" does not provide enough information about the variable.

> Source/WebKit2/WebProcess/WebPage/ios/WebPageIOS.mm:846
> +                case WKHandleTop:
> +                case WKHandleBottom:
> +                    better = (copyRect.height() > currentBox.height());
> +                    break;
> +                case WKHandleRight:
> +                case WKHandleLeft:
> +                    better = (copyRect.width() > currentBox.width());
> +                    break;

Style.

> Source/WebKit2/WebProcess/WebPage/ios/WebPageIOS.mm:871
> +        distance *= multiple;
> +        distance = ceilf(distance);

distance = ceilf(distance * multiple);

> Source/WebKit2/WebProcess/WebPage/ios/WebPageIOS.mm:893
> +    float distance = 1;
> +    RefPtr<Range> best;
> +    IntRect bestRect;

We should reduce the declaration-use span of those variables.

> Source/WebKit2/WebProcess/WebPage/ios/WebPageIOS.mm:907
> +    const CGFloat multiple = powf(maxDistance - 1, 1.0/(MaxHitTests - 1));

Looks like this should be In a function, it is used in two places.

CGFloat -> float.

> Source/WebKit2/WebProcess/WebPage/ios/WebPageIOS.mm:990
> +        ExceptionCode ec;

You should use IGNORE_EXCEPTION instead.
Comment 20 Enrica Casucci 2014-02-21 09:46:22 PST
Created attachment 224878 [details]
Block selection part2

Thank you Ryosuke and Ben for the feedback. Here is the new patch that addresses your comments.
Comment 21 WebKit Commit Bot 2014-02-21 09:49:01 PST
Attachment 224878 [details] did not pass style-queue:


ERROR: Source/WebKit2/WebProcess/WebPage/WebPage.h:441:  The parameter name "point" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/WebKit2/WebProcess/WebPage/WebPage.h:743:  The parameter name "handlePosition" adds no information, so it should be removed.  [readability/parameter_name] [5]
Total errors found: 2 in 18 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 22 Enrica Casucci 2014-02-21 09:53:44 PST
Created attachment 224880 [details]
Block selection part2

Fixes the style issue and removes the redundant exports.
Comment 23 Benjamin Poulain 2014-02-21 12:51:58 PST
Comment on attachment 224880 [details]
Block selection part2

View in context: https://bugs.webkit.org/attachment.cgi?id=224880&action=review

> Source/WebKit2/UIProcess/WebPageProxy.h:461
> +    void updateBlockSelectionWithTouches(const WebCore::IntPoint, uint32_t touches, uint32_t handlePosition);

WithTouches -> WithTouch?

> Source/WebKit2/WebProcess/WebPage/ios/WebPageIOS.mm:716
> +static const int maxHitTests = 10;

If you make a single template (see comment bellow), this constant could simply be in the common function itself.

> Source/WebKit2/WebProcess/WebPage/ios/WebPageIOS.mm:772
> +        return IntPoint(box.x() + box.width() / 2, box.y());

x + width -> maxX?

> Source/WebKit2/WebProcess/WebPage/ios/WebPageIOS.mm:774
> +        return IntPoint(box.maxX(), box.y() + box.height() / 2);

maxY

> Source/WebKit2/WebProcess/WebPage/ios/WebPageIOS.mm:776
> +        return IntPoint(box.x() + box.width() / 2, box.maxY());

ditto

> Source/WebKit2/WebProcess/WebPage/ios/WebPageIOS.mm:778
> +        return IntPoint(box.x(), box.y() + box.height() / 2);

ditto

> Source/WebKit2/WebProcess/WebPage/ios/WebPageIOS.mm:791
> +    RefPtr<Range> best;

bestRange?

> Source/WebKit2/WebProcess/WebPage/ios/WebPageIOS.mm:873
> +PassRefPtr<Range> WebPage::contractedRangeFromHandle(Range* currentRange, WKHandlePosition handlePosition, WKSelectionFlags& flags)

Any way to make expandedRangeFromHandle and contractedRangeFromHandle client of a single template handling the loop?

> Source/WebKit2/WebProcess/WebPage/ios/WebPageIOS.mm:875
> +    // Shrinking with a base and extent will always give better results.  If we only have a single element,

Two spaces after the period.

> Source/WebKit2/WebProcess/WebPage/ios/WebPageIOS.mm:876
> +    // see if we can break that down to a base and extent.  Shrinking base and extent is comparatively straightforward.

Ditto.

> Source/WebKit2/WebProcess/WebPage/ios/WebPageIOS.mm:899
> +    RefPtr<Range> best;

bestRange?

> Source/WebKit2/WebProcess/WebPage/ios/WebPageIOS.mm:987
> +        if (renderer && renderer->childrenInline() && (renderer->isRenderBlock() && toRenderBlock(renderer)->inlineElementContinuation() == nil) && !renderer->isTable()) {

Instead of == nil, it should use ! on the value.

> Source/WebKit2/WebProcess/WebPage/ios/WebPageIOS.mm:998
> +    RefPtr<Range> currentRange = m_currentBlockSelection ? m_currentBlockSelection.get() : frame.selection().selection().toNormalizedRange();

Why are m_currentBlockSelection and frame.selection().selection() different?

> Source/WebKit2/WebProcess/WebPage/ios/WebPageIOS.mm:1010
> +    float current, expanded, contracted;

Need update to match WebKit's style.

> Source/WebKit2/WebProcess/WebPage/ios/WebPageIOS.mm:1105
> +

Blank line.

> Source/WebKit2/WebProcess/WebPage/ios/WebPageIOS.mm:1108
> +

Empty line.

> Source/WebKit2/WebProcess/WebPage/ios/WebPageIOS.mm:1416
> +            info.bounds = hitNode->renderer()->absoluteBoundingBoxRect(true);

useTransform is the default, you could drop the argument here.
Comment 24 Enrica Casucci 2014-02-21 13:19:07 PST
Thanks for the review

> 
> > Source/WebKit2/WebProcess/WebPage/ios/WebPageIOS.mm:772
> > +        return IntPoint(box.x() + box.width() / 2, box.y());
> 
> x + width -> maxX?
No, it is x + width/2. It is true for all the other cases you pointed out.

> > Source/WebKit2/WebProcess/WebPage/ios/WebPageIOS.mm:873
> > +PassRefPtr<Range> WebPage::contractedRangeFromHandle(Range* currentRange, WKHandlePosition handlePosition, WKSelectionFlags& flags)
> 
> Any way to make expandedRangeFromHandle and contractedRangeFromHandle client of a single template handling the loop?
Yes, I'll do it in another patch. I'll add a fix me.

> 
> Why are m_currentBlockSelection and frame.selection().selection() different?

Because once you set a range as Selection it gets normalized.
Comment 25 Enrica Casucci 2014-02-21 13:51:40 PST
Committed revision 164497.