Roll up patch for running layout tests under Aura.

Roll up patch for running layout tests under Aura.

This patch modifies RenderThemeChromiumDefault.cpp to be layout-test-aware and to match the way we draw the controls using the "mock theme" in the native-win code path. We also add a new implementation of the WebThemeEngine that draws the actual controls generically using Skia.

With this patch, we should be able to run the layout tests under Aura and everything should pass (except for the 3 tests suppressed in the TestExpectations), but there is subsequent cleanup we should do to the code to make this more generic and remove much of the testing logic from the main rendering code.

In addition, we will probably need to make a few additional changes to get this to work on Linux Aura, but not a lot.

R=jamesr@chromium.org
BUG=229207
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=162042

[Dirk Pranke, 2013-11-05 03:15:59]

move mock theme code from chromium to blink

[Dirk Pranke, 2013-11-05 03:24:14]

argh ... ignore; this is not ready for review yet.

[Dirk Pranke, 2013-11-06 02:12:44]

hide most (?) theme functionality behind isRunningLayoutTest()

[Dirk Pranke, 2013-11-06 21:21:36]

Okay, this is a large patch as-is, but a lot of it is either copy/paste from the
existing windows code, or boiler plate. Take a quick look and tell me if you
want to review this as-is, or break it up into pieces.

Note that we don't have any trybot coverage for this patch (nor any coverage on
the main waterfall) so the best we can do is to run the try jobs and make sure
nothing weird happens, but we can test this until we flip the gyp flags to
enable aura.

[jamesr, 2013-11-06 21:54:08]

If we're using a different WebThemeEngine in layout tests, then I don't
understand all the branches before we call into WebThemeEngine.  Maybe I don't
understand the relationship between WebTestThemeEngineMock and
Platform::current()->themeEngine() - are they not the same thing?  If they
aren't, then most of my comments below probably don't make sense, but the patch
description also needs more words to describe what's actually going on.

https://codereview.chromium.org/41733004/diff/290001/Source/core/platform/Scr...
File Source/core/platform/ScrollbarThemeGtkOrAura.cpp (right):

https://codereview.chromium.org/41733004/diff/290001/Source/core/platform/Scr...
Source/core/platform/ScrollbarThemeGtkOrAura.cpp:123:
WebKit::Platform::current()->themeEngine()->paint(canvas, paintPart, state,
WebKit::WebRect(rect), 0);
how is this code different from the non-testing codepath?

https://codereview.chromium.org/41733004/diff/290001/Source/core/platform/Scr...
Source/core/platform/ScrollbarThemeGtkOrAura.cpp:165: state =
isRunningLayoutTest() ? WebKit::WebThemeEngine::StateHot :
WebKit::WebThemeEngine::StateHover;
i think you should teach the mock WebThemeEngine to correctly handle Hot vs
Hover and not branch in the prod codepath

https://codereview.chromium.org/41733004/diff/290001/Source/core/rendering/Re...
File Source/core/rendering/RenderThemeChromiumDefault.cpp (right):

https://codereview.chromium.org/41733004/diff/290001/Source/core/rendering/Re...
Source/core/rendering/RenderThemeChromiumDefault.cpp:224: return Color(0x00,
0x00, 0xff); // Royal blue.
the activeSelectionBackgroundColor (and other selection colors) are configurable
today via the setSelectionColors() API. why don't you just have the layout test
harness simply set the correct colors in test startup?

https://codereview.chromium.org/41733004/diff/290001/Source/core/rendering/Re...
Source/core/rendering/RenderThemeChromiumDefault.cpp:266: // These sizes match
what WinXP draws for various menus.
we're about to call into the theme to get the size - can you teach the mock
theme to return the appropriate sizes for layout tests when running them?

https://codereview.chromium.org/41733004/diff/290001/Source/core/rendering/Re...
Source/core/rendering/RenderThemeChromiumDefault.cpp:388: ThemePainter
painter(i.context, rect);
why do we need a ThemePainter in the layout test path and not the production
path?

https://codereview.chromium.org/41733004/diff/290001/Source/core/rendering/Re...
Source/core/rendering/RenderThemeChromiumDefault.cpp:423: ThemePainter
painter(i.context, rect);
same question here

https://codereview.chromium.org/41733004/diff/290001/Source/testing/runner/We...
File Source/testing/runner/WebTestThemeEngineMock.cpp (right):

