Add getSubstringFromRange() and modify selectionRange() for chromium
Created attachment 109076 [details] Patch
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.
Created attachment 109191 [details] Patch
Hi Darin, Could you please review this patch? Thanks.
Created attachment 110547 [details] Patch
Please wait for approval from fishd@chromium.org before submitting because this patch contains changes to the Chromium public API.
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.
I gave similar feedback in https://bugs.webkit.org/show_bug.cgi?id=69846
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?
Created attachment 110751 [details] Patch
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)
Created attachment 110784 [details] Patch
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 :(
Created attachment 110842 [details] Patch
Fixed. Sorry for the mistake.
Comment on attachment 110842 [details] Patch Clearing flags on attachment: 110842 Committed r97368: <http://trac.webkit.org/changeset/97368>
All reviewed patches have been landed. Closing bug.