Mirror images for RTL languages at build time.

Mirror images for RTL languages at build time.

This adds a build step to generate mirrored images for use in
right-to-left (RTL) languages. Images are mirrored by flipping the
original image over the vertical axis. Every image must be explicitly
listed as mirrorable or non-mirrorable in a config file.

The goal: ensure that our RTL image assets are always up-to-date.

BUG=290225
NOTRY=true

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

[newt (away), 2013-12-05 18:54:52]

PTAL.

[newt (away), 2013-12-05 19:33:21]

https://codereview.chromium.org/106173002/diff/40001/build/android/gyp/proces...
File build/android/gyp/process_resources.py (right):

https://codereview.chromium.org/106173002/diff/40001/build/android/gyp/proces...
build/android/gyp/process_resources.py:145: CrunchImages(aapt,
options.image_input_dir, options.crunch_output_dir)
Chris, do we ever delete the res_crunched directory? If so, we should probably
delete the res_mirrored directory then too.

[cjhopman, 2013-12-06 17:28:22]

https://codereview.chromium.org/106173002/diff/40001/build/android/gyp/mirror...
File build/android/gyp/mirror_images.py (right):

https://codereview.chromium.org/106173002/diff/40001/build/android/gyp/mirror...
build/android/gyp/mirror_images.py:152: errors.append(item + ' is listed
multiple times.')
I'd change this message so its different than the one on 145. Maybe " appears in
both lists" or some such.

https://codereview.chromium.org/106173002/diff/40001/build/android/gyp/proces...
File build/android/gyp/process_resources.py (right):

https://codereview.chromium.org/106173002/diff/40001/build/android/gyp/proces...
build/android/gyp/process_resources.py:9: import multiprocessing.pool
unused?

https://codereview.chromium.org/106173002/diff/40001/build/android/gyp/proces...
build/android/gyp/process_resources.py:145: CrunchImages(aapt,
options.image_input_dir, options.crunch_output_dir)
On 2013/12/05 19:33:22, newt wrote:
> Chris, do we ever delete the res_crunched directory? If so, we should probably
> delete the res_mirrored directory then too.

I don't think we do.

I think the correct approach for these things would be to have their output be a
zip file instead of a directory and then the later step that consumes it would
handle it however it had to.

For now, I wouldn't worry about it.

https://codereview.chromium.org/106173002/diff/40001/build/install-build-deps...
File build/install-build-deps-android.sh (right):

https://codereview.chromium.org/106173002/diff/40001/build/install-build-deps...
build/install-build-deps-android.sh:47: sudo apt-get -y install checkstyle
imagemagick lighttpd python-pexpect xvfb \
Could we start a pattern of documenting what these packages are for?

[newt (away), 2013-12-06 19:22:50]

https://codereview.chromium.org/106173002/diff/40001/build/android/gyp/mirror...
File build/android/gyp/mirror_images.py (right):

https://codereview.chromium.org/106173002/diff/40001/build/android/gyp/mirror...
build/android/gyp/mirror_images.py:152: errors.append(item + ' is listed
multiple times.')
On 2013/12/06 17:28:23, cjhopman wrote:
> I'd change this message so its different than the one on 145. Maybe " appears
in
> both lists" or some such.

Done.

https://codereview.chromium.org/106173002/diff/40001/build/android/gyp/proces...
File build/android/gyp/process_resources.py (right):

https://codereview.chromium.org/106173002/diff/40001/build/android/gyp/proces...
build/android/gyp/process_resources.py:9: import multiprocessing.pool
On 2013/12/06 17:28:23, cjhopman wrote:
> unused?

Done.

https://codereview.chromium.org/106173002/diff/40001/build/install-build-deps...
File build/install-build-deps-android.sh (right):

https://codereview.chromium.org/106173002/diff/40001/build/install-build-deps...
build/install-build-deps-android.sh:47: sudo apt-get -y install checkstyle
imagemagick lighttpd python-pexpect xvfb \
On 2013/12/06 17:28:23, cjhopman wrote:
> Could we start a pattern of documenting what these packages are for?

Done. Not sure if there's a prettier way of doing this. You can't intersperse
comments inside a multiline statement (except with weird hacks, e.g.
http://stackoverflow.com/a/12797512/598094)

[Kibeom Kim (inactive), 2013-12-06 20:07:26]

lgtm

https://codereview.chromium.org/106173002/diff/40001/build/android/gyp/mirror...
File build/android/gyp/mirror_images.py (right):

https://codereview.chromium.org/106173002/diff/40001/build/android/gyp/mirror...
build/android/gyp/mirror_images.py:85: self.config_errors = []
how about using exception? or.. passing config_errors around by returning from
the below functions instead of a member variable?

https://codereview.chromium.org/106173002/diff/40001/build/android/gyp/mirror...
build/android/gyp/mirror_images.py:154: # Ensure all images on disk are listed
in config file.
totally unimportant but: how about, instead of 'on disk', 'on filesystem' or 'in
directory' because it's not necessarily disk.

https://codereview.chromium.org/106173002/diff/40001/build/android/gyp/mirror...
build/android/gyp/mirror_images.py:163: 'remove the image).')
Since this will be the error message people see most frequently including who
has never dealt rtl, so, to give more context, how about putting the bug link
here crbug.com/290225 and make the link public?

https://codereview.chromium.org/106173002/diff/40001/build/android/gyp/mirror...
build/android/gyp/mirror_images.py:186: pool.map(self._mirror_image,
images_to_mirror)
I'm not sure, but managing threadpool inside a script feels like defeating the
purpose of gyp&ninja. Practically, I'm ok with this. (cc'ed Shashi since we once
briefly talked this before)

[newt (away), 2013-12-06 23:04:21]

https://codereview.chromium.org/106173002/diff/40001/build/android/gyp/mirror...
File build/android/gyp/mirror_images.py (right):

https://codereview.chromium.org/106173002/diff/40001/build/android/gyp/mirror...
build/android/gyp/mirror_images.py:85: self.config_errors = []
On 2013/12/06 20:07:27, Kibeom Kim wrote:
> how about using exception? or.. passing config_errors around by returning from
> the below functions instead of a member variable?

Done.

https://codereview.chromium.org/106173002/diff/40001/build/android/gyp/mirror...
build/android/gyp/mirror_images.py:154: # Ensure all images on disk are listed
in config file.
On 2013/12/06 20:07:27, Kibeom Kim wrote:
> totally unimportant but: how about, instead of 'on disk', 'on filesystem' or
'in
> directory' because it's not necessarily disk.

Done.

https://codereview.chromium.org/106173002/diff/40001/build/android/gyp/mirror...
build/android/gyp/mirror_images.py:163: 'remove the image).')
On 2013/12/06 20:07:27, Kibeom Kim wrote:
> Since this will be the error message people see most frequently including who
> has never dealt rtl, so, to give more context, how about putting the bug link
> here crbug.com/290225 and make the link public?