https://codereview.chromium.org/41733004/diff/290001/Source/testing/runner/We...
Source/testing/runner/WebTestThemeEngineMock.cpp:35: #include
"Source/testing/runner/WebTestThemeEngineMock.h"
this is a bit odd. grep can't find any #includes that start with "Source/...".
the current directory is implicitly on the include path for ""-style includes,
so i think you should just say

#include "WebTestThemeEngineMock.h"

that's what other files in this directory appear to do

https://codereview.chromium.org/41733004/diff/290001/Source/testing/runner/We...
Source/testing/runner/WebTestThemeEngineMock.cpp:68: return WebKit::WebSize(0,
0); // FIXME: ASSERT_NOT_REACHED();
what do these FIXMEs mean?

https://codereview.chromium.org/41733004/diff/290001/Source/testing/runner/We...
Source/testing/runner/WebTestThemeEngineMock.cpp:97: webRect.x + webRect.width -
1, webRect.y + webRect.height - 1);
this is really frustrating. inside blink, we have nice utilities to go from
WebRect to IntRect which makes converting to SkIRect easy. in chromium, we have
similar things via gfx::Rect. since this is in a no-mans-land it can't use
either

https://codereview.chromium.org/41733004/diff/290001/public/platform/default/...
File public/platform/default/WebThemeEngine.h (right):

https://codereview.chromium.org/41733004/diff/290001/public/platform/default/...
public/platform/default/WebThemeEngine.h:82: bool isBack; // Whether this is the
'back' part or the 'forward' part.
in other parts of theming code, this is handled by having different Parts for
the forward and backward parts of the track. not sure if that'd be more useful
here than what you're doing

[Dirk Pranke, 2013-11-06 22:29:10]

> If we're using a different WebThemeEngine in layout tests, 
> then I don't understand all the branches before we call into
> WebThemeEngine.  Maybe I don't understand the relationship
> between WebTestThemeEngineMock and 
> Platform::current()->themeEngine() - are they not the same thing? 

Yes, Platform::current()->themeEngine() is returning the mock theme engine.

Unfortunately, the logic for drawing themes is badly (and kind of arbitrarily)
distributed between the RenderTheme* code and the WebThemeEngine*
implementations. Different ports do things in different places (as well as do
them just differently), and as a result there's a bunch of code creeping into
the RenderThemeChromiumDefault.cpp case to match the behavior of
RenderThemeChromiumWin.cpp, even though it's not what the non-test Aura
implementation wants.

If you look at the other RenderTheme* implementations, they all have at least a
few places where they do something differently based on whether they're running
layout tests or not. In other words, WebThemeEngine was never designed to hide
the differences between testing code and production code or even really made to
ensure that we could push all of the platform-specific logic behind it.

The "right" thing to do is some combination of using a single RenderTheme
implementation for testing across all the ports, and pushing more information 
down into WebThemeEngine (and changing that interface) so that you can push more
test-specific code into that implementation. I didn't do that as part of this
patch mostly for expediency reasons (get the tests working under aura, then
clean things up) but also to minimize changes to the other implementations and
to lower risk. 

