Moved most viewport layout tests into a unit test

Moved most viewport layout tests into a unit test

BUG=

Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=160873

[bokan, 2013-10-28 22:01:24]

My desktop viewport patch breaks all of these. While fixing them, I thought I'd
move them into a unit test since they're touching internal objects layout tests
seems like the wrong place for them. In addition, they'll now run faster, be
easier to debug, less flaky, etc.

A hand-full remain in fast/viewport that weren't so simple to move.

[aelias_OOO_until_Jul13, 2013-10-28 23:06:43]

This looks fine to me; I like that this will make them run more frequently on
Android bots.  Rune/Ken, have any concerns with this?

https://codereview.chromium.org/48613005/diff/1/Source/web/tests/ViewportTest...
File Source/web/tests/ViewportTest.cpp (right):

https://codereview.chromium.org/48613005/diff/1/Source/web/tests/ViewportTest...
Source/web/tests/ViewportTest.cpp:2: * Copyright (C) 2010 Google Inc. All rights
reserved.
2013

[rune, 2013-10-29 10:03:34]

On 2013/10/28 23:06:43, aelias wrote:
> This looks fine to me; I like that this will make them run more frequently on
> Android bots.  Rune/Ken, have any concerns with this?

sgtm

[bokan, 2013-10-29 15:50:16]

https://codereview.chromium.org/48613005/diff/1/Source/web/tests/ViewportTest...
File Source/web/tests/ViewportTest.cpp (right):

https://codereview.chromium.org/48613005/diff/1/Source/web/tests/ViewportTest...
Source/web/tests/ViewportTest.cpp:2: * Copyright (C) 2010 Google Inc. All rights
reserved.
On 2013/10/28 23:06:46, aelias wrote:
> 2013

Done.

[aelias_OOO_until_Jul13, 2013-10-29 18:10:21]

lgtm

I guess we can't delete the layout test methods in
Source/core/testing/Internals.cpp because of the remaining viewport tests?  What
about them is hard to move over?  (We don't need to do it in this patch, but I'd
like to know for reference.)

[bokan, 2013-10-29 18:29:41]

On 2013/10/29 18:10:21, aelias wrote:
> lgtm
> 
> I guess we can't delete the layout test methods in
> Source/core/testing/Internals.cpp because of the remaining viewport tests? 
What
> about them is hard to move over?  (We don't need to do it in this patch, but
I'd
> like to know for reference.)

Most are testing the console error/warnings generated (I think it's the dev
tools console). There's probably a way to read it in the web test but I decided
to leave them till a future patch. There's also one test that checks that
internals.settings.setViewportEnabled works, I assume that only works in layout
tests. None of these really need to use the internals bits. There's just one
test that actually does that checks removing and adding a DOCTYPE, it looked a
little more involved (though it might be easy) so I left it for now. I'll make a
new CL for that shortly.

[commit-bot: I haz the power, 2013-10-29 18:56:41]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/bokan@chromium.org/48613005/70001

[commit-bot: I haz the power, 2013-10-29 21:38:58]

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

[commit-bot: I haz the power, 2013-10-29 21:41:38]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/bokan@chromium.org/48613005/70001

[commit-bot: I haz the power, 2013-10-29 23:24:20]

Change committed as 160873

[rune, 2013-10-31 10:05:25]

On 2013/10/29 18:10:21, aelias wrote:
> lgtm
> 
> I guess we can't delete the layout test methods in
> Source/core/testing/Internals.cpp because of the remaining viewport tests? 
What
> about them is hard to move over?  (We don't need to do it in this patch, but
I'd
> like to know for reference.)

The css3/device-adapt tests still use internals.viewportAsText, but I think they
should be rewritten to use the production code path instead.

Some points:

1. @viewport support is enabled with a runtime flag, so it's enabled for
LayoutTests.
2. Applying the resolved viewport arguments (from @viewport or meta viewport) is
controlled by --enable-viewport command line switch, which is not on for
LayoutTests by default.
3. internals.viewportAsText does not use the full production code path, so
integration with the rest of the code, timing issues, dom changes etc, can not
be tested with this function.
4. internals.viewportAsText does not check for --enable-viewport, so it works in
LayoutTests without passing --enable-viewport to content_shell.
5. I did introduce internals.settings.setViewportEnabled at some point to be
able to run production code path through javascript instead of passing
--enable-viewport to content_shell when running LayoutTests.


So, what I think should be done is:

1. For the remaining tests using viewportAsText which are actually unit tests
for viewport argument resolving, convert them unit tests like has been done in
this CL.

2. For the remaining tests using viewportAsText which should take the rest of
the code into account, enable viewport through configuring the testing framework
to pass --enable-viewport or use internal.settings.setViewportEnabled(true) in
the script like in css3/device-adapt/viewport-delete-rule.html. I think there
recently was a mail from Eric Seidel saying that passing the command line switch
was the preferred way.

3. The device-adapt/opera tests comes from the csswg test server. There is a
script that generates an alternative version of the tests for Blink that
currently use viewportAsText. With the --enable-viewport passed, or adding
setViewportEnabled(true), I think the tests can be used as originally written
for w3c. I can look into css3/device-adapt/opera, rewrite the Blink test
generation there and import them into Blink in that form.

4. The --enable-viewport setting should probably be made into a runtime flag
instead (I don't think it currently is).