Update navigator-detached-no-crash.html test to use testharness.js.

Update navigator-detached-no-crash.html test to use testharness.js.

That way, we no longer need to depend on an expected file that will be
outdated everytime we add something to Navigator.

BUG=None

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

[mlamouri (slow - plz ping), 2014-05-31 15:06:27]

This requires https://codereview.chromium.org/308223002 to work for two reasons:
* navigator.webkitGamepads call will produce a CONSOLE WARNING;
* There is a bug in the innerText algorithm that adds a space at the beginning
of the string for some reasons that I do not yet understand.

[eseidel, 2014-05-31 18:05:09]

<pre> and <listing> will delete extra spaces/newlines at teh start of text:
http://www.whatwg.org/specs/web-apps/current-work/multipage/tree-construction...

But I don't know of algorithms in the DOM which add them.

There is a fixme about not taking whitespace collapsing into account, not sure
what that's for:
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...

[mlamouri (slow - plz ping), 2014-06-01 11:32:29]

On 2014/05/31 18:05:09, eseidel wrote:
> <pre> and <listing> will delete extra spaces/newlines at teh start of text:
>
http://www.whatwg.org/specs/web-apps/current-work/multipage/tree-construction...
> 
> But I don't know of algorithms in the DOM which add them.
> 
> There is a fixme about not taking whitespace collapsing into account, not sure
> what that's for:
>
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...

The <pre> doesn't have any space but the innerText algorithm will add one space
before it. For the moment, my tests show that this happen if I wait for the
iframe to be loaded before removing it. I will try to nail this down to a test
case.

[mlamouri (slow - plz ping), 2014-06-01 12:07:37]

On 2014/05/31 18:05:09, eseidel wrote:
> <pre> and <listing> will delete extra spaces/newlines at teh start of text:
>
http://www.whatwg.org/specs/web-apps/current-work/multipage/tree-construction...
> 
> But I don't know of algorithms in the DOM which add them.
> 
> There is a fixme about not taking whitespace collapsing into account, not sure
> what that's for:
>
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...

I think the problem is probably related to the TextIterator instead of
::setInnerText. We never call ::setInnerText but we call Element::innerText()
which calls plainText(Range*) which is implemented here:
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...
I guess the problem might come from here.

Note that I have open a bug: http://crbug.com/379551

[Dirk Pranke, 2014-06-01 20:58:27]

I defer to Eric or someone else more appropriate to review the test to make sure
it's still testing the right thing. I'm fine with switching it to a
testharness-based test.

[eseidel, 2014-06-01 21:52:37]

Don't you need the new baseline file?

[eseidel, 2014-06-01 21:53:34]

I think you can just take the original test and make it not print for each
property and that will be fine.  I'd rather not otherwise change the test if we
don't have to.

[Dirk Pranke, 2014-06-01 23:30:02]

On 2014/06/01 21:53:34, eseidel wrote:
> I think you can just take the original test and make it not print for each
> property and that will be fine.  I'd rather not otherwise change the test if
we
> don't have to.

You don't need an -expected.txt for testharness-based tests, if you expect the
test to pass.. That's the whole point of this change.

[mlamouri (slow - plz ping), 2014-06-02 09:55:04]

On 2014/06/01 21:53:34, eseidel wrote:
> I think you can just take the original test and make it not print for each
> property and that will be fine.  I'd rather not otherwise change the test if
we
> don't have to.

This is basically what I'm doing by using testharness.js. I'm not changing the
test otherwise, just did some cleanups here and there because the test file was
a bit messy.

[mlamouri (slow - plz ping), 2014-06-02 09:55:05]

On 2014/06/01 21:53:34, eseidel wrote:
> I think you can just take the original test and make it not print for each
> property and that will be fine.  I'd rather not otherwise change the test if
we
> don't have to.

This is basically what I'm doing by using testharness.js. I'm not changing the
test otherwise, just did some cleanups here and there because the test file was
a bit messy.

[eseidel, 2014-06-02 16:43:04]

So does this test no longer have output?

[Dirk Pranke, 2014-06-02 16:50:49]

On 2014/06/02 16:43:04, eseidel wrote:
> So does this test no longer have output?

It produces output. 

