WebKit Bugzilla
New
Browse
Log In
×
Sign in with GitHub
or
Remember my login
Create Account
·
Forgot Password
Forgotten password account recovery
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
Details
Formatted Diff
Diff
Patch
(10.03 KB, patch)
2011-04-06 09:27 PDT
,
Varun Jain
no flags
Details
Formatted Diff
Diff
Patch
(10.04 KB, patch)
2011-04-07 15:56 PDT
,
Varun Jain
no flags
Details
Formatted Diff
Diff
Show Obsolete
(2)
View All
Add attachment
proposed patch, testcase, etc.
Varun Jain
Comment 1
2011-04-05 15:01:00 PDT
Created
attachment 88314
[details]
Patch
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
Created
attachment 88446
[details]
Patch
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
Created
attachment 88721
[details]
Patch
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.
Top of Page
Format For Printing
XML
Clone This Bug