1168319 - Failed test_interfaces.html after enabling pointer events

Status: RESOLVED FIXED

(Firefox :: Untriaged, defect)

Description

[Maksim Lebedev]

INFO TEST-UNEXPECTED-FAIL  dom/tests/mochitest/general/test_interfaces.html
If this is failing: DANGER, are you sure you want to expose the new interface PointerEvent to all webpages as a property on the window (XBL: true)?

Comment 1

[Maksim Lebedev]

Try-server-build: https://treeherder.mozilla.org/#/jobs?repo=try&revision=b6965ff96a2d

Comment 2

[Maksim Lebedev]

Attachment: nightly_interfaces_ver1.diff

+ Changed  disable  ->  nightly

Suggestions and comments and objections are very welcome.

Comment 3

[Maksim Lebedev]

Without patch: https://treeherder.mozilla.org/#/jobs?repo=try&revision=81769199ab34
With patch: https://treeherder.mozilla.org/#/jobs?repo=try&revision=bed29e116900

Comment 4

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8610490 [details] [diff] [review]
nightly_interfaces_ver1.diff

Does this then fail on b2g and Android?

Comment 5

[Maksim Lebedev]

(In reply to Olli Pettay [:smaug] from comment #4)
> Does this then fail on b2g and Android?
Probability of failed tests very high, if such tests run on that platforms. I can re-run try-builds.

Comment 6

[Maksim Lebedev]

The main issue is inconsistence between enabling PE and support complicated description in tests.
Pointer events enabling:   NIGHTLY && (WIN || LINUX || OSX)
Description in tests:
> if ((entry.nightly === !isNightly) ||
>   (entry.xbl === !isXBLScope) ||
>   (entry.desktop === !isDesktop) ||
>   (entry.b2g === !isB2G) ||
>   (entry.windows === !isWindows) ||
>   (entry.mac === !isMac) ||
>   (entry.linux === !isLinux) ||
>   (entry.android === !isAndroid) ||
>   (entry.release === !isRelease) ||
>   (entry.permission && !hasPermission(entry.permission)) ||
>   entry.disabled) {
>   ...

Comment 7

[Maksim Lebedev]

Without patch: https://treeherder.mozilla.org/#/jobs?repo=try&revision=1e66eb0bd7ec
With patch: https://treeherder.mozilla.org/#/jobs?repo=try&revision=7a11ebab9cd1

Comment 8

[Maksim Lebedev]

Try-testing (on all platforms) showed results: 32 / 5. It is very good I think.

Comment 9

[Maksim Lebedev]

If there are no objections, I will put checkin-needed flag...
...at the same time as bug 1166347 will be checkined too.

Comment 10

[Maksim Lebedev]

If there are no objections, I put checkin-needed flag.

Comment 11

[Pulsebot]

https://hg.mozilla.org/integration/fx-team/rev/00ed09e42a00

Comment 12

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/00ed09e42a00

Comment 13

[Boris Zbarsky [:bzbarsky]]

> Pointer events enabling:   NIGHTLY && (WIN || LINUX || OSX)

Wait, that's not what try is showing.  Try is showing that window.PointerEvent exists on android and b2g emulator as well (which is why just putting "nightly: true" in the test file makes the test pass).  So if the intent is that pointer events only be enabled on desktop, then that intent is failing, no?

Have you checked an actual b2g/android nightly to see whether pointer events are enabled there outside the test harness?

Fwiw, if the intent is in fact nightly and desktop only, then the test file should have:

  nightly: true, desktop: true

is all.

Comment 14

[Maksim Lebedev]

(In reply to Not doing reviews right now from comment #13)
> Have you checked an actual b2g/android nightly to see whether pointer events
> are enabled there outside the test harness?
Could you please see comment 7?
Try servers builds have target: "try: -b do -p all -u mochitests -t none"
Is it not enought?
> Fwiw, if the intent is in fact nightly and desktop only, then the test file
> should have:
>   nightly: true, desktop: true
> is all.
According to comment 6 they have comparisons with "OR" statements.

Have you link to try-servers-build with errors?

Comment 15

[Boris Zbarsky [:bzbarsky]]

> Could you please see comment 7?

I did before making my comment, sure.

The "without patch" link in comment 7 clearly shows that window.PointerEvent is exposed on android and b2g.  See M12 on android and M8/M15 on b2g ICS Emulator opt/debug.

> Is it not enought?

It's enough, and it's showing that pointer events are currently enabled on b2g and android.  That's also why the test run with "nightly: true" without any platform qualification passes!

> According to comment 6 they have comparisons with "OR" statements.

Sure.  If any of those conditions tests true the interface is expected to NOT be exposed.

So if you do "nightly: true, desktop: true" then that means we expect the interface to not be exposed if "true === !isNightly || true === !isDesktop".  Which is the same as expecting it to be exposed in the boolean negation of that, which is "true !== !isNightly && true !== !isDesktop", and since isNightly and isDesktop are both known to be booleans, that's the same as "isNightly && isDesktop".

Basically, the intent of the test is that all the conditions listed in the entry must test as listed for the interface to be exposed and the not-listed conditions are allowed to test either true or false.

> Have you link to try-servers-build with errors?

Your first link from comment 7.

I mean, currently things are green so the test annotations match reality (at least in the test harness, which may or may not be setting prefs we don't set in the real world).  The test harness reality is that pointer events are exposed on all platforms.  If this does not match your expected real-world behavior, then either we have a bug and are exposing on all platforms in the real world or the test harness is not matching the real world, yes?

Comment 16

[Boris Zbarsky [:bzbarsky]]

OK, I just checked and XP_LINUX is defined on Android and b2g.  So this condition in all.js:

  #if defined(XP_WIN) || defined(XP_LINUX) || defined(XP_MACOSX)

tests true on every single one of our tier1 platforms.

Comment 17

[Olli Pettay [:smaug][[e-mail]]]

Sorry, my bad review.

Comment 18

[Maksim Lebedev]

(In reply from comment #16)
> OK, I just checked and XP_LINUX is defined on Android and b2g.
According to comment can I suppose that: XP_WIN || XP_LINUX || XP_MACOSX === "everywhere"
It there any platforms which have no that definitions?

(In reply from comment #13)
>   nightly: true, desktop: true
TRY-build for that changes: https://treeherder.mozilla.org/#/jobs?repo=try&revision=570b446aeab7

Comment 19

[Boris Zbarsky [:bzbarsky]]

> It there any platforms which have no that definitions?

Solaris?  BSD?  Various other things, probably.  But that ifdef covers all our tier1 platforms, yes, like I said in comment 16.

> TRY-build for that changes

Right, this is now failing on Android and b2g, because the interface is exposed there and shouldn't be.

What I would recommend is nixing the platform ifdef in all.js but adding explicit false values of the pref in question to b2g.js and mobile.js.

Comment 20

[Boris Zbarsky [:bzbarsky]]

Backed out in https://hg.mozilla.org/mozilla-central/rev/98820360ab66 because bug 1166347 also got backed out.  In general, it would be best to land orange fixes in the same bug as the thing that caused the orange, so backouts of that bug will also back them out...

Comment 21

[Maksim Lebedev]

Next patches will be attached into corresponding bug 1166347.