Bug 108667 – [Chromium] WebWidget should expose a way to determine the start/end of the selection bounds

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

[Chris Hopman]

[Chromium] WebWidget should expose a way to determine the start/end of the selection bounds

[Chris Hopman]

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.

[Chris Hopman]

Created attachment 186088 [details]
Patch

[WebKit Review Bot]

Please wait for approval from [e-mail], [e-mail], [e-mail], [e-mail] or [e-mail] before submitting, as this patch contains changes to the Chromium public API. See also https://trac.webkit.org/wiki/ChromiumWebKitAPI.

[Tien-Ren Chen]

LGTM. Please make sure you have talked to fishd@ and varunjain@ that they are happy with the solution.

[Build Bot]

Comment on attachment 186088 [details]
Patch

Attachment 186088 [details] did not pass win-ews (win):
Output: http://queues.webkit.org/results/16342060

[Darin Fisher (:fishd, Google)]

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.

[Chris Hopman]

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.

[James Robinson]

A selection can cross frame boundaries, can't it?

[Chris Hopman]

(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).

[Chris Hopman]

Created attachment 186484 [details]
Patch

[Chris Hopman]

(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.

[Darin Fisher (:fishd, Google)]

Thanks for working on moving these methods to WebFrame.  I'm interested in taking a look at that patch of course :-D

[WebKit Review Bot]

Comment on attachment 186484 [details]
Patch

Clearing flags on attachment: 186484

Committed r142011: <http://trac.webkit.org/changeset/142011>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.