Bug 57888

Summary: Need to extend WebKit chromium API to access text selection
Product: WebKit Reporter: Varun Jain <varunjain>
Component: New BugsAssignee: Nobody <webkit-unassigned>
Status: RESOLVED FIXED    
Severity: Normal CC: abarth, commit-queue, eric, fishd, levin, webkit.review.bot
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Other   
OS: OS X 10.5   
Attachments:
Description Flags
Patch
none
Patch
none
Patch none

Description Varun Jain 2011-04-05 14:59:38 PDT
Need to extend WebKit chromium API to access text selection
Comment 1 Varun Jain 2011-04-05 15:01:00 PDT
Created attachment 88314 [details]
Patch
Comment 2 Darin Fisher (:fishd, Google) 2011-04-05 21:55:26 PDT
Comment on attachment 88314 [details]
Patch

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

> Source/WebKit/chromium/public/WebFrame.h:412
> +    virtual void selectBetweenWindowPoints(const WebPoint& startPoint,

would it make sense to call this "selectRange"?

also, in WebWidget.h, you just refer to these parameters using the names
"start" and "end".  maybe you should use the same names here as you do there
for consistency?

> Source/WebKit/chromium/public/WebWidget.h:130
> +    // Returns the start and point for the current selection, aligned to the

"start and [end] point" ??

> Source/WebKit/chromium/public/WebWidget.h:132
> +    virtual bool selectionStartAndEndPoints(WebPoint& start,

would it make sense to call this selectionRange?
Comment 3 Varun Jain 2011-04-06 09:27:46 PDT
Created attachment 88446 [details]
Patch
Comment 4 Varun Jain 2011-04-06 09:28:52 PDT
(In reply to comment #2)
> (From update of attachment 88314 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=88314&action=review
> 
> > Source/WebKit/chromium/public/WebFrame.h:412
> > +    virtual void selectBetweenWindowPoints(const WebPoint& startPoint,
> 
> would it make sense to call this "selectRange"?

Done.

> 
> also, in WebWidget.h, you just refer to these parameters using the names
> "start" and "end".  maybe you should use the same names here as you do there
> for consistency?
> 
> > Source/WebKit/chromium/public/WebWidget.h:130
> > +    // Returns the start and point for the current selection, aligned to the
> 
> "start and [end] point" ??

Done.
> 
> > Source/WebKit/chromium/public/WebWidget.h:132
> > +    virtual bool selectionStartAndEndPoints(WebPoint& start,
> 
> would it make sense to call this selectionRange?

Done.
Comment 5 Darin Fisher (:fishd, Google) 2011-04-07 14:13:31 PDT
Comment on attachment 88446 [details]
Patch

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

> Source/WebKit/chromium/public/WebWidget.h:132
> +    // TODO(varunjain): make this pure virtual after all downstream classes have

nit: we use FIXME instead of TODO(user) in webkit

there should be no downstream implementations of this interface.  why is this needed?
Comment 6 Varun Jain 2011-04-07 15:56:48 PDT
Created attachment 88721 [details]
Patch
Comment 7 Varun Jain 2011-04-07 15:58:29 PDT
(In reply to comment #5)
> (From update of attachment 88446 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=88446&action=review
> 
> > Source/WebKit/chromium/public/WebWidget.h:132
> > +    // TODO(varunjain): make this pure virtual after all downstream classes have
> 
> nit: we use FIXME instead of TODO(user) in webkit

Done

> 
> there should be no downstream implementations of this interface.  why is this needed?

I dont know how or why but someone put an implementation of WebWidget in content/renderer/render_widget_fullscreen_pepper.cc :(
so I break chromium if I make that method pure virtual
thats why I need to do the fix in 3 parts... my current CL + chrome CL to add the method in that implementation + webkitCL to make the method pure virtual
Comment 8 Darin Fisher (:fishd, Google) 2011-04-08 09:57:08 PDT
(In reply to comment #7)
> > there should be no downstream implementations of this interface.  why is this needed?
> 
> I dont know how or why but someone put an implementation of WebWidget in content/renderer/render_widget_fullscreen_pepper.cc :(
> so I break chromium if I make that method pure virtual
> thats why I need to do the fix in 3 parts... my current CL + chrome CL to add the method in that implementation + webkitCL to make the method pure virtual

OK, we should bring this up with piman@chromium.org, and then discuss options.  For now, you are doing the right thing.
Comment 9 WebKit Commit Bot 2011-04-08 11:53:36 PDT
Comment on attachment 88721 [details]
Patch

Clearing flags on attachment: 88721

Committed r83320: <http://trac.webkit.org/changeset/83320>
Comment 10 WebKit Commit Bot 2011-04-08 11:53:42 PDT
All reviewed patches have been landed.  Closing bug.
Comment 11 WebKit Review Bot 2011-04-08 12:47:24 PDT
http://trac.webkit.org/changeset/83320 might have broken Qt Linux Release
The following tests are not passing:
http/tests/loading/preload-append-scan.php