Repack separate 100 / 200 / touch_100 / touch_200 files into single pak files.

Repack separate 100 / 200 / touch_100 / touch_200 files into single pak files.

BUG=139803
TEST=Chrome runs with ui and theme resources correctly loaded.

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

[flackr, 2012-07-31 18:41:14]

Can you take a preliminary look? Plus you can let me know if this should merge
easily with your webkit patch :-).

[Rick Byers, 2012-07-31 19:38:17]

lgtm

https://chromiumcodereview.appspot.com/10825111/diff/1/chrome/chrome_repack_c...
File chrome/chrome_repack_chrome_100_percent.gypi (right):

https://chromiumcodereview.appspot.com/10825111/diff/1/chrome/chrome_repack_c...
chrome/chrome_repack_chrome_100_percent.gypi:1: # Copyright (c) 2012 The
Chromium Authors. All rights reserved.
I was assuming we'd want to do this all in chrome_repack_resources.gypi, just to
keep it conceptually simple and unified.  But maybe adding a separate gypi file
per output pak is better.  What do you think?

https://chromiumcodereview.appspot.com/10825111/diff/1/chrome/chrome_resource...
File chrome/chrome_resources.gyp (right):

https://chromiumcodereview.appspot.com/10825111/diff/1/chrome/chrome_resource...
chrome/chrome_resources.gyp:350: ['enable_hidpi == 1 or chromeos == 1', {
Would be nice to simplify to just enable_hidpi==1.  Perhaps chromeos=1 should
always set enable_hidpi=1, or maybe with it being board-specific now we should
plumb it through.

Anyway you're just moving the existing condition so this is fine for now if it's
extra work to clean it up.

[Rick Byers, 2012-07-31 19:43:40]

On 2012/07/31 18:41:14, flackr wrote:
> Can you take a preliminary look? Plus you can let me know if this should merge
> easily with your webkit patch :-).

Oh, and I'm sure there will be conflicts with my patch
(http://codereview.chromium.org/10701105/) but they're trivial to resolve.  I'm
putting my patch in the CQ now, so presumably I'll land first and you can
update.

[flackr, 2012-07-31 19:48:21]

https://chromiumcodereview.appspot.com/10825111/diff/1/chrome/chrome_repack_c...
File chrome/chrome_repack_chrome_100_percent.gypi (right):

https://chromiumcodereview.appspot.com/10825111/diff/1/chrome/chrome_repack_c...
chrome/chrome_repack_chrome_100_percent.gypi:1: # Copyright (c) 2012 The
Chromium Authors. All rights reserved.
On 2012/07/31 19:38:17, Rick Byers wrote:
> I was assuming we'd want to do this all in chrome_repack_resources.gypi, just
to
> keep it conceptually simple and unified.  But maybe adding a separate gypi
file
> per output pak is better.  What do you think?

I agree doing them in a single file would be nicer, however an includes clause
in gyp expects a single top-level item which in an actions list can only be a
dictionary describing one action. I could do this with a separate target in
chrome_resources.gyp though and have the single gypi include file contain the
entire actions array if this is preferable.

https://chromiumcodereview.appspot.com/10825111/diff/1/chrome/chrome_resource...
File chrome/chrome_resources.gyp (right):

https://chromiumcodereview.appspot.com/10825111/diff/1/chrome/chrome_resource...
chrome/chrome_resources.gyp:350: ['enable_hidpi == 1 or chromeos == 1', {
On 2012/07/31 19:38:17, Rick Byers wrote:
> Would be nice to simplify to just enable_hidpi==1.  Perhaps chromeos=1 should
> always set enable_hidpi=1, or maybe with it being board-specific now we should
> plumb it through.

Yes, we can probably just use enable_hidpi. I don't think chromeos==1 should
imply enable_hidpi since that's board specific as you mentioned.

> 
> Anyway you're just moving the existing condition so this is fine for now if
it's
> extra work to clean it up.

[flackr, 2012-07-31 20:07:45]

Nico, Sailesh, can you review / gives owners approval for the changes to chrome/
and ui/base/resource/? Thanks!

[sail, 2012-07-31 20:11:16]

lgtm

[Rick Byers, 2012-07-31 20:15:18]

On 2012/07/31 19:48:21, flackr wrote:
>
https://chromiumcodereview.appspot.com/10825111/diff/1/chrome/chrome_repack_c...
> File chrome/chrome_repack_chrome_100_percent.gypi (right):
> 
>
https://chromiumcodereview.appspot.com/10825111/diff/1/chrome/chrome_repack_c...
> chrome/chrome_repack_chrome_100_percent.gypi:1: # Copyright (c) 2012 The
> Chromium Authors. All rights reserved.
> On 2012/07/31 19:38:17, Rick Byers wrote:
> > I was assuming we'd want to do this all in chrome_repack_resources.gypi,
just
> to
> > keep it conceptually simple and unified.  But maybe adding a separate gypi
> file
> > per output pak is better.  What do you think?
> 
> I agree doing them in a single file would be nicer, however an includes clause
> in gyp expects a single top-level item which in an actions list can only be a
> dictionary describing one action. I could do this with a separate target in
> chrome_resources.gyp though and have the single gypi include file contain the
> entire actions array if this is preferable.

It sounds like that would be a little nicer, but I don't have a strong care
factor here.

>
https://chromiumcodereview.appspot.com/10825111/diff/1/chrome/chrome_resource...
> File chrome/chrome_resources.gyp (right):
> 
>
https://chromiumcodereview.appspot.com/10825111/diff/1/chrome/chrome_resource...
> chrome/chrome_resources.gyp:350: ['enable_hidpi == 1 or chromeos == 1', {
> On 2012/07/31 19:38:17, Rick Byers wrote:
> > Would be nice to simplify to just enable_hidpi==1.  Perhaps chromeos=1
should
> > always set enable_hidpi=1, or maybe with it being board-specific now we
should
> > plumb it through.
> 
> Yes, we can probably just use enable_hidpi. I don't think chromeos==1 should
> imply enable_hidpi since that's board specific as you mentioned.
> 
> > 
> > Anyway you're just moving the existing condition so this is fine for now if
> it's
> > extra work to clean it up.

[Josh Horwich, 2012-08-01 23:47:58]

Just a drive-by question/concern about removing "or chromeos=1" clause in
including 200% resources. If this lands as-is, the question can be safely
ignored (for now?) :)

https://chromiumcodereview.appspot.com/10825111/diff/1/chrome/chrome_resource...
File chrome/chrome_resources.gyp (right):

https://chromiumcodereview.appspot.com/10825111/diff/1/chrome/chrome_resource...
chrome/chrome_resources.gyp:350: ['enable_hidpi == 1 or chromeos == 1', {
On 2012/07/31 19:48:22, flackr wrote:
> On 2012/07/31 19:38:17, Rick Byers wrote:
> > Would be nice to simplify to just enable_hidpi==1.  Perhaps chromeos=1
should
> > always set enable_hidpi=1, or maybe with it being board-specific now we
should
> > plumb it through.
> 
> Yes, we can probably just use enable_hidpi. I don't think chromeos==1 should
> imply enable_hidpi since that's board specific as you mentioned.

Drive-by question: Wouldn't this mean that HiDPI external monitors wouldn't get
HiDPI assets when plugged into a ChromeOS device whose build doesn't specify
enable_hidpi? If so, is that the desired behavior?

Also, if we do remove 'chromeos==1' here, I suppose it needs to be removed from
the chrome_touch_200_percent.pak section below.

> 
> > 
> > Anyway you're just moving the existing condition so this is fine for now if
> it's
> > extra work to clean it up.
>

[Rick Byers, 2012-08-02 00:30:46]

https://chromiumcodereview.appspot.com/10825111/diff/1/chrome/chrome_resource...
File chrome/chrome_resources.gyp (right):

https://chromiumcodereview.appspot.com/10825111/diff/1/chrome/chrome_resource...
chrome/chrome_resources.gyp:350: ['enable_hidpi == 1 or chromeos == 1', {
On 2012/08/01 23:47:58, Josh Horwich wrote:
> On 2012/07/31 19:48:22, flackr wrote:
> > On 2012/07/31 19:38:17, Rick Byers wrote:
> > > Would be nice to simplify to just enable_hidpi==1.  Perhaps chromeos=1
> should
> > > always set enable_hidpi=1, or maybe with it being board-specific now we
> should
> > > plumb it through.
> > 
> > Yes, we can probably just use enable_hidpi. I don't think chromeos==1 should
> > imply enable_hidpi since that's board specific as you mentioned.
> 
> Drive-by question: Wouldn't this mean that HiDPI external monitors wouldn't
get
> HiDPI assets when plugged into a ChromeOS device whose build doesn't specify
> enable_hidpi? If so, is that the desired behavior?

Yes, see http://code.google.com/p/chromium-os/issues/detail?id=32711
 
> Also, if we do remove 'chromeos==1' here, I suppose it needs to be removed
from
> the chrome_touch_200_percent.pak section below.
> 
> > 
> > > 
> > > Anyway you're just moving the existing condition so this is fine for now
if
> > it's
> > > extra work to clean it up.
> > 
>

[flackr, 2012-08-07 13:51:59]

https://chromiumcodereview.appspot.com/10825111/diff/1/chrome/chrome_resource...
File chrome/chrome_resources.gyp (right):

https://chromiumcodereview.appspot.com/10825111/diff/1/chrome/chrome_resource...
chrome/chrome_resources.gyp:350: ['enable_hidpi == 1 or chromeos == 1', {
On 2012/08/01 23:47:58, Josh Horwich wrote:
> On 2012/07/31 19:48:22, flackr wrote:
> > On 2012/07/31 19:38:17, Rick Byers wrote:
> > > Would be nice to simplify to just enable_hidpi==1.  Perhaps chromeos=1
> should
> > > always set enable_hidpi=1, or maybe with it being board-specific now we
> should
> > > plumb it through.
> > 
> > Yes, we can probably just use enable_hidpi. I don't think chromeos==1 should
> > imply enable_hidpi since that's board specific as you mentioned.
> 
> Drive-by question: Wouldn't this mean that HiDPI external monitors wouldn't
get
> HiDPI assets when plugged into a ChromeOS device whose build doesn't specify
> enable_hidpi? If so, is that the desired behavior?
> 
> Also, if we do remove 'chromeos==1' here, I suppose it needs to be removed
from
> the chrome_touch_200_percent.pak section below.

You're right, Done.

> 
> > 
> > > 
> > > Anyway you're just moving the existing condition so this is fine for now
if
> > it's
> > > extra work to clean it up.
> > 
>

[flackr, 2012-08-07 13:57:00]

Scott, can you take a look? Thanks!

[sky, 2012-08-07 18:38:13]

LGTM

[commit-bot: I haz the power, 2012-08-07 18:50:30]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/flackr@chromium.org/10825111/12001

[commit-bot: I haz the power, 2012-08-07 19:00:21]

Try job failure for 10825111-12001 (retry) on android for steps "compile, build"
(clobber build).
It's a second try, previously, steps "compile, build" failed.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android&nu...

[commit-bot: I haz the power, 2012-08-07 19:18:52]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/flackr@chromium.org/10825111/20

[commit-bot: I haz the power, 2012-08-07 21:17:40]

Change committed as 150411