Add high DPI assets and update defaults for CrOS find in page bar.

Add high DPI assets and update defaults for CrOS find in page bar.

The new theme has a pushed image for the find next and find prev buttons, requires the find in page text boxes to have a transparent background as the theme draws a partially transparent text box area which partially shows the theme panel color through. Also we use the tab close button for the find in page bar.

BUG=149032
TEST=Run chrome with --force-device-scale-factor=2, find in page bar is high DPI.

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=157358

[flackr, 2012-09-13 20:44:51]

Hi,

This adds the new find bar resources. PTAL, thanks!

[oshima, 2012-09-13 21:49:50]

lgtm

[flackr, 2012-09-13 22:22:49]

+sky for owners.

[sky, 2012-09-14 02:02:51]

If this only adds resources, why do you need to touch the find bar files?

[flackr, 2012-09-14 02:40:18]

On 2012/09/14 02:02:51, sky wrote:
> If this only adds resources, why do you need to touch the find bar files?

I had to make the background color of the find in page text box transparent as
the background in the theme is partially transparent to partially show the find
bar color through. I renamed the disabled find in page next/prev buttons from
IDR_FINDINPAGE_(NEXT|PREV)_P to IDR_FINDINPAGE_(NEXT|PREV)_D so that I could use
the _P name for the pressed images which we didn't used to have (so these had to
be added). Also we now use the tab close button for the views find in page close
button.

[flackr, 2012-09-14 02:41:24]

On 2012/09/14 02:40:18, flackr wrote:
> On 2012/09/14 02:02:51, sky wrote:
> > If this only adds resources, why do you need to touch the find bar files?
> 
> I had to make the background color of the find in page text box transparent as
> the background in the theme is partially transparent to partially show the
find
> bar color through. I renamed the disabled find in page next/prev buttons from
> IDR_FINDINPAGE_(NEXT|PREV)_P to IDR_FINDINPAGE_(NEXT|PREV)_D so that I could
use
> the _P name for the pressed images which we didn't used to have (so these had
to
> be added). Also we now use the tab close button for the views find in page
close
> button.

I guess I should clarify that we also are updating the default resources.

[sky, 2012-09-14 16:10:36]

Windows doesn't support transparent child windows. Did you make sure
this works on on windows?

  -Scott

On Thu, Sep 13, 2012 at 7:40 PM,  <flackr@chromium.org> wrote:
> On 2012/09/14 02:02:51, sky wrote:
>>
>> If this only adds resources, why do you need to touch the find bar files?
>
>
> I had to make the background color of the find in page text box transparent
> as
> the background in the theme is partially transparent to partially show the
> find
> bar color through. I renamed the disabled find in page next/prev buttons
> from
> IDR_FINDINPAGE_(NEXT|PREV)_P to IDR_FINDINPAGE_(NEXT|PREV)_D so that I could
> use
> the _P name for the pressed images which we didn't used to have (so these
> had to
> be added). Also we now use the tab close button for the views find in page
> close
> button.
>
> http://codereview.chromium.org/10908234/

[flackr, 2012-09-14 16:49:33]

Hey, I had a build in progress last night, tried it this morning and you're
right the transparency isn't working correctly. Would it makes sense to land the
new resources as cros only?

On 2012/09/14 16:10:36, sky wrote:
> Windows doesn't support transparent child windows. Did you make sure
> this works on on windows?
> 
>   -Scott
> 
> On Thu, Sep 13, 2012 at 7:40 PM,  <mailto:flackr@chromium.org> wrote:
> > On 2012/09/14 02:02:51, sky wrote:
> >>
> >> If this only adds resources, why do you need to touch the find bar files?
> >
> >
> > I had to make the background color of the find in page text box transparent
> > as
> > the background in the theme is partially transparent to partially show the
> > find
> > bar color through. I renamed the disabled find in page next/prev buttons
> > from
> > IDR_FINDINPAGE_(NEXT|PREV)_P to IDR_FINDINPAGE_(NEXT|PREV)_D so that I could
> > use
> > the _P name for the pressed images which we didn't used to have (so these
> > had to
> > be added). Also we now use the tab close button for the views find in page
> > close
> > button.
> >
> > http://codereview.chromium.org/10908234/

[sky, 2012-09-14 17:21:06]

You'll have to talk with the ux folks. If there expectation is this
applies every where the transparency needs to be sorted out.

  -Scott