When I originally was working on this (back in July or whenever it was), I had
planned to do things the right way, but then other things got in the way :(.

https://codereview.chromium.org/41733004/diff/290001/Source/core/platform/Scr...
File Source/core/platform/ScrollbarThemeGtkOrAura.cpp (right):

https://codereview.chromium.org/41733004/diff/290001/Source/core/platform/Scr...
Source/core/platform/ScrollbarThemeGtkOrAura.cpp:123:
WebKit::Platform::current()->themeEngine()->paint(canvas, paintPart, state,
WebKit::WebRect(rect), 0);
On 2013/11/06 21:54:09, jamesr wrote:
> how is this code different from the non-testing codepath?

Are you referring to just lines 123-124, or the whole if block? Lines 123-124
are the same, I copied them here so that we can exit early rather than mixing
flow. I can certainly remove the duplication and put the non-test code in an
else block if you'd prefer.

Generally speaking, I tried to keep the layout test code contiguous with an eye
to moving all of this out into a separate subclass in a future patch. Let me
know if you want me to add comments to indicate that or restructure the code so
that there's fewer duplicated lines instead.

https://codereview.chromium.org/41733004/diff/290001/Source/core/platform/Scr...
Source/core/platform/ScrollbarThemeGtkOrAura.cpp:165: state =
isRunningLayoutTest() ? WebKit::WebThemeEngine::StateHot :
WebKit::WebThemeEngine::StateHover;
On 2013/11/06 21:54:09, jamesr wrote:
> i think you should teach the mock WebThemeEngine to correctly handle Hot vs
> Hover and not branch in the prod codepath

I don't think I can. The theme engine is told whether a part is hot or hovered.
To figure out which was which requires context that it doesn't have. I think
perhaps an alternative is to make the non-mock implementations handle both
states and do the right thing, but I was trying to avoid changing any of the
prod implementations.

Generally speaking, there's some cleanup that can be done to restructure the
WebThemeEngine interface to get rid of some of these problems.

https://codereview.chromium.org/41733004/diff/290001/Source/core/rendering/Re...
File Source/core/rendering/RenderThemeChromiumDefault.cpp (right):

https://codereview.chromium.org/41733004/diff/290001/Source/core/rendering/Re...
Source/core/rendering/RenderThemeChromiumDefault.cpp:224: return Color(0x00,
0x00, 0xff); // Royal blue.
On 2013/11/06 21:54:09, jamesr wrote:
> the activeSelectionBackgroundColor (and other selection colors) are
configurable
> today via the setSelectionColors() API. why don't you just have the layout
test
> harness simply set the correct colors in test startup?

I don't think I was aware of that API. I will look into it.

https://codereview.chromium.org/41733004/diff/290001/Source/core/rendering/Re...
Source/core/rendering/RenderThemeChromiumDefault.cpp:266: // These sizes match
what WinXP draws for various menus.
On 2013/11/06 21:54:09, jamesr wrote:
> we're about to call into the theme to get the size - can you teach the mock
> theme to return the appropriate sizes for layout tests when running them?

Good question, I'll look into that.

https://codereview.chromium.org/41733004/diff/290001/Source/core/rendering/Re...
Source/core/rendering/RenderThemeChromiumDefault.cpp:388: ThemePainter
painter(i.context, rect);
On 2013/11/06 21:54:09, jamesr wrote:
> why do we need a ThemePainter in the layout test path and not the production
> path?

To be honest, I don't fully understand/recall how this code works, but what I
think is the case is that (a) we need the ThemePainter in the
RenderThemeChromiumWin case to deal w/ drawing scaled/transformed GDI-based
controls, (b) the windows mock theme is written assuming you're painting into
the painter canvas, and (c) we want to make the windows baselines here.

So, we don't actually need the painter in normal Aura code, but it's not
immediately obvious how to match the windows baselines without using the
ThemePainter logic. That's part of the FIXME at the top of the file next to the
ThemePainter definitions ... I need to investigate whether I can just rebaseline
the results and get rid of this code altogether, or if there are real features
that need to be supported here.

Not supporting the ThemePainter code path causes ~7 tests to fail. If you'd
prefer, I can just mark those as expected failures and yank all of this code out
for now.

https://codereview.chromium.org/41733004/diff/290001/Source/core/rendering/Re...
Source/core/rendering/RenderThemeChromiumDefault.cpp:423: ThemePainter
painter(i.context, rect);
On 2013/11/06 21:54:09, jamesr wrote:
> same question here

same answer :).

https://codereview.chromium.org/41733004/diff/290001/Source/testing/runner/We...
File Source/testing/runner/WebTestThemeEngineMock.cpp (right):

https://codereview.chromium.org/41733004/diff/290001/Source/testing/runner/We...
Source/testing/runner/WebTestThemeEngineMock.cpp:35: #include
"Source/testing/runner/WebTestThemeEngineMock.h"
On 2013/11/06 21:54:09, jamesr wrote:
> this is a bit odd. grep can't find any #includes that start with "Source/...".
> the current directory is implicitly on the include path for ""-style includes,
> so i think you should just say
> 
> #include "WebTestThemeEngineMock.h"
> 
> that's what other files in this directory appear to do

Done.

https://codereview.chromium.org/41733004/diff/290001/Source/testing/runner/We...
Source/testing/runner/WebTestThemeEngineMock.cpp:68: return WebKit::WebSize(0,
0); // FIXME: ASSERT_NOT_REACHED();
On 2013/11/06 21:54:09, jamesr wrote:
> what do these FIXMEs mean?

These mean that they should really be ASSERT_NOT_REACHED(), but I didn't get
around to actually implementing that :). I will take a look at it now.

https://codereview.chromium.org/41733004/diff/290001/Source/testing/runner/We...
Source/testing/runner/WebTestThemeEngineMock.cpp:97: webRect.x + webRect.width -
1, webRect.y + webRect.height - 1);
On 2013/11/06 21:54:09, jamesr wrote:
> this is really frustrating. inside blink, we have nice utilities to go from
> WebRect to IntRect which makes converting to SkIRect easy. in chromium, we
have
> similar things via gfx::Rect. since this is in a no-mans-land it can't use
> either

