Adjusted action box menu position relative to omnibox and action box menu button.

Adjusted action box menu position relative to omnibox and action box menu button.

BUG=147287
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=156958

[Peter Kasting, 2012-09-14 22:21:38]

http://codereview.chromium.org/10905296/diff/1/chrome/browser/ui/views/action...
File chrome/browser/ui/views/action_box_menu.cc (right):

http://codereview.chromium.org/10905296/diff/1/chrome/browser/ui/views/action...
chrome/browser/ui/views/action_box_menu.cc:47:
screen_location.Offset(menu_offset.width(), menu_offset.height());
If you pass in the offset as a point rather than a size, you can just do
ConvertPointToScreen() directly on it.

http://codereview.chromium.org/10905296/diff/1/chrome/browser/ui/views/locati...
File chrome/browser/ui/views/location_bar/action_box_button_view.h (right):

http://codereview.chromium.org/10905296/diff/1/chrome/browser/ui/views/locati...
chrome/browser/ui/views/location_bar/action_box_button_view.h:23: void
SetMenuOffset(gfx::Size menu_offset) {
Provide this as a constructor arg rather than using a separate setter.

[yefimt, 2012-09-14 22:46:35]

http://codereview.chromium.org/10905296/diff/1/chrome/browser/ui/views/action...
File chrome/browser/ui/views/action_box_menu.cc (right):

http://codereview.chromium.org/10905296/diff/1/chrome/browser/ui/views/action...
chrome/browser/ui/views/action_box_menu.cc:47:
screen_location.Offset(menu_offset.width(), menu_offset.height());
On 2012/09/14 22:21:38, Peter Kasting wrote:
> If you pass in the offset as a point rather than a size, you can just do
> ConvertPointToScreen() directly on it.

Done.

http://codereview.chromium.org/10905296/diff/1/chrome/browser/ui/views/locati...
File chrome/browser/ui/views/location_bar/action_box_button_view.h (right):

http://codereview.chromium.org/10905296/diff/1/chrome/browser/ui/views/locati...
chrome/browser/ui/views/location_bar/action_box_button_view.h:23: void
SetMenuOffset(gfx::Size menu_offset) {
On 2012/09/14 22:21:38, Peter Kasting wrote:
> Provide this as a constructor arg rather than using a separate setter.

Done.

[Peter Kasting, 2012-09-14 22:50:53]

LGTM

http://codereview.chromium.org/10905296/diff/8/chrome/browser/ui/views/action...
File chrome/browser/ui/views/action_box_menu.cc (right):

http://codereview.chromium.org/10905296/diff/8/chrome/browser/ui/views/action...
chrome/browser/ui/views/action_box_menu.cc:45: gfx::Point
screen_location(menu_offset);
Nit: Omit this line, you're passing by value precisely so that you can modify
|menu_offset| directly

http://codereview.chromium.org/10905296/diff/8/chrome/browser/ui/views/locati...
File chrome/browser/ui/views/location_bar/action_box_button_view.h (right):

http://codereview.chromium.org/10905296/diff/8/chrome/browser/ui/views/locati...
chrome/browser/ui/views/location_bar/action_box_button_view.h:17: explicit
ActionBoxButtonView(Browser* browser, gfx::Point menu_offset);
Nit: Pass by const ref

http://codereview.chromium.org/10905296/diff/8/chrome/browser/ui/views/locati...
File chrome/browser/ui/views/location_bar/location_bar_view.cc (right):

http://codereview.chromium.org/10905296/diff/8/chrome/browser/ui/views/locati...
chrome/browser/ui/views/location_bar/location_bar_view.cc:314: gfx::Point
menu_offset(
Nit: I would probably inline this into the next statement but it's not too
important

[yefimt, 2012-09-14 23:00:56]

http://codereview.chromium.org/10905296/diff/8/chrome/browser/ui/views/action...
File chrome/browser/ui/views/action_box_menu.cc (right):

http://codereview.chromium.org/10905296/diff/8/chrome/browser/ui/views/action...
chrome/browser/ui/views/action_box_menu.cc:45: gfx::Point
screen_location(menu_offset);
On 2012/09/14 22:50:53, Peter Kasting wrote:
> Nit: Omit this line, you're passing by value precisely so that you can modify
> |menu_offset| directly

Done.

http://codereview.chromium.org/10905296/diff/8/chrome/browser/ui/views/locati...
File chrome/browser/ui/views/location_bar/action_box_button_view.h (right):

http://codereview.chromium.org/10905296/diff/8/chrome/browser/ui/views/locati...
chrome/browser/ui/views/location_bar/action_box_button_view.h:17: explicit
ActionBoxButtonView(Browser* browser, gfx::Point menu_offset);
On 2012/09/14 22:50:53, Peter Kasting wrote:
> Nit: Pass by const ref

Done.

http://codereview.chromium.org/10905296/diff/8/chrome/browser/ui/views/locati...
File chrome/browser/ui/views/location_bar/location_bar_view.cc (right):

http://codereview.chromium.org/10905296/diff/8/chrome/browser/ui/views/locati...
chrome/browser/ui/views/location_bar/location_bar_view.cc:314: gfx::Point
menu_offset(
I always felt that N one line expressions are more readable then one N line
expression :)

On 2012/09/14 22:50:53, Peter Kasting wrote:
> Nit: I would probably inline this into the next statement but it's not too
> important

[commit-bot: I haz the power, 2012-09-14 23:05:53]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/yefim@chromium.org/10905296/6002

[commit-bot: I haz the power, 2012-09-14 23:43:15]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/yefim@chromium.org/10905296/6002

[commit-bot: I haz the power, 2012-09-15 01:51:13]

Change committed as 156958