New issue
Advanced search Search tips

Issue 888262 link

Starred by 1 user
Status: Fixed
Owner:
derat@chromium.org
Closed: Oct 4
Cc:
jkardatzke@chromium.org
nya@chromium.org
Components:
EstimatedDays: ----
NextAction: ----
OS: Chrome
Pri: 2
Type: Bug



Sign in to add a comment

Clean up colors in screenshot package

Project Member Reported by derat@chromium.org, Sep 22 
Per discussion on https://crrev.com/c/1216505, the 
chromiumos/tast/local/screenshot package should probably use color.Color (or the color.RGBA struct?) instead of defining its own struct for RGB colors. The screenshot struct is already a bit weird since it holds 16-bit components even though I think it only wants 8-bit components.

It may also be good to move the image-comparison code into its own package.
Project Member

Comment 1 by bugdroid1@chromium.org, Sep 25 

The following revision refers to this bug:
  https://chromium.googlesource.com/chromiumos/platform/tast-tests/+/66428c64d22ceb5dc68c183c2e105cedb90d57e0

commit 66428c64d22ceb5dc68c183c2e105cedb90d57e0
Author: Daniel Erat <derat@chromium.org>
Date: Tue Sep 25 00:49:39 2018

tast-tests: Fix go vet warnings in tests.

Add a screenshot.RGB function and make the
graphics.Screenshot test call it to avoid a "composite
literal uses unkeyed fields" error from go vet. The
alternative would be calling "screenshot.Color{R: 123, G:
123, B: 123}" everywhere.

Also fix a Fatal call that should be Fatalf in the
subtest.VerifyAppFromTerminal function called by
vm.CrostiniStartEverything.

BUG= chromium:888262 , chromium:888259 
TEST=screenshot test still passes and "go vet" no longer
     complains

Change-Id: I4b481f6a3a01f2292188488d84d3579f2a4019e4
Reviewed-on: https://chromium-review.googlesource.com/1239191
Commit-Ready: Dan Erat <derat@chromium.org>
Tested-by: Dan Erat <derat@chromium.org>
Reviewed-by: Jeffrey Kardatzke <jkardatzke@google.com>

[modify] https://crrev.com/66428c64d22ceb5dc68c183c2e105cedb90d57e0/src/chromiumos/tast/local/bundles/cros/graphics/screenshot.go
[modify] https://crrev.com/66428c64d22ceb5dc68c183c2e105cedb90d57e0/src/chromiumos/tast/local/bundles/cros/vm/subtest/verify_app_from_terminal.go
[modify] https://crrev.com/66428c64d22ceb5dc68c183c2e105cedb90d57e0/src/chromiumos/tast/local/screenshot/screenshot.go
Project Member

Comment 2 by bugdroid1@chromium.org, Oct 2 

The following revision refers to this bug:
  https://chromium.googlesource.com/chromiumos/platform/tast-tests/+/d1ced17123d125451d96548bb653ccca70f336ca

commit d1ced17123d125451d96548bb653ccca70f336ca
Author: Daniel Erat <derat@chromium.org>
Date: Tue Oct 02 23:42:06 2018

tast-tests: Make screenshot package use color.Color.

Remove the screenshot package's Color struct and make it
instead use the existing color.Color interface (and
color.NRGBA behind the scenes for non-alpha-premultiplied,
8-bit-per-channel RGBA colors).

Also simplify some code for getting an image's dimensions.

BUG= chromium:888262 
TEST=verified that graphics.Screenshot passes on sumo and
     vm.CrostiniStartEverything passes on banon

Change-Id: Id42cb39ef398367d2492ea989e278713484ee4ab
Reviewed-on: https://chromium-review.googlesource.com/1255459
Commit-Ready: Dan Erat <derat@chromium.org>
Tested-by: Dan Erat <derat@chromium.org>
Reviewed-by: Dan Erat <derat@chromium.org>

[modify] https://crrev.com/d1ced17123d125451d96548bb653ccca70f336ca/src/chromiumos/tast/local/bundles/cros/vm/subtest/verify_app_from_terminal.go
[modify] https://crrev.com/d1ced17123d125451d96548bb653ccca70f336ca/src/chromiumos/tast/local/screenshot/screenshot.go
[modify] https://crrev.com/d1ced17123d125451d96548bb653ccca70f336ca/src/chromiumos/tast/local/bundles/cros/graphics/sshot/sshot.go
[modify] https://crrev.com/d1ced17123d125451d96548bb653ccca70f336ca/src/chromiumos/tast/local/bundles/cros/vm/crostini_start_everything.go
[modify] https://crrev.com/d1ced17123d125451d96548bb653ccca70f336ca/src/chromiumos/tast/local/bundles/cros/vm/subtest/verify_launcher_app.go
Project Member

Comment 3 by bugdroid1@chromium.org, Oct 4 

The following revision refers to this bug:
  https://chromium.googlesource.com/chromiumos/platform/tast-tests/+/89eb621e2357ceca1014c85141c05d93128093fe

commit 89eb621e2357ceca1014c85141c05d93128093fe
Author: Daniel Erat <derat@chromium.org>
Date: Thu Oct 04 07:37:00 2018

tast-tests: Add colorcmp package.

Move color-related code out of the screenshot package and
into a new colorcmp package. Also add a few quick unit tests
for it.

BUG= chromium:888262 
TEST=new unit tests and graphics.ScreenshotCLI pass

Change-Id: Ibdfaf3322b4f1f9d75e27e1a1356e46e1ddc4631
Reviewed-on: https://chromium-review.googlesource.com/1260060
Commit-Ready: Dan Erat <derat@chromium.org>
Tested-by: Dan Erat <derat@chromium.org>
Reviewed-by: Dan Erat <derat@chromium.org>

[add] https://crrev.com/89eb621e2357ceca1014c85141c05d93128093fe/src/chromiumos/tast/local/colorcmp/cmp.go
[add] https://crrev.com/89eb621e2357ceca1014c85141c05d93128093fe/src/chromiumos/tast/local/colorcmp/cmp_test.go
[modify] https://crrev.com/89eb621e2357ceca1014c85141c05d93128093fe/src/chromiumos/tast/local/bundles/cros/vm/crostini_start_everything.go
[modify] https://crrev.com/89eb621e2357ceca1014c85141c05d93128093fe/src/chromiumos/tast/local/bundles/cros/vm/subtest/verify_launcher_app.go
[modify] https://crrev.com/89eb621e2357ceca1014c85141c05d93128093fe/src/chromiumos/tast/local/bundles/cros/vm/subtest/verify_app_from_terminal.go
[modify] https://crrev.com/89eb621e2357ceca1014c85141c05d93128093fe/src/chromiumos/tast/local/screenshot/screenshot.go
[modify] https://crrev.com/89eb621e2357ceca1014c85141c05d93128093fe/src/chromiumos/tast/local/bundles/cros/graphics/sshot/sshot.go

Comment 4 by derat@chromium.org, Oct 4

Status: Fixed (was: Available)
►

Sign in to add a comment