[chromium] Add a setSelectionOnFocusedFrame() method to WebWidget.
Created attachment 110520 [details] Patch
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
Please wait for approval from fishd@chromium.org 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.
Created attachment 110527 [details] Patch
> Webkit style: put { on next line. Done, thanks.
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.
(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").
(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 :)
I gave similar feedback in https://bugs.webkit.org/show_bug.cgi?id=69028
Created attachment 110618 [details] Patch
Comment on attachment 110618 [details] Patch (not for review, just snapshotting)
Created attachment 110621 [details] Patch
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 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.
Created attachment 110720 [details] Patch
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) ?
Created attachment 110726 [details] Patch
(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 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);
Created attachment 110733 [details] Patch
> Ah, what we do elsewhere is this: > > RefPtr<Range> replacementRange = PassRefPtr<Range>(range); Done.
Created attachment 110735 [details] Patch
Comment on attachment 110735 [details] Patch Thanks for all the revisions!
Comment on attachment 110735 [details] Patch Thanks!
Comment on attachment 110735 [details] Patch Clearing flags on attachment: 110735 Committed r97301: <http://trac.webkit.org/changeset/97301>
All reviewed patches have been landed. Closing bug.