Separate image processing logic from presentation logic.

[ui/surface] Separate image processing logic from presentation logic.

The D3D image processing code from AcceleratedPresenter moves to its own class, "ui/surface/accelerated_surface_transformer_win.h". This split allows the image processing code to be tested independently of the present-scheduling system, and so we add unit tests doing exactly that. Utility functions (loading d3d, creating temp surfaces) doing things commonly required by test, transform, and present code -- these functions are moved to a third location, "ui/surface/d3d9_utils_win.h"

The new unit tests -- which live in the ui_unittests binary -- make extensive use of pseudorandom image content, which I think is kind of neat. The tests use D3D HAL devices; I tried to use REF, but it didn't work, as StretchRect+LINEAR is not supported by refrast. So as a result we may have a GPU vendor dependency in these results.

BUG=161537
TEST=new ui_unittests

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

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

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

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

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

[ncarter (slow), 2012-12-14 22:07:46]

Hi Al,

Please review and let me know what you think. It probably makes sense to start
with the edits in accelerated_presenter -- the code in the "new" files is
really, for the most part, copypasta from the old file.

One thing I'm not certain about: is ui_unittests the right binary for these
(gpu-dependent) unit tests to belong to?

[apatrick_chromium, 2012-12-14 23:07:58]

https://codereview.chromium.org/11464017/diff/6011/ui/surface/accelerated_sur...
File ui/surface/accelerated_surface_transformer_win.cc (right):

https://codereview.chromium.org/11464017/diff/6011/ui/surface/accelerated_sur...
ui/surface/accelerated_surface_transformer_win.cc:174:
device()->SetRenderTarget(0, NULL);
This will fail with D3DERR_INVALIDCALL I think.
http://msdn.microsoft.com/en-us/library/windows/desktop/bb174455(v=vs.85).aspx

https://codereview.chromium.org/11464017/diff/6011/ui/surface/accelerated_sur...
File ui/surface/accelerated_surface_transformer_win_unittest.cc (right):

https://codereview.chromium.org/11464017/diff/6011/ui/surface/accelerated_sur...
ui/surface/accelerated_surface_transformer_win_unittest.cc:29: 
nit: extra newline

https://codereview.chromium.org/11464017/diff/6011/ui/surface/accelerated_sur...
ui/surface/accelerated_surface_transformer_win_unittest.cc:136: max_error =
std::max(max_error, std::abs((int)a[i] - (int)b[i]));
nit: no c-style casts

https://codereview.chromium.org/11464017/diff/6011/ui/surface/accelerated_sur...
ui/surface/accelerated_surface_transformer_win_unittest.cc:182: // Asserts that
an image is symmetric with respect to itself: both
Comment is copy and paste of as AssertSymmetry?

https://codereview.chromium.org/11464017/diff/6011/ui/surface/accelerated_sur...
ui/surface/accelerated_surface_transformer_win_unittest.cc:273: #if
defined(OS_WIN)
Won't this whole file need to be ifdefed out? Assuming gyp doesn't know not to
compile _win_unittest.cc files on non-windows platforms?

