Remove display() from more layout tests

Remove display() from more layout tests

These are the remaining tests that call display() directly and also do
not need actually need a paint phase to test what they intend to.

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

[enne (OOO), 2014-03-19 20:13:26]

PTAL

[scherkus (not reviewing), 2014-03-19 20:20:35]

https://codereview.chromium.org/196573042/diff/1/LayoutTests/media/video-fram...
File LayoutTests/media/video-frame-size-change.html (right):

https://codereview.chromium.org/196573042/diff/1/LayoutTests/media/video-fram...
LayoutTests/media/video-frame-size-change.html:52: runAfterDisplay(function() {
some notes on this test as I was the one who wrote it:

this test did want to run painting *before* playing the video to make sure that
our blink/media/gfx code handles frame size changes (otherwise we only end up
painting the final frame of the video, which would be as if the video didn't
have any frame size changes)

will rAD() (which uses rAF()) still provide the same coverage?

[enne (OOO), 2014-03-19 20:22:33]

https://codereview.chromium.org/196573042/diff/1/LayoutTests/media/video-fram...
File LayoutTests/media/video-frame-size-change.html (right):

https://codereview.chromium.org/196573042/diff/1/LayoutTests/media/video-fram...
LayoutTests/media/video-frame-size-change.html:52: runAfterDisplay(function() {
On 2014/03/19 20:20:35, scherkus wrote:
> some notes on this test as I was the one who wrote it:
> 
> this test did want to run painting *before* playing the video to make sure
that
> our blink/media/gfx code handles frame size changes (otherwise we only end up
> painting the final frame of the video, which would be as if the video didn't
> have any frame size changes)
> 
> will rAD() (which uses rAF()) still provide the same coverage?

The frame size changes happen during painting and not during layout?

[enne (OOO), 2014-03-19 21:02:38]

From offline conversation, it sounds like this is not an issue for a composited
video and maybe the solution is to make this test composited or use a canvas
instead.  In the short term, I've removed it from this patch.

[scherkus (not reviewing), 2014-03-19 21:14:33]

On 2014/03/19 21:02:38, enne wrote:
> From offline conversation, it sounds like this is not an issue for a
composited
> video and maybe the solution is to make this test composited or use a canvas
> instead.  In the short term, I've removed it from this patch.

Feel free to bug one of us media folk to get it re-written.

[scherkus (not reviewing), 2014-03-19 21:14:46]

On 2014/03/19 21:14:33, scherkus wrote:
> On 2014/03/19 21:02:38, enne wrote:
> > From offline conversation, it sounds like this is not an issue for a
> composited
> > video and maybe the solution is to make this test composited or use a canvas
> > instead.  In the short term, I've removed it from this patch.
> 
> Feel free to bug one of us media folk to get it re-written.

(or file a bug / assign it to me / etc)

[enne (OOO), 2014-03-20 19:49:55]

Filed as http://crbug.com/354168, and that test is removed from this patch.

[enne (OOO), 2014-03-27 19:28:36]

+ojan

[ojan, 2014-03-27 20:31:49]

lgtm. Thanks for pushing this through!

https://codereview.chromium.org/196573042/diff/20001/LayoutTests/TestExpectat...
File LayoutTests/TestExpectations (right):

https://codereview.chromium.org/196573042/diff/20001/LayoutTests/TestExpectat...
LayoutTests/TestExpectations:244: crbug.com/345027
css3/filters/multiple-filters-invalidation.html [ ImageOnlyFailure
NeedsRebaseline ]
The ImageOnlyFailure here is redundant (and ignored).

https://codereview.chromium.org/196573042/diff/20001/LayoutTests/css3/filters...
File LayoutTests/css3/filters/multiple-filters-invalidation.html (right):

https://codereview.chromium.org/196573042/diff/20001/LayoutTests/css3/filters...
LayoutTests/css3/filters/multiple-filters-invalidation.html:17: <script
src="../../resources/run-after-display.js"></script>
Should run-after-display.js call testRunner.waitUntilDone for you? There's no
way to use runAfterDisplay without calling waitUntilDone.

https://codereview.chromium.org/196573042/diff/20001/LayoutTests/fast/animati...
File LayoutTests/fast/animation/request-animation-frame-during-modal.html
(right):

https://codereview.chromium.org/196573042/diff/20001/LayoutTests/fast/animati...
LayoutTests/fast/animation/request-animation-frame-during-modal.html:17: var src
= 'window.setTimeout(window.close, 100);';
Why 100ms? This seems fragile and likely to be flaky.

Maybe we don't care since we're getting rid of showModalDialog in a couple
weeks?

https://codereview.chromium.org/196573042/diff/20001/LayoutTests/fast/replace...
File LayoutTests/fast/replaced/width100percent-textarea.html (left):

https://codereview.chromium.org/196573042/diff/20001/LayoutTests/fast/replace...
LayoutTests/fast/replaced/width100percent-textarea.html:10:
testRunner.display();
Does removing this not change behavior? Didn't this used to paint the overlay
and then paint the table contents on top of that?

Now, I'm not sure why that's actually useful.

[enne (OOO), 2014-03-27 20:51:05]

https://codereview.chromium.org/196573042/diff/20001/LayoutTests/TestExpectat...
File LayoutTests/TestExpectations (right):

https://codereview.chromium.org/196573042/diff/20001/LayoutTests/TestExpectat...
LayoutTests/TestExpectations:244: crbug.com/345027
css3/filters/multiple-filters-invalidation.html [ ImageOnlyFailure
NeedsRebaseline ]
On 2014/03/27 20:31:49, ojan wrote:
> The ImageOnlyFailure here is redundant (and ignored).

Oh, ok.  Does NeedsRebaseline imply Failure?

https://codereview.chromium.org/196573042/diff/20001/LayoutTests/css3/filters...
File LayoutTests/css3/filters/multiple-filters-invalidation.html (right):

https://codereview.chromium.org/196573042/diff/20001/LayoutTests/css3/filters...
LayoutTests/css3/filters/multiple-filters-invalidation.html:17: <script
src="../../resources/run-after-display.js"></script>
On 2014/03/27 20:31:49, ojan wrote:
> Should run-after-display.js call testRunner.waitUntilDone for you? There's no
> way to use runAfterDisplay without calling waitUntilDone.

It's a good point.  I think maybe I didn't like the magic of including something
that makes your test asynchronous and like the explicit nature of having the
test both waitUntilDone+notifyDone?  There are other js files in other
resources/ folder that do some weird setup that always surprises me, so this is
a bit reactionary to that.

https://codereview.chromium.org/196573042/diff/20001/LayoutTests/fast/animati...
File LayoutTests/fast/animation/request-animation-frame-during-modal.html
(right):

https://codereview.chromium.org/196573042/diff/20001/LayoutTests/fast/animati...
LayoutTests/fast/animation/request-animation-frame-during-modal.html:17: var src
= 'window.setTimeout(window.close, 100);';
On 2014/03/27 20:31:49, ojan wrote:
> Why 100ms? This seems fragile and likely to be flaky.
> 
> Maybe we don't care since we're getting rid of showModalDialog in a couple
> weeks?

Yeah, it's a magic number.  Since raf can't be invoked while in a modal dialog,
I can't use raf and have to fall back on setTimeout.

This test is trying to make sure that raf is not invoked while a modal dialog is
up.  raf should almost certianly run within 100ms, so it seemed like a
reasonable period of time to wait.

Do you have a better suggestion?

https://codereview.chromium.org/196573042/diff/20001/LayoutTests/fast/replace...
File LayoutTests/fast/replaced/width100percent-textarea.html (left):

https://codereview.chromium.org/196573042/diff/20001/LayoutTests/fast/replace...
LayoutTests/fast/replaced/width100percent-textarea.html:10:
testRunner.display();
On 2014/03/27 20:31:49, ojan wrote:
> Does removing this not change behavior? Didn't this used to paint the overlay
> and then paint the table contents on top of that?
> 
> Now, I'm not sure why that's actually useful.

Yeah, I don't think it's useful either.  It does change behavior, but I don't
think this test is attempting to check for over repainting (since it invalidates
the entire page as it adds the elements below after the display.  The text
indicates it's only looking for the final pixel output.

[ojan, 2014-03-27 21:58:44]

lgtm

https://codereview.chromium.org/196573042/diff/20001/LayoutTests/TestExpectat...
File LayoutTests/TestExpectations (right):

https://codereview.chromium.org/196573042/diff/20001/LayoutTests/TestExpectat...
LayoutTests/TestExpectations:244: crbug.com/345027
css3/filters/multiple-filters-invalidation.html [ ImageOnlyFailure
NeedsRebaseline ]
On 2014/03/27 20:51:05, enne wrote:
> On 2014/03/27 20:31:49, ojan wrote:
> > The ImageOnlyFailure here is redundant (and ignored).
> 
> Oh, ok.  Does NeedsRebaseline imply Failure?

Yes. You should only ever need to put NeedsRebaseline by itself, anything else
is either redundant or wrong (e.g. if you do Crash NeedsRebaseline, then that's
wrong because we can't rebaseline a test that crashes).

https://codereview.chromium.org/196573042/diff/20001/LayoutTests/fast/animati...
File LayoutTests/fast/animation/request-animation-frame-during-modal.html
(right):

https://codereview.chromium.org/196573042/diff/20001/LayoutTests/fast/animati...
LayoutTests/fast/animation/request-animation-frame-during-modal.html:17: var src
= 'window.setTimeout(window.close, 100);';
On 2014/03/27 20:51:05, enne wrote:
> On 2014/03/27 20:31:49, ojan wrote:
> > Why 100ms? This seems fragile and likely to be flaky.
> > 
> > Maybe we don't care since we're getting rid of showModalDialog in a couple
> > weeks?
> 
> Yeah, it's a magic number.  Since raf can't be invoked while in a modal
dialog,
> I can't use raf and have to fall back on setTimeout.
> 
> This test is trying to make sure that raf is not invoked while a modal dialog
is
> up.  raf should almost certianly run within 100ms, so it seemed like a
> reasonable period of time to wait.
> 
> Do you have a better suggestion?

Can you requestAnimationFrame inside the showModalDialog? I guess that doesn't
guarantee that we would have tried to put up a frame in the parent page.

Meh. Not worth worrying about.

[ojan, 2014-03-27 21:59:53]

https://codereview.chromium.org/196573042/diff/20001/LayoutTests/css3/filters...
File LayoutTests/css3/filters/multiple-filters-invalidation.html (right):

https://codereview.chromium.org/196573042/diff/20001/LayoutTests/css3/filters...
LayoutTests/css3/filters/multiple-filters-invalidation.html:17: <script
src="../../resources/run-after-display.js"></script>
On 2014/03/27 20:51:05, enne wrote:
> On 2014/03/27 20:31:49, ojan wrote:
> > Should run-after-display.js call testRunner.waitUntilDone for you? There's
no
> > way to use runAfterDisplay without calling waitUntilDone.
> 
> It's a good point.  I think maybe I didn't like the magic of including
something
> that makes your test asynchronous and like the explicit nature of having the
> test both waitUntilDone+notifyDone?  There are other js files in other
> resources/ folder that do some weird setup that always surprises me, so this
is
> a bit reactionary to that.

Yeah, I don't like js files doing weird setup either. I guess you can imagine
adding other functions to this file, a which point it wouldn't make sense to
waitUntilDone all the time.

[enne (OOO), 2014-03-27 22:03:04]

https://codereview.chromium.org/196573042/diff/20001/LayoutTests/TestExpectat...
File LayoutTests/TestExpectations (right):

https://codereview.chromium.org/196573042/diff/20001/LayoutTests/TestExpectat...
LayoutTests/TestExpectations:244: crbug.com/345027
css3/filters/multiple-filters-invalidation.html [ ImageOnlyFailure
NeedsRebaseline ]
On 2014/03/27 21:58:45, ojan wrote:
> On 2014/03/27 20:51:05, enne wrote:
> > On 2014/03/27 20:31:49, ojan wrote:
> > > The ImageOnlyFailure here is redundant (and ignored).
> > 
> > Oh, ok.  Does NeedsRebaseline imply Failure?
> 
> Yes. You should only ever need to put NeedsRebaseline by itself, anything else
> is either redundant or wrong (e.g. if you do Crash NeedsRebaseline, then
that's
> wrong because we can't rebaseline a test that crashes).

Thanks.  Done.

https://codereview.chromium.org/196573042/diff/20001/LayoutTests/fast/animati...
File LayoutTests/fast/animation/request-animation-frame-during-modal.html
(right):

https://codereview.chromium.org/196573042/diff/20001/LayoutTests/fast/animati...
LayoutTests/fast/animation/request-animation-frame-during-modal.html:17: var src
= 'window.setTimeout(window.close, 100);';
On 2014/03/27 21:58:45, ojan wrote:
> On 2014/03/27 20:51:05, enne wrote:
> > On 2014/03/27 20:31:49, ojan wrote:
> > > Why 100ms? This seems fragile and likely to be flaky.
> > > 
> > > Maybe we don't care since we're getting rid of showModalDialog in a couple
> > > weeks?
> > 
> > Yeah, it's a magic number.  Since raf can't be invoked while in a modal
> dialog,
> > I can't use raf and have to fall back on setTimeout.
> > 
> > This test is trying to make sure that raf is not invoked while a modal
dialog
> is
> > up.  raf should almost certianly run within 100ms, so it seemed like a
> > reasonable period of time to wait.
> > 
> > Do you have a better suggestion?
> 
> Can you requestAnimationFrame inside the showModalDialog? I guess that doesn't
> guarantee that we would have tried to put up a frame in the parent page.

There's a rAF right before it.  Since showModalDialog is synchronous, then
that's equivalent to it being inside, I think.

> Meh. Not worth worrying about.

Yeah, my thought exactly.

[enne (OOO), 2014-03-27 22:03:09]

The CQ bit was checked by enne@chromium.org

[commit-bot: I haz the power, 2014-03-27 22:03:16]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/enne@chromium.org/196573042/60001

[commit-bot: I haz the power, 2014-03-27 22:36:26]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-03-27 22:36:26]

Try jobs failed on following builders:
  tryserver.blink on linux_blink_rel

[enne (OOO), 2014-03-27 22:52:54]

The CQ bit was checked by enne@chromium.org

[commit-bot: I haz the power, 2014-03-27 22:53:06]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/enne@chromium.org/196573042/100001

[commit-bot: I haz the power, 2014-03-27 23:59:29]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-03-27 23:59:30]

Try jobs failed on following builders:
  tryserver.blink on win_blink_rel

[enne (OOO), 2014-03-28 00:06:50]

The CQ bit was checked by enne@chromium.org

[commit-bot: I haz the power, 2014-03-28 00:06:52]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/enne@chromium.org/196573042/100001

[commit-bot: I haz the power, 2014-03-28 00:42:28]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-03-28 00:42:28]

Try jobs failed on following builders:
  tryserver.blink on win_blink_rel

[enne (OOO), 2014-03-28 00:53:18]

The CQ bit was checked by enne@chromium.org

[commit-bot: I haz the power, 2014-03-28 00:53:26]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/enne@chromium.org/196573042/100001

[commit-bot: I haz the power, 2014-03-28 01:39:20]

Change committed as 170239