Bug 69846 - [chromium] Add a setSelectionToRange() method to WebWidget.
Summary: [chromium] Add a setSelectionToRange() method to WebWidget.
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Nico Weber
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2011-10-11 08:59 PDT by Nico Weber
Modified: 2011-10-12 14:09 PDT (History)
4 users (show)

See Also:


Attachments
Patch (4.72 KB, patch)
2011-10-11 09:03 PDT, Nico Weber
no flags Details | Formatted Diff | Diff
Patch (4.72 KB, patch)
2011-10-11 09:50 PDT, Nico Weber
no flags Details | Formatted Diff | Diff
Patch (4.52 KB, patch)
2011-10-11 17:49 PDT, Nico Weber
no flags Details | Formatted Diff | Diff
Patch (4.57 KB, patch)
2011-10-11 18:11 PDT, Nico Weber
no flags Details | Formatted Diff | Diff
Patch (6.97 KB, patch)
2011-10-12 12:33 PDT, Nico Weber
no flags Details | Formatted Diff | Diff
Patch (5.83 KB, patch)
2011-10-12 13:00 PDT, Nico Weber
no flags Details | Formatted Diff | Diff
Patch (5.81 KB, patch)
2011-10-12 13:14 PDT, Nico Weber
no flags Details | Formatted Diff | Diff
Patch (5.98 KB, patch)
2011-10-12 13:23 PDT, Nico Weber
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Nico Weber 2011-10-11 08:59:46 PDT
[chromium] Add a setSelectionOnFocusedFrame() method to WebWidget.
Comment 1 Nico Weber 2011-10-11 09:03:41 PDT
Created attachment 110520 [details]
Patch
Comment 2 Nico Weber 2011-10-11 09:04:22 PDT
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
Comment 3 WebKit Review Bot 2011-10-11 09:06:04 PDT
Please wait for approval from fishd@chromium.org before submitting because this patch contains changes to the Chromium public API.
Comment 4 asvitkine 2011-10-11 09:31:12 PDT
> Source/WebKit/chromium/src/WebViewImpl.cpp:1366
> +                                             int selectionEnd) {

Webkit style: put { on next line.
Comment 5 Nico Weber 2011-10-11 09:50:11 PDT
Created attachment 110527 [details]
Patch
Comment 6 Nico Weber 2011-10-11 09:52:57 PDT
> Webkit style: put { on next line.

Done, thanks.
Comment 7 Darin Fisher (:fishd, Google) 2011-10-11 13:01:06 PDT
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.
Comment 8 Nico Weber 2011-10-11 13:07:32 PDT
(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").
Comment 9 Darin Fisher (:fishd, Google) 2011-10-11 14:00:00 PDT
(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 :)
Comment 10 Darin Fisher (:fishd, Google) 2011-10-11 15:56:50 PDT
I gave similar feedback in https://bugs.webkit.org/show_bug.cgi?id=69028
Comment 11 Nico Weber 2011-10-11 17:49:40 PDT
Created attachment 110618 [details]
Patch
Comment 12 Nico Weber 2011-10-11 17:50:07 PDT
Comment on attachment 110618 [details]
Patch

(not for review, just snapshotting)
Comment 13 Nico Weber 2011-10-11 18:11:04 PDT
Created attachment 110621 [details]
Patch
Comment 14 Nico Weber 2011-10-11 18:13:35 PDT
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")
Comment 15 Darin Fisher (:fishd, Google) 2011-10-12 09:43:04 PDT
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.
Comment 16 Nico Weber 2011-10-12 12:33:32 PDT
Created attachment 110720 [details]
Patch
Comment 17 Darin Fisher (:fishd, Google) 2011-10-12 12:44:21 PDT
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) ?
Comment 18 Nico Weber 2011-10-12 13:00:59 PDT
Created attachment 110726 [details]
Patch
Comment 19 Nico Weber 2011-10-12 13:01:16 PDT
(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.)
Comment 20 Darin Fisher (:fishd, Google) 2011-10-12 13:04:03 PDT
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);
Comment 21 Nico Weber 2011-10-12 13:14:29 PDT
Created attachment 110733 [details]
Patch
Comment 22 Nico Weber 2011-10-12 13:14:50 PDT
> Ah, what we do elsewhere is this:
> 
>   RefPtr<Range> replacementRange = PassRefPtr<Range>(range);

Done.
Comment 23 Nico Weber 2011-10-12 13:23:10 PDT
Created attachment 110735 [details]
Patch
Comment 24 Darin Fisher (:fishd, Google) 2011-10-12 13:51:09 PDT
Comment on attachment 110735 [details]
Patch

Thanks for all the revisions!
Comment 25 Nico Weber 2011-10-12 13:53:23 PDT
Comment on attachment 110735 [details]
Patch

Thanks!
Comment 26 WebKit Review Bot 2011-10-12 14:09:23 PDT
Comment on attachment 110735 [details]
Patch

Clearing flags on attachment: 110735

Committed r97301: <http://trac.webkit.org/changeset/97301>
Comment 27 WebKit Review Bot 2011-10-12 14:09:28 PDT
All reviewed patches have been landed.  Closing bug.