https://codereview.chromium.org/11464017/diff/6011/ui/surface/accelerated_sur...
ui/surface/accelerated_surface_transformer_win_unittest.cc:275:
TEST_F(AcceleratedSurfaceTransformerTest, Init) {
I think you'll need to disable these tests for XP.

https://codereview.chromium.org/11464017/diff/6011/ui/surface/accelerated_sur...
ui/surface/accelerated_surface_transformer_win_unittest.cc:284:
ASSERT_EQ(2922058934, d1);
Prefixing the test with FLAKY_ or FAILS_ would change its behavior.

https://codereview.chromium.org/11464017/diff/6011/ui/surface/accelerated_sur...
ui/surface/accelerated_surface_transformer_win_unittest.cc:348: int src_height =
(int) (((int64) src_width) * dst_height / dst_width);
nit: no c-style casts

https://codereview.chromium.org/11464017/diff/6011/ui/surface/accelerated_sur...
ui/surface/accelerated_surface_transformer_win_unittest.cc:382: int src_height =
(int) (((int64) src_width) * dst_height / dst_width);
nit: no c-style casts

https://codereview.chromium.org/11464017/diff/6011/ui/surface/d3d9_utils_win.cc
File ui/surface/d3d9_utils_win.cc (right):

https://codereview.chromium.org/11464017/diff/6011/ui/surface/d3d9_utils_win....
ui/surface/d3d9_utils_win.cc:31: 
nit: extra newline

[ncarter (slow), 2012-12-15 00:04:15]

https://codereview.chromium.org/11464017/diff/6011/ui/surface/accelerated_sur...
File ui/surface/accelerated_surface_transformer_win.cc (right):

https://codereview.chromium.org/11464017/diff/6011/ui/surface/accelerated_sur...
ui/surface/accelerated_surface_transformer_win.cc:174:
device()->SetRenderTarget(0, NULL);
On 2012/12/14 23:07:58, apatrick_chromium wrote:
> This will fail with D3DERR_INVALIDCALL I think.
>
http://msdn.microsoft.com/en-us/library/windows/desktop/bb174455%28v=vs.85%29...

Good catch; I'll switch it to save/restore the default rendertarget.

https://codereview.chromium.org/11464017/diff/6011/ui/surface/accelerated_sur...
File ui/surface/accelerated_surface_transformer_win_unittest.cc (right):

https://codereview.chromium.org/11464017/diff/6011/ui/surface/accelerated_sur...
ui/surface/accelerated_surface_transformer_win_unittest.cc:273: #if
defined(OS_WIN)
On 2012/12/14 23:07:58, apatrick_chromium wrote:
> Won't this whole file need to be ifdefed out? Assuming gyp doesn't know not to
> compile _win_unittest.cc files on non-windows platforms?

I think it's unnecessary -- this was cruft from the file I used as a template.

https://codereview.chromium.org/11464017/diff/6011/ui/surface/accelerated_sur...
ui/surface/accelerated_surface_transformer_win_unittest.cc:275:
TEST_F(AcceleratedSurfaceTransformerTest, Init) {
On 2012/12/14 23:07:58, apatrick_chromium wrote:
> I think you'll need to disable these tests for XP.

Good point.

https://codereview.chromium.org/11464017/diff/6011/ui/surface/accelerated_sur...
ui/surface/accelerated_surface_transformer_win_unittest.cc:284:
ASSERT_EQ(2922058934, d1);
On 2012/12/14 23:07:58, apatrick_chromium wrote:
> Prefixing the test with FLAKY_ or FAILS_ would change its behavior.

Eek, gross. I guess I'll move to an explicit seed.

[ncarter (slow), 2012-12-17 18:58:59]

All fixed.

https://codereview.chromium.org/11464017/diff/6011/ui/surface/accelerated_sur...
File ui/surface/accelerated_surface_transformer_win.cc (right):

https://codereview.chromium.org/11464017/diff/6011/ui/surface/accelerated_sur...
ui/surface/accelerated_surface_transformer_win.cc:174:
device()->SetRenderTarget(0, NULL);
On 2012/12/15 00:04:15, ncarter wrote:
> On 2012/12/14 23:07:58, apatrick_chromium wrote:
> > This will fail with D3DERR_INVALIDCALL I think.
> >
>
http://msdn.microsoft.com/en-us/library/windows/desktop/bb174455%2528v=vs.85%...
> 
> Good catch; I'll switch it to save/restore the default rendertarget.

Done. I verified that the tests don't pass with the d3d debug mode "break on
error" setting.

https://codereview.chromium.org/11464017/diff/6011/ui/surface/accelerated_sur...
File ui/surface/accelerated_surface_transformer_win_unittest.cc (right):

https://codereview.chromium.org/11464017/diff/6011/ui/surface/accelerated_sur...
ui/surface/accelerated_surface_transformer_win_unittest.cc:29: 
On 2012/12/14 23:07:58, apatrick_chromium wrote:
> nit: extra newline

Done.

https://codereview.chromium.org/11464017/diff/6011/ui/surface/accelerated_sur...
ui/surface/accelerated_surface_transformer_win_unittest.cc:136: max_error =
std::max(max_error, std::abs((int)a[i] - (int)b[i]));
On 2012/12/14 23:07:58, apatrick_chromium wrote:
> nit: no c-style casts

Done.  Looks like my vacation in Java-land rotted my brain a little.

https://codereview.chromium.org/11464017/diff/6011/ui/surface/accelerated_sur...
ui/surface/accelerated_surface_transformer_win_unittest.cc:273: #if
defined(OS_WIN)
On 2012/12/15 00:04:15, ncarter wrote:
> On 2012/12/14 23:07:58, apatrick_chromium wrote:
> > Won't this whole file need to be ifdefed out? Assuming gyp doesn't know not
to
> > compile _win_unittest.cc files on non-windows platforms?
> 
> I think it's unnecessary -- this was cruft from the file I used as a template.

Done.

https://codereview.chromium.org/11464017/diff/6011/ui/surface/accelerated_sur...
ui/surface/accelerated_surface_transformer_win_unittest.cc:275:
TEST_F(AcceleratedSurfaceTransformerTest, Init) {
On 2012/12/15 00:04:15, ncarter wrote:
> On 2012/12/14 23:07:58, apatrick_chromium wrote:
> > I think you'll need to disable these tests for XP.
> 
> Good point.

Done.

https://codereview.chromium.org/11464017/diff/6011/ui/surface/accelerated_sur...
ui/surface/accelerated_surface_transformer_win_unittest.cc:284:
ASSERT_EQ(2922058934, d1);
On 2012/12/15 00:04:15, ncarter wrote:
> On 2012/12/14 23:07:58, apatrick_chromium wrote:
> > Prefixing the test with FLAKY_ or FAILS_ would change its behavior.
> 
> Eek, gross. I guess I'll move to an explicit seed.

Done.

https://codereview.chromium.org/11464017/diff/6011/ui/surface/accelerated_sur...
ui/surface/accelerated_surface_transformer_win_unittest.cc:348: int src_height =
(int) (((int64) src_width) * dst_height / dst_width);
On 2012/12/14 23:07:58, apatrick_chromium wrote:
> nit: no c-style casts

Done.

https://codereview.chromium.org/11464017/diff/6011/ui/surface/accelerated_sur...
ui/surface/accelerated_surface_transformer_win_unittest.cc:382: int src_height =
(int) (((int64) src_width) * dst_height / dst_width);
On 2012/12/14 23:07:58, apatrick_chromium wrote:
> nit: no c-style casts

Done.

https://codereview.chromium.org/11464017/diff/6011/ui/surface/d3d9_utils_win.cc
File ui/surface/d3d9_utils_win.cc (right):

https://codereview.chromium.org/11464017/diff/6011/ui/surface/d3d9_utils_win....
ui/surface/d3d9_utils_win.cc:31: 
On 2012/12/14 23:07:58, apatrick_chromium wrote:
> nit: extra newline

Done.

[ncarter (slow), 2012-12-17 19:13:48]

https://codereview.chromium.org/11464017/diff/24001/ui/ui_unittests.gypi
File ui/ui_unittests.gypi (right):

https://codereview.chromium.org/11464017/diff/24001/ui/ui_unittests.gypi#newc...
ui/ui_unittests.gypi:54: '../ui/surface/surface.gyp:surface',
I'm seeing a circular dependency error on the Mac trybots (though I don't quite
follow why). I'll fix this somehow.

[apatrick_chromium, 2012-12-17 21:30:44]

LGTM once mac cyclical dependency in figured out.

https://codereview.chromium.org/11464017/diff/24001/ui/surface/accelerated_su...
File ui/surface/accelerated_surface_transformer_win_unittest.cc (right):

https://codereview.chromium.org/11464017/diff/24001/ui/surface/accelerated_su...
ui/surface/accelerated_surface_transformer_win_unittest.cc:60:
rng_.seed(base::Hash(StringPrintf("%s", seed)));
rng_.seed(base::Hash(seed)) doesn't work? I think the std::string implicit
constructor from const char* will make it work.

https://codereview.chromium.org/11464017/diff/24001/ui/surface/accelerated_su...
ui/surface/accelerated_surface_transformer_win_unittest.cc:278: if
(!IsVistaOrLater())
I was hoping you would discover a better way of doing this! Oh well. This is how
I deal with tests that shouldn't run certain platforms as well.

https://codereview.chromium.org/11464017/diff/24001/ui/ui_unittests.gypi
File ui/ui_unittests.gypi (right):

https://codereview.chromium.org/11464017/diff/24001/ui/ui_unittests.gypi#newc...
ui/ui_unittests.gypi:54: '../ui/surface/surface.gyp:surface',
On 2012/12/17 19:13:49, ncarter wrote:
> I'm seeing a circular dependency error on the Mac trybots (though I don't
quite
> follow why). I'll fix this somehow.

Still or does this fix it? There's a gyp-explain tool:

https://codereview.chromium.org/8672006/

I think you can run this on a mac to see what the dependency is:

python tools/gyp-explain.py surface

Or something along those lines. The internet knows how.

[ncarter (slow), 2012-12-17 23:48:46]

https://codereview.chromium.org/11464017/diff/24001/ui/surface/accelerated_su...
File ui/surface/accelerated_surface_transformer_win_unittest.cc (right):

https://codereview.chromium.org/11464017/diff/24001/ui/surface/accelerated_su...
ui/surface/accelerated_surface_transformer_win_unittest.cc:60:
rng_.seed(base::Hash(StringPrintf("%s", seed)));
On 2012/12/17 21:30:44, apatrick_chromium wrote:
> rng_.seed(base::Hash(seed)) doesn't work? I think the std::string implicit
> constructor from const char* will make it work.

Done.

https://codereview.chromium.org/11464017/diff/24001/ui/surface/accelerated_su...
ui/surface/accelerated_surface_transformer_win_unittest.cc:278: if
(!IsVistaOrLater())
On 2012/12/17 21:30:44, apatrick_chromium wrote:
> I was hoping you would discover a better way of doing this! Oh well. This is
how
> I deal with tests that shouldn't run certain platforms as well.

Please have a look at this file again; what I did was to make the test case
parameterized, and the parameters are either a zero- or one- element list
depending on whether the windows version is supported. I think it's a little
cleaner, but just barely. I could go either way.

https://codereview.chromium.org/11464017/diff/24001/ui/ui_unittests.gypi
File ui/ui_unittests.gypi (right):

https://codereview.chromium.org/11464017/diff/24001/ui/ui_unittests.gypi#newc...
ui/ui_unittests.gypi:54: '../ui/surface/surface.gyp:surface',
On 2012/12/17 21:30:44, apatrick_chromium wrote:
> On 2012/12/17 19:13:49, ncarter wrote:
> > I'm seeing a circular dependency error on the Mac trybots (though I don't
> quite
> > follow why). I'll fix this somehow.
> 
> Still or does this fix it? There's a gyp-explain tool:
> 
> https://codereview.chromium.org/8672006/
> 
> I think you can run this on a mac to see what the dependency is:
> 
> python tools/gyp-explain.py surface
> 
> Or something along those lines. The internet knows how.

