[webkit-reviews] review denied: [Bug 34915] Chromium: Need to be able to get the bounds of selection rectangle(s) : [Attachment 49029] Changed patch to address reviewer feedback.
bugzilla-daemon at webkit.org
bugzilla-daemon at webkit.org
Fri Feb 19 11:27:31 PST 2010
Darin Fisher (:fishd, Google) <fishd at chromium.org> has denied Dmitriy Belenko
<dbelenko at google.com>'s request for review:
Bug 34915: Chromium: Need to be able to get the bounds of selection
rectangle(s)
https://bugs.webkit.org/show_bug.cgi?id=34915
Attachment 49029: Changed patch to address reviewer feedback.
https://bugs.webkit.org/attachment.cgi?id=49029&action=review
------- Additional Comments from Darin Fisher (:fishd, Google)
<fishd at chromium.org>
> diff --git a/WebKit/chromium/ChangeLog b/WebKit/chromium/ChangeLog
> index 4e8a629..0d90fd0 100644
> --- a/WebKit/chromium/ChangeLog
> +++ b/WebKit/chromium/ChangeLog
> @@ -1,3 +1,20 @@
> +2010-02-18 Dmitriy Belenko <dbelenko at google.com>
> +
> + Reviewed by NOBODY (OOPS!).
> +
> + This change will enable about 30 test cases to pass in Chromium.
> + All of these test cases are related to selection rect boundaries.
> + The three issues that this will solve can be found here:
> +
> + http://code.google.com/p/chromium/issues/detail?id=28646
> + http://code.google.com/p/chromium/issues/detail?id=23481
> + http://code.google.com/p/chromium/issues/detail?id=23482
I'm sorry to nit-pick, but you should really just reference the WebKit
bug number in the WebKit ChangeLog. (The bug can have references to these
Chromium bug reports.)
> +++ b/WebKit/chromium/public/WebFrame.h
> @@ -492,6 +492,11 @@ public:
> float pageWidthInPixels,
> float pageHeightInPixels) const =
0;
>
> + // Returns the bounds rect for current selection. If selection is
performed
> + // on transformed text, the rect will still bound the selection, but
will
> + // not be transformed itself. If no selection is present the rect will
be
> + // empty ((0,0), (0,0))
> + virtual WebRect selectionBoundsRect() const = 0;
> protected:
nit: please preserve the new line above the "protected:" line.
> +++ b/WebKit/chromium/src/WebFrameImpl.cpp
...
> +WebRect WebFrameImpl::selectionBoundsRect() const
> +{
> + if (hasSelection()) {
> + FloatRect fr = frame()->selectionBounds(false);
> + return WebRect(roundf(fr.x()), roundf(fr.y()),
> + roundf(fr.width()), roundf(fr.height()));
Are you sure we want to round like this? IntRect(const FloatRect&) just rounds
down.
> + }
> +
> + return WebRect(0, 0, 0, 0);
nit: Just do "return WebRect();" since the default constructor does the same
thing.
More information about the webkit-reviews
mailing list