Bug 69846 – [chromium] Add a setSelectionToRange() method to WebWidget.

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

[Nico Weber]

[chromium] Add a setSelectionOnFocusedFrame() method to WebWidget.

[Nico Weber]

Created attachment 110520 [details]
Patch

[Nico Weber]

Hi Darin,

a WebKit API review. The ChangeLog entry should tell you everything you need. I'd love to get this in early this week.

Nico

[WebKit Review Bot]

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

> Source/WebKit/chromium/src/WebViewImpl.cpp:1366
> +                                             int selectionEnd) {

Webkit style: put { on next line.

[Nico Weber]

Created attachment 110527 [details]
Patch

[Nico Weber]

> Webkit style: put { on next line.

Done, thanks.

[Darin Fisher (:fishd, Google)]

Comment on attachment 110527 [details]
Patch

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

> Source/WebKit/chromium/public/WebWidget.h:115
> +    virtual void setSelectionOnFocusedFrame(int selectionStart,

nit: WebWidget is not supposed to know about WebFrame.  You can use a WebWidget
that has no concept of frames.  Maybe you want to have this be a method on
WebView, which does know about frames?

next question:  perhaps you should just add a method to WebFrame.  then, use
view->focusedFrame()->setSelectionRange(int selectionStart, int selectionEnd).

what are the coordinates of selectionStart and selectionEnd?  should you be
using WebRange to express the range?  It looks like your implementation needs
to construct a WebCore::Range (which WebRange wraps).

i guess my overall suggestion is to consider exposing primitives here instead.

[Nico Weber]

(In reply to comment #7)
> (From update of attachment 110527 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=110527&action=review
> 
> > Source/WebKit/chromium/public/WebWidget.h:115
> > +    virtual void setSelectionOnFocusedFrame(int selectionStart,
> 
> nit: WebWidget is not supposed to know about WebFrame.  You can use a WebWidget
> that has no concept of frames.  Maybe you want to have this be a method on
> WebView, which does know about frames?
> 
> next question:  perhaps you should just add a method to WebFrame.  then, use
> view->focusedFrame()->setSelectionRange(int selectionStart, int selectionEnd).

Sounds good, will do.

> what are the coordinates of selectionStart and selectionEnd?  should you be
> using WebRange to express the range?  It looks like your implementation needs
> to construct a WebCore::Range (which WebRange wraps).

As far as I understand, WebRange is not constructible from user code (it has a comment saying "read only").

[Darin Fisher (:fishd, Google)]

(In reply to comment #8)
...
> As far as I understand, WebRange is not constructible from user code (it has a comment saying "read only").

That could be changed of course :)

[Darin Fisher (:fishd, Google)]

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

[Nico Weber]

Created attachment 110618 [details]
Patch

[Nico Weber]

Comment on attachment 110618 [details]
Patch

(not for review, just snapshotting)

[Nico Weber]

Created attachment 110621 [details]
Patch

[Nico Weber]

I didn't go the WebRange route because:

1.) This is now consistent with WebFrame::setMarkedText() (switched from start/end to start/length)
2.) Even if I added a WebRange::FromRange() factory method, WebFrame wouldn't have a good api to convert that back to a "normal" range, which is what WebFrame needs to do its thing. If you're fine with adding an `AsRange()` method to WebRange (only if WEBKIT_IMPLEMENTATION is defined), then I can use WebRange. Are you? (I guessed "no")

[Darin Fisher (:fishd, Google)]

Comment on attachment 110621 [details]
Patch

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

I suspect we could probably clean up all of these functions that express ranges.
The methods seem overly specific.  If we had the right primitives, we'd probably
be much happier.

R- because of the nit.

> Source/WebKit/chromium/src/WebFrameImpl.cpp:1100
> +PassRefPtr<Range> convertToRange(Frame* frame, unsigned start, unsigned length)

nit: this should be a 'static' function.  please move it up to the top of the file so it
does not interrupt the flow of WebFrameImpl methods.

[Nico Weber]

Created attachment 110720 [details]
Patch

[Darin Fisher (:fishd, Google)]

Comment on attachment 110720 [details]
Patch

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

> Source/WebKit/chromium/public/WebFrame.h:585
> +    virtual operator WTF::PassRefPtr<WebCore::Frame>() const = 0;

We normally cast WebFrame to WebFrameImpl to get at the WebCore::Frame.  There is only one implementation of WebFrame in our system.

> Source/WebKit/chromium/src/WebFrameImpl.cpp:1103
> +        RefPtr<Range> replacementRange = static_cast<PassRefPtr<WebCore::Range> >(range);

is the static_cast really necessary?  won't it be implicitly invoked?

did you try RefPtr<Range> replacementRange(range) ?

[Nico Weber]

Created attachment 110726 [details]
Patch

[Nico Weber]

(In reply to comment #17)
> (From update of attachment 110720 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=110720&action=review
> 
> > Source/WebKit/chromium/public/WebFrame.h:585
> > +    virtual operator WTF::PassRefPtr<WebCore::Frame>() const = 0;
> 
> We normally cast WebFrame to WebFrameImpl to get at the WebCore::Frame.  There is only one implementation of WebFrame in our system.

Done.

> > Source/WebKit/chromium/src/WebFrameImpl.cpp:1103
> > +        RefPtr<Range> replacementRange = static_cast<PassRefPtr<WebCore::Range> >(range);
> 
> is the static_cast really necessary?  won't it be implicitly invoked?

Seems to be necessary.

> did you try RefPtr<Range> replacementRange(range) ?

I tried it ( "A a = b;" is really just syntactic sugar for "A a(b);", so I would've been surprised had it made a difference.)

[Darin Fisher (:fishd, Google)]

Comment on attachment 110726 [details]
Patch

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

> Source/WebKit/chromium/src/WebFrameImpl.cpp:1103
> +        RefPtr<Range> replacementRange = static_cast<PassRefPtr<WebCore::Range> >(range);

Ah, what we do elsewhere is this:

  RefPtr<Range> replacementRange = PassRefPtr<Range>(range);

[Nico Weber]

Created attachment 110733 [details]
Patch

[Nico Weber]

> Ah, what we do elsewhere is this:
> 
>   RefPtr<Range> replacementRange = PassRefPtr<Range>(range);

Done.

[Nico Weber]

Created attachment 110735 [details]
Patch

[Darin Fisher (:fishd, Google)]

Comment on attachment 110735 [details]
Patch

Thanks for all the revisions!

[Nico Weber]

Comment on attachment 110735 [details]
Patch

Thanks!

[WebKit Review Bot]

Comment on attachment 110735 [details]
Patch

Clearing flags on attachment: 110735

Committed r97301: <http://trac.webkit.org/changeset/97301>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.