Skia: Add --colorImages to DM

Skia: Add --colorImages to DM

BUG=

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

[borenet, 2016-05-04 12:09:54]

borenet@google.com changed reviewers:
+ borenet@google.com, msarett@google.com, rmistry@chromium.org

[borenet, 2016-05-04 12:09:55]

https://codereview.chromium.org/1951843002/diff/1/scripts/slave/recipe_module...
File scripts/slave/recipe_modules/skia/api.py (right):

https://codereview.chromium.org/1951843002/diff/1/scripts/slave/recipe_module...
scripts/slave/recipe_modules/skia/api.py:716: self.device_dirs.images_dir,
'colorspace'),
I have a couple of concerns:
1. This will break for backfills before this flag was added to DM.
2. Does DM properly handle this directory being missing? AFAICT it hasn't yet
been added to GS.

[borenet, 2016-05-04 12:10:52]

On 2016/05/04 12:09:55, borenet wrote:
>
https://codereview.chromium.org/1951843002/diff/1/scripts/slave/recipe_module...
> File scripts/slave/recipe_modules/skia/api.py (right):
> 
>
https://codereview.chromium.org/1951843002/diff/1/scripts/slave/recipe_module...
> scripts/slave/recipe_modules/skia/api.py:716: self.device_dirs.images_dir,
> 'colorspace'),
> I have a couple of concerns:
> 1. This will break for backfills before this flag was added to DM.
> 2. Does DM properly handle this directory being missing? AFAICT it hasn't yet
> been added to GS.

Maybe we should consider adding the colorspace images to v5 in GS and then make
this flag conditional for versions 5+?

[scroggo, 2016-05-04 12:50:31]

scroggo@google.com changed reviewers:
+ scroggo@google.com

[scroggo, 2016-05-04 12:50:32]

On 2016/05/04 12:10:52, borenet wrote:
> Maybe we should consider adding the colorspace images to v5 in GS and then
make
> this flag conditional for versions 5+?

Definitely. I think that is a perfect use of the version number.

https://codereview.chromium.org/1951843002/diff/1/scripts/slave/recipe_module...
File scripts/slave/recipe_modules/skia/api.py (right):

https://codereview.chromium.org/1951843002/diff/1/scripts/slave/recipe_module...
scripts/slave/recipe_modules/skia/api.py:716: self.device_dirs.images_dir,
'colorspace'),
On 2016/05/04 12:09:55, borenet wrote:
> I have a couple of concerns:
> 1. This will break for backfills before this flag was added to DM.

Isn't this a more general problem any time we add a new flag to DM? Has this not
happened before? (I suppose we have not added any since we started backfilling?)

> 2. Does DM properly handle this directory being missing? AFAICT it hasn't yet
> been added to GS.

I believe CollectImages only causes a problem if a named directory/file is
missing/empty, but if no directories/files were listed, so not being there yet
should not be a problem.

[borenet, 2016-05-04 12:56:16]

https://codereview.chromium.org/1951843002/diff/1/scripts/slave/recipe_module...
File scripts/slave/recipe_modules/skia/api.py (right):

https://codereview.chromium.org/1951843002/diff/1/scripts/slave/recipe_module...
scripts/slave/recipe_modules/skia/api.py:716: self.device_dirs.images_dir,
'colorspace'),
On 2016/05/04 12:50:32, scroggo wrote:
> On 2016/05/04 12:09:55, borenet wrote:
> > I have a couple of concerns:
> > 1. This will break for backfills before this flag was added to DM.
> 
> Isn't this a more general problem any time we add a new flag to DM? Has this
not
> happened before? (I suppose we have not added any since we started
backfilling?)
> 

Yeah, it's a general problem.  I just feel I need to point it out when it
happens and think briefly about whether there's any way to avoid it.  I think
"version >= 5" conditional would prevent any issues.

> > 2. Does DM properly handle this directory being missing? AFAICT it hasn't
yet
> > been added to GS.
> 
> I believe CollectImages only causes a problem if a named directory/file is
> missing/empty, but if no directories/files were listed, so not being there yet
> should not be a problem.

[msarett, 2016-05-04 13:22:42]

I think it makes perfect sense to make this flag conditional on v5.

I've added a v5 directory to gsutil that has subfolders v5/dm, v5/nanobench, and
v5/colorspace.

[borenet, 2016-05-04 13:53:47]

On 2016/05/04 13:22:42, msarett wrote:
> I think it makes perfect sense to make this flag conditional on v5.
> 
> I've added a v5 directory to gsutil that has subfolders v5/dm, v5/nanobench,
and
> v5/colorspace.

Patch set 2 makes the flag conditional based on the version. I'm triple-checking
locally.

[msarett, 2016-05-04 14:51:39]

lgtm

[borenet, 2016-05-05 17:33:50]

The CQ bit was checked by borenet@google.com

[commit-bot: I haz the power, 2016-05-05 17:33:55]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1951843002/20001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1951843002/20001

[borenet, 2016-05-05 17:40:29]

The CQ bit was checked by borenet@google.com

[borenet, 2016-05-05 17:40:29]

The patchset sent to the CQ was uploaded after l-g-t-m from msarett@google.com
Link to the patchset: https://codereview.chromium.org/1951843002/#ps40001
(title: "rebase")

[commit-bot: I haz the power, 2016-05-05 17:40:35]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1951843002/40001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1951843002/40001

[commit-bot: I haz the power, 2016-05-05 17:44:35]

Description was changed from

==========
Skia: Add --colorImages to DM

BUG=
==========

to

==========
Skia: Add --colorImages to DM

BUG=

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

[commit-bot: I haz the power, 2016-05-05 17:44:36]

Committed patchset #3 (id:40001) as
http://src.chromium.org/viewvc/chrome?view=rev&revision=300458