Good idea. I added an extra message after all the error messages, see line ~260

https://codereview.chromium.org/106173002/diff/40001/build/android/gyp/mirror...
build/android/gyp/mirror_images.py:186: pool.map(self._mirror_image,
images_to_mirror)
On 2013/12/06 20:07:27, Kibeom Kim wrote:
> I'm not sure, but managing threadpool inside a script feels like defeating the
> purpose of gyp&ninja. Practically, I'm ok with this. (cc'ed Shashi since we
once
> briefly talked this before)

I agree that it would nice to have the parallelism managed by ninja, but in
practice this doesn't seem easy to achieve. We'd have to define each image as a
separate gyp target, which is messier, less self-contained, and requires running
part of this script at gyp time.

[cjhopman, 2013-12-13 21:42:03]

lgtm

[commit-bot: I haz the power, 2014-01-07 15:47:12]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/newt@chromium.org/106173002/250001

[commit-bot: I haz the power, 2014-01-07 17:56:16]

Retried try job too often on win_rel for step(s) nacl_integration
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...

[commit-bot: I haz the power, 2014-01-07 17:59:57]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/newt@chromium.org/106173002/250001

[commit-bot: I haz the power, 2014-01-07 18:02:32]

Change committed as 243332

[pschmidt1, 2014-01-07 23:54:46]

The change to install-build-deps-android.sh to install imagemagick has
introduced a package dependency bug where ia32-libs cannot be installed on
ubuntu precise systems.

This is blocking chrome-infra's ability to deploy new systems.     Can you
please revert this?

Thanks.

[newt (away), 2014-01-07 23:58:30]

A revert of this CL has been created in
https://codereview.chromium.org/126543004/ by newt@chromium.org.

The reason for reverting is: this introduced a package dependency bug where
ia32-libs cannot be installed on ubuntu precise systems.

[newt (away), 2014-01-08 00:01:56]

pschmidt: I've reverted this for now, but would like to re-land it after
dealing with the package dependencies. Could you explain the problem in
some more detail?


On Tue, Jan 7, 2014 at 3:54 PM, <pschmidt@chromium.org> wrote:

> The change to install-build-deps-android.sh to install imagemagick has
> introduced a package dependency bug where ia32-libs cannot be installed on
> ubuntu precise systems.
>
> This is blocking chrome-infra's ability to deploy new systems.     Can you
> please revert this?
>
> Thanks.
>
> https://codereview.chromium.org/106173002/
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[pschmidt1, 2014-01-08 00:13:39]

Our deploy scripts call build-install-deps.sh as well as
build-install-deps-android.sh
After calling these it go head with some additional package installs including
ia32-libs

With imagemagick installed ia32-libs can't be installed because of a now broken
package dependency.

So something is borked at Canonical's end.   We should be able to find a fix for
this.



On 2014/01/08 00:01:56, newt wrote:
> pschmidt: I've reverted this for now, but would like to re-land it after
> dealing with the package dependencies. Could you explain the problem in
> some more detail?
> 
> 
> On Tue, Jan 7, 2014 at 3:54 PM, <mailto:pschmidt@chromium.org> wrote:
> 
> > The change to install-build-deps-android.sh to install imagemagick has
> > introduced a package dependency bug where ia32-libs cannot be installed on
> > ubuntu precise systems.
> >
> > This is blocking chrome-infra's ability to deploy new systems.     Can you
> > please revert this?
> >
> > Thanks.
> >
> > https://codereview.chromium.org/106173002/
> >
> 
> To unsubscribe from this group and stop receiving emails from it, send an
email
> to mailto:chromium-reviews+unsubscribe@chromium.org.