Cleanup adding BoundsInWidget to View to convert callers...

Cleanup adding BoundsInWidget to View to convert callers
from ConvertToWidget to use this instead

BUG=5133
(http://crbug.com/5133)

[Peter Kasting, 2008-12-07 02:16:31]

Are there places besides these that can use this function?  It seems strange to
add a function that creates and returns a Rect, only to have all the callers use
it like a Point.  I wonder if Ben wasn't looking for a broader refactoring here.

http://codereview.chromium.org/13235/diff/13/18
File chrome/browser/views/find_bar_win.cc (right):

http://codereview.chromium.org/13235/diff/13/18#newcode490
Line 490: gfx::Rect topleft = toolbar->BoundsInWidget();
Nit: A Rect should not be named "topleft".  That's a name for a point.

http://codereview.chromium.org/13235/diff/13/15
File chrome/views/hwnd_view.cc (right):

http://codereview.chromium.org/13235/diff/13/15#newcode73
Line 73: gfx::Rect top_left = BoundsInWidget();
Nit: Same comment

http://codereview.chromium.org/13235/diff/13/14
File chrome/views/tooltip_manager.cc (right):

http://codereview.chromium.org/13235/diff/13/14#newcode387
Line 387: gfx::Rect relative_point_coordinates = focused_view->BoundsInWidget();
Nit: Same comment

http://codereview.chromium.org/13235/diff/13/17
File chrome/views/view.h (right):

http://codereview.chromium.org/13235/diff/13/17#newcode658
Line 658: // A common pattern to convert bounds from the current view Widget.
This comment tells me almost nothing about the function.  Please make this
clearer.

[Mohamed Mansour (USE mhm), 2008-12-07 06:37:52]

I hope that comment is good.. And yes, I was wondering the same thing why all
the callers using it as a Point while its a Rect. Waiting on Ben's
clarification.

http://codereview.chromium.org/13235/diff/13/18
File chrome/browser/views/find_bar_win.cc (right):

http://codereview.chromium.org/13235/diff/13/18#newcode490
Line 490: gfx::Rect topleft = toolbar->BoundsInWidget();
On 2008/12/07 02:16:31, pkasting wrote:
> Nit: A Rect should not be named "topleft".  That's a name for a point.

Done.

http://codereview.chromium.org/13235/diff/13/15
File chrome/views/hwnd_view.cc (right):

http://codereview.chromium.org/13235/diff/13/15#newcode73
Line 73: gfx::Rect top_left = BoundsInWidget();
On 2008/12/07 02:16:31, pkasting wrote:
> Nit: Same comment

Done.

http://codereview.chromium.org/13235/diff/13/14
File chrome/views/tooltip_manager.cc (right):

http://codereview.chromium.org/13235/diff/13/14#newcode387
Line 387: gfx::Rect relative_point_coordinates = focused_view->BoundsInWidget();
On 2008/12/07 02:16:31, pkasting wrote:
> Nit: Same comment

Done.

http://codereview.chromium.org/13235/diff/13/17
File chrome/views/view.h (right):

http://codereview.chromium.org/13235/diff/13/17#newcode658
Line 658: // A common pattern to convert bounds from the current view Widget.
On 2008/12/07 02:16:31, pkasting wrote:
> This comment tells me almost nothing about the function.  Please make this
> clearer.

Done.

[Ben Goodger (Google), 2008-12-08 20:40:08]

http://codereview.chromium.org/13235/diff/409/31
File chrome/views/view.h (right):

http://codereview.chromium.org/13235/diff/409/31#newcode660
Line 660: gfx::Rect BoundsInWidget();
Yeah I agree with Peter now after some reflection, I think this would be better
as

gfx::Point PositionInWidget();

[Mohamed Mansour (USE mhm), 2008-12-08 23:47:50]

So now I gotta roll back everything? Change back the naming from widget_bounds
to top_left, beh ... here I go again :x

[Mohamed Mansour (USE mhm), 2008-12-09 00:03:59]

But now, since we just need a Point, the method becomes this:
gfx::Point View::PositionInWidget() {
  gfx::Point origin;
  ConvertPointToWidget(this, &origin);
  return origin;
}

Since from the old code bounds_in_widget.set_origin(origin);, is what x() and
y() are derived from.

Is this needed now?

[Peter Kasting, 2008-12-09 00:06:45]

On 2008/12/09 00:03:59, Mohamed Mansour wrote:
> But now, since we just need a Point, the method becomes this:
> gfx::Point View::PositionInWidget() {
>   gfx::Point origin;
>   ConvertPointToWidget(this, &origin);
>   return origin;
> }

I agree that this doesn't seem like a huge win to me.  Maybe Ben differs.

[Peter Kasting, 2008-12-09 00:50:32]

Closing per beng.