alternate ntp: focus border of omnibox in ntp

alternate ntp: focus border of omnibox in ntp

- another attempt at drawing the ntp omnibox blue focus border, as a filled rounded rect with a mask, on top of the standard omnibox border
- fix to remove the white outline around the omnibox border
- fix the left and right edges of omnibox border to the same thickness and brightness as those of the top and bottom edges

BUG=135466, 150871
TEST=verify per bug rpt


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

[dhollowa, 2012-09-20 18:33:13]

https://chromiumcodereview.appspot.com/10957010/diff/2001/chrome/app/theme/th...
File chrome/app/theme/theme_resources.grd (right):

https://chromiumcodereview.appspot.com/10957010/diff/2001/chrome/app/theme/th...
chrome/app/theme/theme_resources.grd:437: <structure type="chrome_scaled_image"
name="IDR_LOCATION_BAR_SEARCH_FOCUS_BORDER_MASK"
file="common/browser_omnibox_search_focus_border_mask.png" />
You'll check this in separately then?

https://chromiumcodereview.appspot.com/10957010/diff/2001/chrome/browser/ui/v...
File chrome/browser/ui/views/location_bar/location_bar_view.cc (right):

https://chromiumcodereview.appspot.com/10957010/diff/2001/chrome/browser/ui/v...
chrome/browser/ui/views/location_bar/location_bar_view.cc:428: // If mode
transitions from |NTP|, repaint to not draw focus border.
This would be simpler if you just always schedule a paint here.  Then in
|OnPaint| lazily create the background if/when you need it.

https://chromiumcodereview.appspot.com/10957010/diff/2001/chrome/browser/ui/v...
File chrome/browser/ui/views/location_bar/location_bar_view.h (right):

https://chromiumcodereview.appspot.com/10957010/diff/2001/chrome/browser/ui/v...
chrome/browser/ui/views/location_bar/location_bar_view.h:489:
scoped_ptr<views::Painter> painter_;
Might be worth renaming this to disambiguate.  Maybe |background_painter_|?

[kuan, 2012-09-20 18:55:50]

i've addressed all comments in patch set 3.  ptal.  thx.

https://chromiumcodereview.appspot.com/10957010/diff/2001/chrome/app/theme/th...
File chrome/app/theme/theme_resources.grd (right):

https://chromiumcodereview.appspot.com/10957010/diff/2001/chrome/app/theme/th...
chrome/app/theme/theme_resources.grd:437: <structure type="chrome_scaled_image"
name="IDR_LOCATION_BAR_SEARCH_FOCUS_BORDER_MASK"
file="common/browser_omnibox_search_focus_border_mask.png" />
On 2012/09/20 18:33:13, dhollowa wrote:
> You'll check this in separately then?

yes, i'll land the images (1x and 2x) when this cl is lgtm'ed b4 sending it to
cq.

https://chromiumcodereview.appspot.com/10957010/diff/2001/chrome/browser/ui/v...
File chrome/browser/ui/views/location_bar/location_bar_view.cc (right):

https://chromiumcodereview.appspot.com/10957010/diff/2001/chrome/browser/ui/v...
chrome/browser/ui/views/location_bar/location_bar_view.cc:428: // If mode
transitions from |NTP|, repaint to not draw focus border.
On 2012/09/20 18:33:13, dhollowa wrote:
> This would be simpler if you just always schedule a paint here.  Then in
> |OnPaint| lazily create the background if/when you need it.

Done.

https://chromiumcodereview.appspot.com/10957010/diff/2001/chrome/browser/ui/v...
File chrome/browser/ui/views/location_bar/location_bar_view.h (right):

https://chromiumcodereview.appspot.com/10957010/diff/2001/chrome/browser/ui/v...
chrome/browser/ui/views/location_bar/location_bar_view.h:489:
scoped_ptr<views::Painter> painter_;
On 2012/09/20 18:33:13, dhollowa wrote:
> Might be worth renaming this to disambiguate.  Maybe |background_painter_|?

Done.

[dhollowa, 2012-09-20 19:05:11]

LGTM w/ nits.

https://chromiumcodereview.appspot.com/10957010/diff/7001/chrome/browser/ui/v...
File chrome/browser/ui/views/location_bar/location_bar_view.cc (right):

https://chromiumcodereview.appspot.com/10957010/diff/7001/chrome/browser/ui/v...
chrome/browser/ui/views/location_bar/location_bar_view.cc:1019: if
(!search_focus_painter_.get())
This should be a DCHECK.  No return.

https://chromiumcodereview.appspot.com/10957010/diff/7001/chrome/browser/ui/v...
chrome/browser/ui/views/location_bar/location_bar_view.cc:1038: gfx::ImageSkia
focuse_image =
nit: s/focuse_image/focus_image/

[kuan, 2012-09-20 19:47:21]

i've addressed all comments in patch set 4.  ptal.  thx.

https://chromiumcodereview.appspot.com/10957010/diff/7001/chrome/browser/ui/v...
File chrome/browser/ui/views/location_bar/location_bar_view.cc (right):

https://chromiumcodereview.appspot.com/10957010/diff/7001/chrome/browser/ui/v...
chrome/browser/ui/views/location_bar/location_bar_view.cc:1019: if
(!search_focus_painter_.get())
On 2012/09/20 19:05:11, dhollowa wrote:
> This should be a DCHECK.  No return.

Done.

https://chromiumcodereview.appspot.com/10957010/diff/7001/chrome/browser/ui/v...
chrome/browser/ui/views/location_bar/location_bar_view.cc:1038: gfx::ImageSkia
focuse_image =
On 2012/09/20 19:05:11, dhollowa wrote:
> nit: s/focuse_image/focus_image/

Done.

[sky, 2012-09-20 19:50:29]

https://chromiumcodereview.appspot.com/10957010/diff/1005/chrome/browser/ui/v...
File chrome/browser/ui/views/location_bar/location_bar_view.cc (right):

https://chromiumcodereview.appspot.com/10957010/diff/1005/chrome/browser/ui/v...
chrome/browser/ui/views/location_bar/location_bar_view.cc:429: SchedulePaint();
Can we only do this if necessary?

https://chromiumcodereview.appspot.com/10957010/diff/1005/chrome/browser/ui/v...
chrome/browser/ui/views/location_bar/location_bar_view.cc:1009: if
(is_search_ntp) {
Move this into its own function.

[kuan, 2012-09-20 20:12:46]

i've addressed all comments in patch set 5.  ptal.  thx.

https://chromiumcodereview.appspot.com/10957010/diff/1005/chrome/browser/ui/v...
File chrome/browser/ui/views/location_bar/location_bar_view.cc (right):

https://chromiumcodereview.appspot.com/10957010/diff/1005/chrome/browser/ui/v...
chrome/browser/ui/views/location_bar/location_bar_view.cc:429: SchedulePaint();
On 2012/09/20 19:50:29, sky wrote:
> Can we only do this if necessary?

Done.

https://chromiumcodereview.appspot.com/10957010/diff/1005/chrome/browser/ui/v...
chrome/browser/ui/views/location_bar/location_bar_view.cc:1009: if
(is_search_ntp) {
On 2012/09/20 19:50:29, sky wrote:
> Move this into its own function.

Done.

[sky, 2012-09-20 20:38:47]

LGTM

[commit-bot: I haz the power, 2012-09-20 21:30:15]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/kuan@chromium.org/10957010/11004

[commit-bot: I haz the power, 2012-09-21 00:08:33]

Change committed as 157876