Update use of OTR avatars for touch. We now select

Update use of OTR avatars for touch.  We now select
IDR_OTR_ICON_FULLSCREEN when in touch layout and either fullscreen or
running in Metro.

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

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

[Jói, 2012-06-11 14:34:15]

Sky:  OWNERS approval.

Ben:  Please verify that what I'm doing on non-Windows (checking for touch
layout and whether BrowserView is fullscreen) looks good.  I'm not in a good
situation to test that config.

Cheers,
Jói

[sky, 2012-06-11 18:24:14]

LGTM

http://codereview.chromium.org/10538082/diff/1/chrome/browser/ui/views/frame/...
File chrome/browser/ui/views/frame/browser_view.h (right):

http://codereview.chromium.org/10538082/diff/1/chrome/browser/ui/views/frame/...
chrome/browser/ui/views/frame/browser_view.h:402: int GetOtrIconResourceID()
const;
Otr -> OTR

[benrg, 2012-06-11 20:35:45]

LGTM with the file name fixed.

http://codereview.chromium.org/10538082/diff/1/chrome/app/theme/theme_resourc...
File chrome/app/theme/theme_resources_standard.grd (right):

http://codereview.chromium.org/10538082/diff/1/chrome/app/theme/theme_resourc...
chrome/app/theme/theme_resources_standard.grd:160: <include
name="IDR_OTR_ICON_FULLSCREEN" file="default_100_percent/otr_icon.png"
type="BINDATA" />
This resource ID is never used in non-touch, so this is a dummy image and could
point to anything. It's going to be a problem when these files (finally) get
merged, since there will just be a single <structure> pointing to
otr_icon_fullscreen, which won't exist in default_[12]00_percent. Do you know
what the plan is for this situation?

(Also in theme_resources_2x.grd)

http://codereview.chromium.org/10538082/diff/1/chrome/app/theme/theme_resourc...
File chrome/app/theme/theme_resources_touch_2x.grd (right):

http://codereview.chromium.org/10538082/diff/1/chrome/app/theme/theme_resourc...
chrome/app/theme/theme_resources_touch_2x.grd:157: <include
name="IDR_OTR_ICON_FULLSCREEN" file="touch_200_percent/otr_icon.png"
type="BINDATA" />
Should be otr_icon_fullscreen.

http://codereview.chromium.org/10538082/diff/1/chrome/browser/ui/views/frame/...
File chrome/browser/ui/views/frame/browser_view.cc (right):

http://codereview.chromium.org/10538082/diff/1/chrome/browser/ui/views/frame/...
chrome/browser/ui/views/frame/browser_view.cc:1782: int
BrowserView::GetOtrIconResourceID() const {
I asked glen@ and he says this is correct.

Nit: you could save a line by dropping the variable and using three return
statements.

[Jói, 2012-06-11 22:53:42]

Will put this to the CQ now.  If we want to do 1x1 images for
otr_icon_fullscreen.png in non-touch layouts, I will do that as a follow-up. 
For now, the duplicate images should compress very nicely.

Cheers,
Jói

http://codereview.chromium.org/10538082/diff/1/chrome/app/theme/theme_resourc...
File chrome/app/theme/theme_resources_standard.grd (right):

http://codereview.chromium.org/10538082/diff/1/chrome/app/theme/theme_resourc...
chrome/app/theme/theme_resources_standard.grd:160: <include
name="IDR_OTR_ICON_FULLSCREEN" file="default_100_percent/otr_icon.png"
type="BINDATA" />
On 2012/06/11 20:35:45, benrg wrote:
> This resource ID is never used in non-touch, so this is a dummy image and
could
> point to anything. It's going to be a problem when these files (finally) get
> merged, since there will just be a single <structure> pointing to
> otr_icon_fullscreen, which won't exist in default_[12]00_percent. Do you know
> what the plan is for this situation?
> 
> (Also in theme_resources_2x.grd)

We could have these point to a 1x1 image in layouts where the resource is not
used.  I think this would also work once we collapse to a single <structure>
node for the resources, i.e. as long as we provide a file for each layout that
file will get used.

http://codereview.chromium.org/10538082/diff/1/chrome/app/theme/theme_resourc...
File chrome/app/theme/theme_resources_touch_2x.grd (right):

http://codereview.chromium.org/10538082/diff/1/chrome/app/theme/theme_resourc...
chrome/app/theme/theme_resources_touch_2x.grd:157: <include
name="IDR_OTR_ICON_FULLSCREEN" file="touch_200_percent/otr_icon.png"
type="BINDATA" />
On 2012/06/11 20:35:45, benrg wrote:
> Should be otr_icon_fullscreen.

Thanks for the catch!

http://codereview.chromium.org/10538082/diff/1/chrome/browser/ui/views/frame/...
File chrome/browser/ui/views/frame/browser_view.cc (right):

http://codereview.chromium.org/10538082/diff/1/chrome/browser/ui/views/frame/...
chrome/browser/ui/views/frame/browser_view.cc:1782: int
BrowserView::GetOtrIconResourceID() const {
On 2012/06/11 20:35:45, benrg wrote:
> I asked glen@ and he says this is correct.
> 
> Nit: you could save a line by dropping the variable and using three return
> statements.

Thanks for checking.

Re the extra line:  I prefer to avoid multiple returns out of functions if it's
easy.

http://codereview.chromium.org/10538082/diff/1/chrome/browser/ui/views/frame/...
File chrome/browser/ui/views/frame/browser_view.h (right):

http://codereview.chromium.org/10538082/diff/1/chrome/browser/ui/views/frame/...
chrome/browser/ui/views/frame/browser_view.h:402: int GetOtrIconResourceID()
const;
On 2012/06/11 18:24:14, sky wrote:
> Otr -> OTR

Done.

[commit-bot: I haz the power, 2012-06-11 22:54:28]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/joi@chromium.org/10538082/3002

[commit-bot: I haz the power, 2012-06-11 22:54:29]

Failed to request the patch to try. Please note that binary filesare still
unsupported at the moment, this is being worked on.

Thanks for your patience.

HTTP Error 404: Not Found

[commit-bot: I haz the power, 2012-06-11 22:59:03]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/joi@chromium.org/10538082/5002

[commit-bot: I haz the power, 2012-06-12 00:33:20]

Change committed as 141573

[commit-bot: I haz the power, 2012-06-12 13:49:46]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/joi@chromium.org/10538082/13001

[commit-bot: I haz the power, 2012-06-12 15:32:23]

Change committed as 141663