On Fri, Sep 14, 2012 at 9:49 AM,  <flackr@chromium.org> wrote:
> Hey, I had a build in progress last night, tried it this morning and you're
> right the transparency isn't working correctly. Would it makes sense to land
> the
> new resources as cros only?
>
> On 2012/09/14 16:10:36, sky wrote:
>>
>> Windows doesn't support transparent child windows. Did you make sure
>> this works on on windows?
>
>
>>    -Scott
>
>
>> On Thu, Sep 13, 2012 at 7:40 PM,  <mailto:flackr@chromium.org> wrote:
>> > On 2012/09/14 02:02:51, sky wrote:
>> >>
>> >> If this only adds resources, why do you need to touch the find bar
>> >> files?
>> >
>> >
>> > I had to make the background color of the find in page text box
>> > transparent
>> > as
>> > the background in the theme is partially transparent to partially show
>> > the
>> > find
>> > bar color through. I renamed the disabled find in page next/prev buttons
>> > from
>> > IDR_FINDINPAGE_(NEXT|PREV)_P to IDR_FINDINPAGE_(NEXT|PREV)_D so that I
>> > could
>> > use
>> > the _P name for the pressed images which we didn't used to have (so
>> > these
>> > had to
>> > be added). Also we now use the tab close button for the views find in
>> > page
>> > close
>> > button.
>> >
>> > http://codereview.chromium.org/10908234/
>
>
>
>
> http://codereview.chromium.org/10908234/

[flackr, 2012-09-14 20:11:28]

Hi,

As discussed, I've updated the patch to only use the new resources on cros which
supports the transparency. I've also tested on windows (touch and non touch) to
make sure the transparency change hasn't affected the appearance with the old
resources and that using the tab close button doesn't cause any layout issues.

Can you take another look?

Thanks.

On 2012/09/14 17:21:06, sky wrote:
> You'll have to talk with the ux folks. If there expectation is this
> applies every where the transparency needs to be sorted out.
> 
>   -Scott
> 
> On Fri, Sep 14, 2012 at 9:49 AM,  <mailto:flackr@chromium.org> wrote:
> > Hey, I had a build in progress last night, tried it this morning and you're
> > right the transparency isn't working correctly. Would it makes sense to land
> > the
> > new resources as cros only?
> >
> > On 2012/09/14 16:10:36, sky wrote:
> >>
> >> Windows doesn't support transparent child windows. Did you make sure
> >> this works on on windows?
> >
> >
> >>    -Scott
> >
> >
> >> On Thu, Sep 13, 2012 at 7:40 PM,  <mailto:flackr@chromium.org> wrote:
> >> > On 2012/09/14 02:02:51, sky wrote:
> >> >>
> >> >> If this only adds resources, why do you need to touch the find bar
> >> >> files?
> >> >
> >> >
> >> > I had to make the background color of the find in page text box
> >> > transparent
> >> > as
> >> > the background in the theme is partially transparent to partially show
> >> > the
> >> > find
> >> > bar color through. I renamed the disabled find in page next/prev buttons
> >> > from
> >> > IDR_FINDINPAGE_(NEXT|PREV)_P to IDR_FINDINPAGE_(NEXT|PREV)_D so that I
> >> > could
> >> > use
> >> > the _P name for the pressed images which we didn't used to have (so
> >> > these
> >> > had to
> >> > be added). Also we now use the tab close button for the views find in
> >> > page
> >> > close
> >> > button.
> >> >
> >> > http://codereview.chromium.org/10908234/
> >
> >
> >
> >
> > http://codereview.chromium.org/10908234/

[sky, 2012-09-14 21:57:30]

http://codereview.chromium.org/10908234/diff/3005/chrome/browser/ui/views/fin...
File chrome/browser/ui/views/find_bar_view.cc (right):

http://codereview.chromium.org/10908234/diff/3005/chrome/browser/ui/views/fin...
chrome/browser/ui/views/find_bar_view.cc:87:
find_text_->SetBackgroundColor(kFindBoxBackgroundColor);
Don't you only want this for chromeos?

[flackr, 2012-09-15 01:46:34]

http://codereview.chromium.org/10908234/diff/3005/chrome/browser/ui/views/fin...
File chrome/browser/ui/views/find_bar_view.cc (right):

http://codereview.chromium.org/10908234/diff/3005/chrome/browser/ui/views/fin...
chrome/browser/ui/views/find_bar_view.cc:87:
find_text_->SetBackgroundColor(kFindBoxBackgroundColor);
On 2012/09/14 21:57:30, sky wrote:
> Don't you only want this for chromeos?

It doesn't change the appearance on Windows since it's white with alpha
transparency which doesn't actually make the text box transparent. If you think
I should though I can put an #if defined on this.

[sky, 2012-09-16 17:37:23]

Yes, please with the ifdef.

On Fri, Sep 14, 2012 at 6:46 PM,  <flackr@chromium.org> wrote:
>
>
http://codereview.chromium.org/10908234/diff/3005/chrome/browser/ui/views/fin...
> File chrome/browser/ui/views/find_bar_view.cc (right):
>
>
http://codereview.chromium.org/10908234/diff/3005/chrome/browser/ui/views/fin...
> chrome/browser/ui/views/find_bar_view.cc:87:
> find_text_->SetBackgroundColor(kFindBoxBackgroundColor);
> On 2012/09/14 21:57:30, sky wrote:
>>
>> Don't you only want this for chromeos?
>
>
> It doesn't change the appearance on Windows since it's white with alpha
> transparency which doesn't actually make the text box transparent. If
> you think I should though I can put an #if defined on this.
>
> http://codereview.chromium.org/10908234/

