RESOLVED FIXED 57888
Need to extend WebKit chromium API to access text selection
https://bugs.webkit.org/show_bug.cgi?id=57888
Summary Need to extend WebKit chromium API to access text selection
Varun Jain
Reported 2011-04-05 14:59:38 PDT
Need to extend WebKit chromium API to access text selection
Attachments
Patch (10.30 KB, patch)
2011-04-05 15:01 PDT, Varun Jain
no flags
Patch (10.03 KB, patch)
2011-04-06 09:27 PDT, Varun Jain
no flags
Patch (10.04 KB, patch)
2011-04-07 15:56 PDT, Varun Jain
no flags
Varun Jain
Comment 1 2011-04-05 15:01:00 PDT
Darin Fisher (:fishd, Google)
Comment 2 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?
Varun Jain
Comment 3 2011-04-06 09:27:46 PDT
Varun Jain
Comment 4 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.
Darin Fisher (:fishd, Google)
Comment 5 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?
Varun Jain
Comment 6 2011-04-07 15:56:48 PDT
Varun Jain
Comment 7 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
Darin Fisher (:fishd, Google)
Comment 8 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.
WebKit Commit Bot
Comment 9 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>
WebKit Commit Bot
Comment 10 2011-04-08 11:53:42 PDT
All reviewed patches have been landed. Closing bug.
WebKit Review Bot
Comment 11 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
Note You need to log in before you can comment on or make changes to this bug.