I think it's a circular dependency in the .gyp files, not in the targets.

Basically, surface.gyp depends on ui.gyp, and with my change ui_unittests.gypi
depends on surface.gyp, and  ui_unittests.gypi is included by ui.gyp.

None of the options I can think of are particularly appealing:
 - Create off a new unit test executable in surface.gyp?
 - Split ui_unittests out of ui.gyp?
 - Change ui/surface/surface.gyp, so that it's a .gypi included by ui.gyp?

The last of these options seems the most workable, but I dislike all three -- do
speak up if you have a better idea.

[apatrick_chromium, 2012-12-18 00:01:12]

https://codereview.chromium.org/11464017/diff/24001/ui/surface/accelerated_su...
File ui/surface/accelerated_surface_transformer_win_unittest.cc (right):

https://codereview.chromium.org/11464017/diff/24001/ui/surface/accelerated_su...
ui/surface/accelerated_surface_transformer_win_unittest.cc:278: if
(!IsVistaOrLater())
On 2012/12/17 23:48:46, ncarter wrote:
> On 2012/12/17 21:30:44, apatrick_chromium wrote:
> > I was hoping you would discover a better way of doing this! Oh well. This is
> how
> > I deal with tests that shouldn't run certain platforms as well.
> 
> Please have a look at this file again; what I did was to make the test case
> parameterized, and the parameters are either a zero- or one- element list
> depending on whether the windows version is supported. I think it's a little
> cleaner, but just barely. I could go either way.

