Bug 57888 – Need to extend WebKit chromium API to access text selection

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

[Varun Jain]

Need to extend WebKit chromium API to access text selection

[Varun Jain]

Created attachment 88314 [details]
Patch

[Darin Fisher (:fishd, Google)]

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?

[Varun Jain]

Created attachment 88446 [details]
Patch

[Varun Jain]

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

[Darin Fisher (:fishd, Google)]

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?

[Varun Jain]

Created attachment 88721 [details]
Patch

[Varun Jain]

(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

[Darin Fisher (:fishd, Google)]

(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 [e-mail], and then discuss options.  For now, you are doing the right thing.

[WebKit Commit Bot]

Comment on attachment 88721 [details]
Patch

Clearing flags on attachment: 88721

Committed r83320: <http://trac.webkit.org/changeset/83320>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[WebKit Review Bot]

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