[flackr, 2012-09-16 23:18:31]

Done.

On 2012/09/16 17:37:23, sky wrote:
> Yes, please with the ifdef.
> 
> On Fri, Sep 14, 2012 at 6:46 PM,  <mailto:flackr@chromium.org> wrote:
> >
> >
>
http://codereview.chromium.org/10908234/diff/3005/chrome/browser/ui/views/fin...
> > File chrome/browser/ui/views/find_bar_view.cc (right):
> >
> >
>
http://codereview.chromium.org/10908234/diff/3005/chrome/browser/ui/views/fin...
> > chrome/browser/ui/views/find_bar_view.cc:87:
> > find_text_->SetBackgroundColor(kFindBoxBackgroundColor);
> > On 2012/09/14 21:57:30, sky wrote:
> >>
> >> Don't you only want this for chromeos?
> >
> >
> > It doesn't change the appearance on Windows since it's white with alpha
> > transparency which doesn't actually make the text box transparent. If
> > you think I should though I can put an #if defined on this.
> >
> > http://codereview.chromium.org/10908234/

[sky, 2012-09-17 13:47:06]

http://codereview.chromium.org/10908234/diff/14007/chrome/browser/ui/views/fi...
File chrome/browser/ui/views/find_bar_view.cc (right):

http://codereview.chromium.org/10908234/diff/14007/chrome/browser/ui/views/fi...
chrome/browser/ui/views/find_bar_view.cc:150:
rb.GetImageSkiaNamed(IDR_TAB_CLOSE));
Sorry, one last question. Are you sure about this? At least on windows the find
bar close button is much different than the tab close button.

[flackr, 2012-09-17 14:02:27]

http://codereview.chromium.org/10908234/diff/14007/chrome/browser/ui/views/fi...
File chrome/browser/ui/views/find_bar_view.cc (right):

http://codereview.chromium.org/10908234/diff/14007/chrome/browser/ui/views/fi...
chrome/browser/ui/views/find_bar_view.cc:150:
rb.GetImageSkiaNamed(IDR_TAB_CLOSE));
On 2012/09/17 13:47:06, sky wrote:
> Sorry, one last question. Are you sure about this? At least on windows the
find
> bar close button is much different than the tab close button.

I believe Sebastien wanted the tab close button to be used on the find bar on
Windows too. Sebastien, can you comment on this?

[oshima, 2012-09-17 16:28:39]

just want to double check. are you sure png files you deleted in
touch_100_percent aren't used by metro build?

[flackr, 2012-09-17 16:30:39]

On 2012/09/17 16:28:39, oshima wrote:
> just want to double check. are you sure png files you deleted in
> touch_100_percent aren't used by metro build?

Yes, the old theme_resources.grd path pointed to the root folder, common/find_*
was in the source tree but unused.

[oshima, 2012-09-17 16:41:34]

thanks. resource lgtm

On Mon, Sep 17, 2012 at 9:30 AM, <flackr@chromium.org> wrote:

> On 2012/09/17 16:28:39, oshima wrote:
>
>> just want to double check. are you sure png files you deleted in
>> touch_100_percent aren't used by metro build?
>>
>
> Yes, the old theme_resources.grd path pointed to the root folder,
> common/find_*
> was in the source tree but unused.
>
>
http://codereview.chromium.**org/10908234/<http://codereview.chromium.org/109...
>

[flackr, 2012-09-17 18:34:34]

Sorry for no diff from previous version, git-cl upload is encountering errors
uploading previous versions of the file.

http://codereview.chromium.org/10908234/diff/14007/chrome/browser/ui/views/fi...
File chrome/browser/ui/views/find_bar_view.cc (right):

http://codereview.chromium.org/10908234/diff/14007/chrome/browser/ui/views/fi...
chrome/browser/ui/views/find_bar_view.cc:150:
rb.GetImageSkiaNamed(IDR_TAB_CLOSE));
On 2012/09/17 14:02:28, flackr wrote:
> On 2012/09/17 13:47:06, sky wrote:
> > Sorry, one last question. Are you sure about this? At least on windows the
> find
> > bar close button is much different than the tab close button.
> 
> I believe Sebastien wanted the tab close button to be used on the find bar on
> Windows too. Sebastien, can you comment on this?

Okay, talked to Sebastien and he wants to only use the tab close button when we
can use new resources, so I've put this behind #ifdef as well.

[sky, 2012-09-17 21:06:54]

LGTM