Yeah :(. Fortunately, it's not a huge amount of code, nor very volatile.

https://codereview.chromium.org/41733004/diff/290001/public/platform/default/...
File public/platform/default/WebThemeEngine.h (right):

https://codereview.chromium.org/41733004/diff/290001/public/platform/default/...
public/platform/default/WebThemeEngine.h:82: bool isBack; // Whether this is the
'back' part or the 'forward' part.
On 2013/11/06 21:54:09, jamesr wrote:
> in other parts of theming code, this is handled by having different Parts for
> the forward and backward parts of the track. not sure if that'd be more useful
> here than what you're doing

Yup. I was trying to avoid changes to the WebThemeEngine that would impact the
other implementations. Arguably, since no other theme actually cares about
whether the part is a forward or backward part, we shouldn't either and we don't
need this, but we wouldn't match the existing baselines that way, so I'd prefer
to revisit this later.

[jamesr, 2013-11-06 23:16:14]

https://codereview.chromium.org/41733004/diff/290001/Source/core/platform/Scr...
File Source/core/platform/ScrollbarThemeGtkOrAura.cpp (right):

https://codereview.chromium.org/41733004/diff/290001/Source/core/platform/Scr...
Source/core/platform/ScrollbarThemeGtkOrAura.cpp:123:
WebKit::Platform::current()->themeEngine()->paint(canvas, paintPart, state,
WebKit::WebRect(rect), 0);
On 2013/11/06 22:29:11, Dirk Pranke wrote:
> Generally speaking, I tried to keep the layout test code contiguous with an
eye
> to moving all of this out into a separate subclass in a future patch. Let me
> know if you want me to add comments to indicate that or restructure the code
so
> that there's fewer duplicated lines instead.

Let's optimize for how the code is in this patch, not how we think it'll be in
the future.  I think it'll be a lot easier to see what needs to be extracted if
the test-only paths are just the parts needed for tests and not duplicates of
parts of the production code.

https://codereview.chromium.org/41733004/diff/290001/Source/core/platform/Scr...
Source/core/platform/ScrollbarThemeGtkOrAura.cpp:165: state =
isRunningLayoutTest() ? WebKit::WebThemeEngine::StateHot :
WebKit::WebThemeEngine::StateHover;
On 2013/11/06 22:29:11, Dirk Pranke wrote:
> On 2013/11/06 21:54:09, jamesr wrote:
> > i think you should teach the mock WebThemeEngine to correctly handle Hot vs
> > Hover and not branch in the prod codepath
> 
> I don't think I can. The theme engine is told whether a part is hot or
hovered.
> To figure out which was which requires context that it doesn't have. I think
> perhaps an alternative is to make the non-mock implementations handle both
> states and do the right thing, but I was trying to avoid changing any of the
> prod implementations.

It looks like what you want to do is treat StateHover as StateHot when painting
the thumb in layout tests.  What piece of information are you missing in the
WebThemeEngine implementation in order to do that?  Are there ever cases where
we paint a thumb and need to tell the difference in the mock implementation
between the two?  It looks like we're calling directly into WebThemeEngine here.

https://codereview.chromium.org/41733004/diff/290001/Source/core/rendering/Re...
File Source/core/rendering/RenderThemeChromiumDefault.cpp (right):

https://codereview.chromium.org/41733004/diff/290001/Source/core/rendering/Re...
Source/core/rendering/RenderThemeChromiumDefault.cpp:388: ThemePainter
painter(i.context, rect);
On 2013/11/06 22:29:11, Dirk Pranke wrote:
> Not supporting the ThemePainter code path causes ~7 tests to fail. If you'd
> prefer, I can just mark those as expected failures and yank all of this code
out
> for now.

I think it'd be better to either mark those as failing or move the ThemePainter
setup/teardown into the mock theme itself.  It appears to be responsible for a
disproportionate amount of delta in the prod codepaths.

[Dirk Pranke, 2013-11-06 23:46:37]

https://codereview.chromium.org/41733004/diff/290001/Source/core/platform/Scr...
File Source/core/platform/ScrollbarThemeGtkOrAura.cpp (right):

https://codereview.chromium.org/41733004/diff/290001/Source/core/platform/Scr...
Source/core/platform/ScrollbarThemeGtkOrAura.cpp:123:
WebKit::Platform::current()->themeEngine()->paint(canvas, paintPart, state,
WebKit::WebRect(rect), 0);
On 2013/11/06 23:16:15, jamesr wrote:
> On 2013/11/06 22:29:11, Dirk Pranke wrote:
> > Generally speaking, I tried to keep the layout test code contiguous with an
> eye
> > to moving all of this out into a separate subclass in a future patch. Let me
> > know if you want me to add comments to indicate that or restructure the code
> so
> > that there's fewer duplicated lines instead.
> 
> Let's optimize for how the code is in this patch, not how we think it'll be in
> the future.  I think it'll be a lot easier to see what needs to be extracted
if
> the test-only paths are just the parts needed for tests and not duplicates of
> parts of the production code.

Okay, I was pretty sure that keeping all the code together that would need to be
in a subclass would make it easier to extract, but I don't feel strongly one way
or another (I originally wrote it the other way, in fact). So, I will change
this.

https://codereview.chromium.org/41733004/diff/290001/Source/core/rendering/Re...
File Source/core/rendering/RenderThemeChromiumDefault.cpp (right):

https://codereview.chromium.org/41733004/diff/290001/Source/core/rendering/Re...
Source/core/rendering/RenderThemeChromiumDefault.cpp:388: ThemePainter
painter(i.context, rect);
On 2013/11/06 23:16:15, jamesr wrote:
> On 2013/11/06 22:29:11, Dirk Pranke wrote:
> > Not supporting the ThemePainter code path causes ~7 tests to fail. If you'd
> > prefer, I can just mark those as expected failures and yank all of this code
> out
> > for now.
> 
> I think it'd be better to either mark those as failing or move the
ThemePainter
> setup/teardown into the mock theme itself.  It appears to be responsible for a
> disproportionate amount of delta in the prod codepaths.

Yes, that is certainly true. I will remove it and save it for a separate patch.
Initially I had thought that it had to be on this side of WebThemeEngine for
access to the GraphicsContext, but it looks like we really only need the canvas.
There may also be an issue w/ calling into TransparencyWin from the runner code.

[Dirk Pranke, 2013-11-07 04:07:20]

Okay, I think we're getting closer. Changes since the last substantive review:

I removed the ThemePainter code and added TestExpectations entries for the
half-dozen tests that fail

I removed the distinction between the "hot" and "hover" states; it turns out
that only one test was affected by this. 

Apart from the hot/hover thing, the major delta in scrollbar rendering has to do
w/ handling the "disabled" state. It turns out that Aura has a very different
interpretation of what this means than the Win theme (in terms of how the scroll
bars are actually rendered), and trying to match Aura's behavior causes hundreds
of tests to fail.

There are still new states for "readonly" and "focused"; each of these states
has about 15-30 tests that test that something is rendered in that state
properly, so I left things as in the previous patch.

I looked at the setSelectionColor() APIs, but it had a few issues. First, it
doesn't give me a way to set the colors for list boxes (i.e., select/option
elements), so I still need to special-case code for that. Second, I'm not 100%
sure I can put the code in the right location w/o a two-sided patch (I think the
"right" place is in content shell code). Third, it turns out that some of the
RenderTheme code is also buggy and this still ends up working a little oddly. I 
can show you the diff for all this, but it might be best to take this up in a
separate patch.

I think that's everything. Please take a look and let me know if I missed
anything?

[jamesr, 2013-11-07 18:47:57]

ok, this lgtm

You'll have to fix the selection color piping soon - we don't want to have a
runtime branch in that call all the time - but it doesn't have to be in this
patch.

[commit-bot: I haz the power, 2013-11-12 23:13:52]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/dpranke@chromium.org/41733004/530001

[commit-bot: I haz the power, 2013-11-13 17:32:19]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/dpranke@chromium.org/41733004/660001

[commit-bot: I haz the power, 2013-11-13 18:05:36]

Sorry for I got bad news for ya.
Compile failed with a clobber build on mac_layout.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=mac_layout...
Your code is likely broken or HEAD is junk. Please ensure your
code is not broken then alert the build sheriffs.
Look at the try server FAQ for more details.

[commit-bot: I haz the power, 2013-11-13 22:58:21]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/dpranke@chromium.org/41733004/790001

[commit-bot: I haz the power, 2013-11-14 00:31:03]

Retried try job too often on win_blink_rel for step(s) webkit_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_blink_...

[Dirk Pranke, 2013-11-14 19:19:44]

Committed patchset #21 manually as r162042 (presubmit successful).