Bug 69028 – [chromium] Add a selectionBounds() method to WebWidget.

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Peng Huang]

Add getSubstringFromRange() and modify selectionRange() for chromium

[Peng Huang]

Created attachment 109076 [details]
Patch

[James Su]

I'm not authorized to add comment to the patch. Just paste my comment here:

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

And it's probably reasonable to rename WebWidget::caretOrSelectionRange() to selectionRange() to match the name of other similar methods.
I'm not qualified for reviewing the actual implementation, please find a webkit reviewer to help review that part.

> Source/WebKit/chromium/public/WebWidget.h:148
>      virtual bool getSelectionOffsetsAndTextInEditableContent(WebString&, size_t& focus, size_t& anchor) const { return false; }

Will this method be deprecated in favor of getSubstringFromRange()? If yes, then I suggest to add a comment and then remove it in an upcoming CL.

> Source/WebKit/chromium/public/WebWidget.h:155
>      virtual WebRect caretOrSelectionBounds() { return WebRect(); }

ditto.

> Source/WebKit/chromium/public/WebWidget.h:-159
> -    virtual bool selectionRange(WebPoint& start, WebPoint& end) const { return false; }

You cannot remove this method in this CL, otherwise you will break chrome code. Removing this method in an upcoming CL after landing this CL and migrating chrome to use to new methods.
So just add a comment in this CL stating that this method has been deprecated.

[Peng Huang]

Created attachment 109191 [details]
Patch

[Peng Huang]

Hi Darin, Could you please review this patch? Thanks.

[Peng Huang]

Created attachment 110547 [details]
Patch

[WebKit Review Bot]

Please wait for approval from [e-mail] before submitting because this patch contains changes to the Chromium public API.

[Darin Fisher (:fishd, Google)]

Comment on attachment 110547 [details]
Patch

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

> Source/WebKit/chromium/public/WebWidget.h:154
> +    virtual WebString getSubstringFromRange(size_t location, size_t length) const { return WebString(); }

shouldn't this just be a method on WebRange?  it seems like we should be using WebRange
for more of these range related APIs.  we should also be providing ways to get a WebRange
or construct a WebRange.

[Darin Fisher (:fishd, Google)]

I gave similar feedback in https://bugs.webkit.org/show_bug.cgi?id=69846

[Peng Huang]

Comment on attachment 110547 [details]
Patch

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

>> Source/WebKit/chromium/public/WebWidget.h:154
>> +    virtual WebString getSubstringFromRange(size_t location, size_t length) const { return WebString(); }
> 
> shouldn't this just be a method on WebRange?  it seems like we should be using WebRange
> for more of these range related APIs.  we should also be providing ways to get a WebRange
> or construct a WebRange.

Did you mean add a function `WebRange WebView::getRange(size_t location, size_t length)' here?
Or add a constructor WebRange::WebRange(WebFrame *frame, size_t location, size_t length).

For adding WebRange::WebRange, WebRange can not get enough information from WebFrame for creating a new WebRange. I think we need a function to convert WebFrame back to WebCore::Frame. Is it OK? What's you suggestion?

[Peng Huang]

Created attachment 110751 [details]
Patch

[Darin Fisher (:fishd, Google)]

Comment on attachment 110751 [details]
Patch

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

Otherwise, this patch looks fine to me.  R=me.

> Source/WebKit/chromium/src/WebViewImpl.h:123
> +    // TODO(penghuang): It has been replaced by selectionBounds. Remove it when

nit: we use FIXME in WebKit instead of TODO(user)

[Peng Huang]

Created attachment 110784 [details]
Patch

[Darin Fisher (:fishd, Google)]

Comment on attachment 110784 [details]
Patch

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

still R=me otherwise.

> Source/WebKit/chromium/src/WebViewImpl.cpp:1573
> +      return false;

nit: should be 4 space indent.  sorry for not catching this earlier :(

[Peng Huang]

Created attachment 110842 [details]
Patch

[Peng Huang]

Fixed. Sorry for the mistake.

[WebKit Review Bot]

Comment on attachment 110842 [details]
Patch

Clearing flags on attachment: 110842

Committed r97368: <http://trac.webkit.org/changeset/97368>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.