I prefer this. Thanks.

https://codereview.chromium.org/11464017/diff/24001/ui/ui_unittests.gypi
File ui/ui_unittests.gypi (right):

https://codereview.chromium.org/11464017/diff/24001/ui/ui_unittests.gypi#newc...
ui/ui_unittests.gypi:54: '../ui/surface/surface.gyp:surface',
Any of those three are okay I think. Do whichever one works :)

[ncarter (slow), 2012-12-18 00:19:14]

I just made the problematic dependency Windows-only, which fixes the problem by
sweeping it under the rug. It'll have to be solved if someone were to add a Mac
unittest under surface/. I think when that happens, we'll want to merge
surface.gyp into ui.gyp, but that sounds like a pretty finicky change.

Adding Scott for ui/ OWNERS approval.

[apatrick_chromium, 2012-12-18 00:23:20]

I have owners in ui/surface. Just one issue remaining.

https://codereview.chromium.org/11464017/diff/25015/ui/surface/surface.gyp
File ui/surface/surface.gyp (right):

https://codereview.chromium.org/11464017/diff/25015/ui/surface/surface.gyp#ne...
ui/surface/surface.gyp:64: # '<(DEPTH)/ui/ui.gyp:ui',
This is commented out.

[ncarter (slow), 2012-12-18 00:33:17]