testharness-based tests have a prescribed format. There's a header and a footer,
and in between there are lines that start with either "PASS" or "FAIL" depending
on the results of the assertion (this is all done in testharnessreport.js).

If a test's output contains the header and footer, at least one PASS, and no
FAILs, then the test is considered to have passed.

We do this so that we don't have to track and keep up-to-date -expected.txt
files for all of the imported w3c tests.

I don't think we have much of a precedent for doing this for blink-specific
tests, but I don't think there's a real problem w/ doing this if people get used
to it, and it would allow us to delete a fairly large number of baselines.

I think I mentioned this somewhere or other, but if we do find an -expected.txt,
we use it in preference to the parsing code described above; this allows us to
track tests that partially fail and of course is backwards-compatible as well.

[mlamouri (slow - plz ping), 2014-06-02 18:13:24]

On 2014/06/02 16:50:49, Dirk Pranke wrote:
> On 2014/06/02 16:43:04, eseidel wrote:
> > So does this test no longer have output?
> 
> It produces output. 
> 
> testharness-based tests have a prescribed format. There's a header and a
footer,
> and in between there are lines that start with either "PASS" or "FAIL"
depending
> on the results of the assertion (this is all done in testharnessreport.js).
> 
> If a test's output contains the header and footer, at least one PASS, and no
> FAILs, then the test is considered to have passed.
> 
> We do this so that we don't have to track and keep up-to-date -expected.txt
> files for all of the imported w3c tests.
> 
> I don't think we have much of a precedent for doing this for blink-specific
> tests, but I don't think there's a real problem w/ doing this if people get
used
> to it, and it would allow us to delete a fairly large number of baselines.
> 
> I think I mentioned this somewhere or other, but if we do find an
-expected.txt,
> we use it in preference to the parsing code described above; this allows us to
> track tests that partially fail and of course is backwards-compatible as well.

Thanks for explaining what I was trying to do :)

I actually wrote a script that detects useless -expected.txt files. I will write
a PRESUBMIT rule to prevent -expected.txt when not needed and land the changes
from my script at the same time.

[eseidel, 2014-06-02 18:23:16]

lgtm

I'm gonna trust you here.

https://codereview.chromium.org/301193006/diff/1/LayoutTests/fast/dom/navigat...
File LayoutTests/fast/dom/navigator-detached-no-crash.html (right):

https://codereview.chromium.org/301193006/diff/1/LayoutTests/fast/dom/navigat...
LayoutTests/fast/dom/navigator-detached-no-crash.html:50: }, 200);
Bleh!  setTimeout tests = flaky tests. :(

[mlamouri (slow - plz ping), 2014-06-02 18:36:09]

Thanks for the review Eric! :)

https://codereview.chromium.org/301193006/diff/1/LayoutTests/fast/dom/navigat...
File LayoutTests/fast/dom/navigator-detached-no-crash.html (right):

https://codereview.chromium.org/301193006/diff/1/LayoutTests/fast/dom/navigat...
LayoutTests/fast/dom/navigator-detached-no-crash.html:50: }, 200);
On 2014/06/02 18:23:16, eseidel wrote:
> Bleh!  setTimeout tests = flaky tests. :(

I entirely agree but I prefer to not change the behaviour of the test. At least
not in that change.

[mlamouri (slow - plz ping), 2014-06-02 18:36:13]

The CQ bit was checked by mlamouri@chromium.org

[commit-bot: I haz the power, 2014-06-02 18:36:36]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/mlamouri@chromium.org/301193006/1

[commit-bot: I haz the power, 2014-06-02 20:18:52]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  mac_blink_rel on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/mac_blink_rel/builds/9901)
  linux_gpu_triggered_tests on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/linux_gpu_trigger...)

[commit-bot: I haz the power, 2014-06-02 20:19:48]

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

[commit-bot: I haz the power, 2014-06-02 20:19:49]

Try jobs failed on following builders:
  linux_gpu_triggered_tests on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/linux_gpu_trigger...)

[mlamouri (slow - plz ping), 2014-06-03 08:54:34]

The CQ bit was checked by mlamouri@chromium.org

[commit-bot: I haz the power, 2014-06-03 08:55:26]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/mlamouri@chromium.org/301193006/1

[commit-bot: I haz the power, 2014-06-03 11:04:43]

Change committed as 175342