[Chromium] WebWidget should expose a way to determine the start/end of the selection bounds
WebWidget::selectionBounds() returns the anchor and focus of the selection. This matches the arguments to WebFrame::selectRange(). However, sometimes (/often) a caller wants to know the start/end of the selection rather than anchor/focus. Currently, this information is inaccessible. There was previous suggestion to have selectionBounds() return start/end but I think that matching WebFrame::selectRange() is valuable and selectRange has to take anchor/focus. Instead, we should just expose whether or not the selection anchor is the start or end and then a caller can derive start/end from anchor/focus.
Created attachment 186088 [details] Patch
Please wait for approval from abarth@webkit.org, dglazkov@chromium.org, fishd@chromium.org, jamesr@chromium.org or tkent@chromium.org before submitting, as this patch contains changes to the Chromium public API. See also https://trac.webkit.org/wiki/ChromiumWebKitAPI.
LGTM. Please make sure you have talked to fishd@ and varunjain@ that they are happy with the solution.
Comment on attachment 186088 [details] Patch Attachment 186088 [details] did not pass win-ews (win): Output: http://queues.webkit.org/results/16342060
Comment on attachment 186088 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=186088&action=review > Source/WebKit/chromium/src/WebViewImpl.cpp:2401 > +bool WebViewImpl::isSelectionAnchorFirst() const nit: isSelectionAnchorFirst is declared after selectionTextDirection. please define functions in the same order that you declare them. > Source/WebKit/chromium/src/WebViewImpl.cpp:2403 > + const Frame* frame = focusedWebCoreFrame(); I apologize for randomizing you further, but it seems like we might be making a mistake by putting all of these selection APIs on WebWidget. It seems like all of them act on the focused frame, so perhaps we should really be putting selection methods on WebFrame instead. Then, if it is important for selectRange and selectionBounds to be in-sync, it'll be more natural because they will be defined adjacent to one another. WDYT? The thing to know about WebWidget is that it shouldn't know anything about document or frame or any DOM concepts. It is just a rendering surface. Originally, selection stuff ended up on WebWidget because selection as a concept was just a rectangular region of the widget, but maybe in hindsight that was a poor choice.
Comment on attachment 186088 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=186088&action=review >> Source/WebKit/chromium/src/WebViewImpl.cpp:2403 >> + const Frame* frame = focusedWebCoreFrame(); > > I apologize for randomizing you further, but it seems like we might be > making a mistake by putting all of these selection APIs on WebWidget. > It seems like all of them act on the focused frame, so perhaps we > should really be putting selection methods on WebFrame instead. Then, > if it is important for selectRange and selectionBounds to be in-sync, > it'll be more natural because they will be defined adjacent to one > another. WDYT? > > The thing to know about WebWidget is that it shouldn't know anything > about document or frame or any DOM concepts. It is just a rendering > surface. Originally, selection stuff ended up on WebWidget because > selection as a concept was just a rectangular region of the widget, > but maybe in hindsight that was a poor choice. I noticed that WebWidget was an odd place for these methods. It looks to me that both the selection and the composition methods could/should be moved to WebFrame. There are also four selection/composition methods on WebView (curiously listed under GPU acceleration support in WebView.h) that should probably also be moved then. I can move the selection APIs to WebFrame first, then add this.
A selection can cross frame boundaries, can't it?
(In reply to comment #8) > A selection can cross frame boundaries, can't it? I don't know, fishd can probably answer that. I do know that all of these functions operate only on the focused frame's selection and editor (::selectionBounds uses the webview for contentsToWindow).
Created attachment 186484 [details] Patch
(In reply to comment #10) > Created an attachment (id=186484) [details] > Patch I've addressed only fishd@'s nit, not the movement of these APIs to WebFrame. I have a patch ready to move all these APIs to WebFrame if we decide to do that, but while we wait for agreement on that we should be able to land this change as it won't make it any harder to move the APIs if we decide that's the way to go.
Thanks for working on moving these methods to WebFrame. I'm interested in taking a look at that patch of course :-D
Comment on attachment 186484 [details] Patch Clearing flags on attachment: 186484 Committed r142011: <http://trac.webkit.org/changeset/142011>
All reviewed patches have been landed. Closing bug.