On 2012/12/18 00:23:20, apatrick_chromium wrote:
> I have owners in ui/surface. Just one issue remaining.
> 
> https://codereview.chromium.org/11464017/diff/25015/ui/surface/surface.gyp
> File ui/surface/surface.gyp (right):
> 
>
https://codereview.chromium.org/11464017/diff/25015/ui/surface/surface.gyp#ne...
> ui/surface/surface.gyp:64: # '<(DEPTH)/ui/ui.gyp:ui',
> This is commented out.

Won't I need owners for ui/ui_unittests.gypi ?

[ncarter (slow), 2012-12-18 22:13:42]

https://codereview.chromium.org/11464017/diff/25015/ui/surface/surface.gyp
File ui/surface/surface.gyp (right):

https://codereview.chromium.org/11464017/diff/25015/ui/surface/surface.gyp#ne...
ui/surface/surface.gyp:64: # '<(DEPTH)/ui/ui.gyp:ui',
On 2012/12/18 00:23:21, apatrick_chromium wrote:
> This is commented out.

Done.

[commit-bot: I haz the power, 2012-12-18 22:34:31]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/nick@chromium.org/11464017/39001

[commit-bot: I haz the power, 2012-12-18 22:34:40]

Presubmit check for 11464017-39001 failed and returned exit status 1.


Running presubmit commit checks ...

** Presubmit ERRORS **
Missing LGTM from an OWNER for files in these directories:
    ui

Presubmit checks took 1.2s to calculate.

[sky, 2012-12-18 23:38:58]

.gyp* LGTM

[apatrick_chromium, 2012-12-19 00:15:56]

lgtm

[commit-bot: I haz the power, 2012-12-19 20:07:52]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/nick@chromium.org/11464017/39001

[commit-bot: I haz the power, 2012-12-19 23:07:11]

Change committed as 174028

[commit-bot: I haz the power, 2012-12-21 01:47:52]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/nick@chromium.org/11464017/39001

[commit-bot: I haz the power, 2012-12-21 03:20:34]

Change committed as 174338

[ncarter (slow), 2012-12-21 18:46:13]

On 2012/12/21 03:20:34, I haz the power (commit-bot) wrote:
> Change committed as 174338

FYI - this change has been reverted twice now due to issues on the builders.
There are three issues:

 (1) Two of the builders don't have DX enabled (we've fixed this on a third
builder already.

Also, more interestingly, there were these failures:
http://build.chromium.org/p/chromium.win/builders/Win7%20Tests%20%28dbg%29%28...

 (2) In the "large sizes" cases for the CopyInverted test, the above builder
can't allocate a 16K-wide texture. We'll need to change the test, maybe to look
at the MaxTextureWidth/MaxTextureHeight D3DCAPS field.

 (3) In the ResizeBilinear minification test, there are some pretty severe pixel
errors that suggest different behavior of StretchRect versus what we expected.

I'd like to re-commit this change with all the tests marked as FAILS_.

[ncarter (slow), 2012-12-21 22:30:11]

apatrick, Please take another look -- (patch set 9 vs 12). I've marked the tests
as FAILS_, included some bugfixes for a second GPU I had handy, and improved the
diagnostic information when the tests do fail. Hopefully this should make the
tests stick.

[ncarter (slow), 2013-01-02 20:23:22]

Ping: Needs an LGTM to land for the third time.

[apatrick_chromium, 2013-01-02 20:45:58]

Sorry for the delay.

https://codereview.chromium.org/11464017/diff/54002/ui/surface/accelerated_su...
File ui/surface/accelerated_surface_transformer_win_unittest.cc (right):

https://codereview.chromium.org/11464017/diff/54002/ui/surface/accelerated_su...
ui/surface/accelerated_surface_transformer_win_unittest.cc:79:
device()->Present(0, 0, 0, 0));
Eek! Have you considered using a reference device instead of HAL?

[ncarter (slow), 2013-01-02 23:35:21]

https://codereview.chromium.org/11464017/diff/54002/ui/surface/accelerated_su...
File ui/surface/accelerated_surface_transformer_win_unittest.cc (right):

https://codereview.chromium.org/11464017/diff/54002/ui/surface/accelerated_su...
ui/surface/accelerated_surface_transformer_win_unittest.cc:79:
device()->Present(0, 0, 0, 0));
On 2013/01/02 20:45:58, apatrick_chromium wrote:
> Eek! Have you considered using a reference device instead of HAL?

Using a reference device was my original intent when I wrote these tests, but
the reference device doesn't support our usage of StretchRect (it fails if you
ask for LINEAR filtering). So here we are.

I was really caught off guard by the behavior where draw/lock didn't
synchronize. I did not try using an event query to flush, but I think it might
work too.

My immediate goal is to get these tests running on the GPU bots and see what
other gotchas might be lurking.

[apatrick_chromium, 2013-01-02 23:38:09]

On 2013/01/02 23:35:21, ncarter wrote:
>
https://codereview.chromium.org/11464017/diff/54002/ui/surface/accelerated_su...
> File ui/surface/accelerated_surface_transformer_win_unittest.cc (right):
> 
>
https://codereview.chromium.org/11464017/diff/54002/ui/surface/accelerated_su...
> ui/surface/accelerated_surface_transformer_win_unittest.cc:79:
> device()->Present(0, 0, 0, 0));
> On 2013/01/02 20:45:58, apatrick_chromium wrote:
> > Eek! Have you considered using a reference device instead of HAL?
> 
> Using a reference device was my original intent when I wrote these tests, but
> the reference device doesn't support our usage of StretchRect (it fails if you
> ask for LINEAR filtering). So here we are.
> 
> I was really caught off guard by the behavior where draw/lock didn't
> synchronize. I did not try using an event query to flush, but I think it might
> work too.
> 
> My immediate goal is to get these tests running on the GPU bots and see what
> other gotchas might be lurking.

Okay let's try this. Queries like to crash on older Intel drivers. LGTM.

BTW, when this is running in Chrome itself, GMA965 shouldn't be an issue because
it is blacklisted I think.

[commit-bot: I haz the power, 2013-01-02 23:55:00]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/nick@chromium.org/11464017/54002

[commit-bot: I haz the power, 2013-01-03 12:09:40]

Change committed as 174943

[sadrul, 2013-01-03 16:38:15]

Hi! This CL was reverted because the test was failing on the bots:
http://build.chromium.org/p/chromium.win/builders/Win7%20Tests%20%28dbg%29%28...

http://build.chromium.org/p/chromium.win/builders/Win7%20Tests%20%283%29/buil...

http://build.chromium.org/p/chromium.win/builders/Vista%20Tests%20%283%29/bui...

[sadrul, 2013-01-03 16:41:02]

On 2013/01/03 16:38:15, sadrul wrote:
> Hi! This CL was reverted because the test was failing on the bots:
>
http://build.chromium.org/p/chromium.win/builders/Win7%2520Tests%2520%2528dbg...
> 
>
http://build.chromium.org/p/chromium.win/builders/Win7%2520Tests%2520%25283%2...
> 
>
http://build.chromium.org/p/chromium.win/builders/Vista%2520Tests%2520%25283%...

The links again:

http://build.chromium.org/p/chromium.win/builders/Win7%20Tests%20%28dbg%29%28...

http://build.chromium.org/p/chromium.win/builders/Win7%20Tests%20%283%29/buil...

http://build.chromium.org/p/chromium.win/builders/Vista%20Tests%20%283%29/bui...

[apatrick_chromium, 2013-01-04 20:33:01]

https://codereview.chromium.org/11464017/diff/85001/ui/surface/surface.gyp
File ui/surface/surface.gyp (right):

https://codereview.chromium.org/11464017/diff/85001/ui/surface/surface.gyp#ne...
ui/surface/surface.gyp:61: '<(DEPTH)/ui/gl/gl.gyp:gl',
I can't find the dependencies on skia or gl?

[ncarter (slow), 2013-01-04 20:56:27]

PTAL

https://codereview.chromium.org/11464017/diff/85001/ui/surface/surface.gyp
File ui/surface/surface.gyp (right):

https://codereview.chromium.org/11464017/diff/85001/ui/surface/surface.gyp#ne...
ui/surface/surface.gyp:61: '<(DEPTH)/ui/gl/gl.gyp:gl',
On 2013/01/04 20:33:01, apatrick_chromium wrote:
> I can't find the dependencies on skia or gl?

Fixed. I actually botched this change right before uploading it -- the new
target can't be under target_defaults.

[apatrick_chromium, 2013-01-04 21:11:23]

lgtm