976563 - Talos rck2 (300%+) regression due to bug 941995

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: Toolbar, defect)

Description

[Kartikaya Gupta (email:[e-mail])]

I landed bug 941995 and got Talos regression notifications (http://mzl.la/1erXQgn and http://mzl.la/1fAUr3v) that indicated regressions from that change to the rck2 test.

The rck2 test loads a canned CNN page and simulates a bunch of user-driven panning/zooming actions on it. The CNN page has a meta-viewport tag [1] that says width=1024, so the patches on bug 941995 should only make a difference if the device the test is running on has a screen width of 1024 or higher. (IIRC the Tegras satisfy this criteria - gbrown, can you confirm?).

Assuming this is what is happening, we have three options that I can think of:
1) Do nothing - declare the user input now performs a different set of user actions than before by design, and declare a new baseline value for the test
2) Change the meta-viewport tag in the canned CNN page to have a wider viewport width, thereby restoring the double-tap-to-zoom behaviour on it. However changing the viewport width will probably again mean that pinches and such will behave differently than they did before, and so there is no guarantee that this will improve the numbers at all. It's basically trading one random baseline for another.
3) Implement some "escape mechanism" as wesj suggested at [2] and updated the canned CNN page to use it. This will likely bring us our old numbers back.

[1] http://hg.mozilla.org/build/talos/file/7674ffd4b494/talos/startup_test/fennecmark/cnn/cnn.com/index.html#l14
[2] https://bugzilla.mozilla.org/show_bug.cgi?id=941995#c4

Comment 1

[Geoff Brown [:gbrown]]

The tegra screen width is 1024. (1024 x 768)

Comment 2

[Kartikaya Gupta (email:[e-mail])]

Thanks.

Wes, did you have any thoughts on the "escape mechanism" to get back the double-tap delay?

Comment 3

[Wesley Johnston (:wesj)]

So just to be clear, the problem is that the tests test double-tap zoom and because of the new viewport stuff, double tap is disabled on it?

I think we should just add a (-moz-?)enable-double-tap-zoom arg to the viewport tag. If its set, double tap is enabled, regardless of other tags like user-scalable or width.

Comment 4

[Kartikaya Gupta (email:[e-mail])]

(In reply to Wesley Johnston (:wesj) from comment #3)
> So just to be clear, the problem is that the tests test double-tap zoom and
> because of the new viewport stuff, double tap is disabled on it?

Yeah, basically. The talos test simulates user input sequences which includes double-tap, and that doesn't work any more.

> I think we should just add a (-moz-?)enable-double-tap-zoom arg to the
> viewport tag. If its set, double tap is enabled, regardless of other tags
> like user-scalable or width.

Hm, ok. I don't have any particularly strong feelings about this either way. Adding mbrubeck also in case he has any objections.

Comment 5

[Wesley Johnston (:wesj)]

FWIW, I think we could just alter the test here though.

I've wondered if its users on tablets would be more frustrated with us disabling double-tap zoom there...

Comment 6

[Kartikaya Gupta (email:[e-mail])]

Alter the test in what way? If you're referring to option (2) in comment 0 then I don't think that's going to help with the numbers and I would rather just leave everything as-is.

Comment 7

[Geoff Brown [:gbrown]]

It looks like this is now on aurora:

Regression: Mozilla-Aurora - Robocop Checkerboarding Real User Benchmark - Android 2.2 (Native) - 791% increase
---------------------------------------------------------------------------------------------------------------
    Previous: avg 2.738 stddev 0.272 of 12 runs up to revision 07f5580d8a54
    New     : avg 24.397 stddev 4.346 of 12 runs since revision c0befb3c8038
    Change  : +21.659 (791% / z=79.541)
    Graph   : http://mzl.la/1dg6mp5

Changeset range: http://hg.mozilla.org/releases/mozilla-aurora/pushloghtml?fromchange=07f5580d8a54&tochange=c0befb3c8038

Comment 8

[Joel Maher ( :jmaher ) (UTC -8)]

are we concerned about this?  I want to make sure we address this during Aurora instead of waiting for we uplift to Beta.

Comment 9

[Kartikaya Gupta (email:[e-mail])]

I am not. I think we should just accept the new baseline value and close this bug.

Comment 10

[Joel Maher ( :jmaher ) (UTC -8)]

we don't have a new baseline anymore, we have a bunch of random data points:
http://graphs.mozilla.org/graph.html#tests=[[201,52,20],[201,63,20]]&sel=none&displayrange=90&datatype=running

So at the moment this test is not very useful- should we fix the test, or disable it?

Comment 11

[Wesley Johnston (:wesj)]

I mostly agree with kats. I don't have time to dig into this right now, and given we know why it happened, its not a high priority for me.

I don't really know where this test is though. Can we just disable the doubletap zoom part of this?

Comment 12

[Joel Maher ( :jmaher ) (UTC -8)]

here is the test:
http://dxr.mozilla.org/mozilla-central/source/mobile/android/base/tests/testCheck2.java

Comment 13

[Kartikaya Gupta (email:[e-mail])]

(In reply to Joel Maher (:jmaher) from comment #10)
> we don't have a new baseline anymore, we have a bunch of random data points:
> http://graphs.mozilla.org/graph.html#tests=[[201,52,20],[201,63,
> 20]]&sel=none&displayrange=90&datatype=running
> 
> So at the moment this test is not very useful- should we fix the test, or
> disable it?

Ah, I hadn't seen that! Yeah in that case we should do something about it.

Comment 14

[Kartikaya Gupta (email:[e-mail])]

Attachment: Force-enable double-tap zooming in the talos test

Pending try run at https://tbpl.mozilla.org/?tree=Try&rev=6906614ed8bc to verify it restores the old baseline.

I also pushed https://tbpl.mozilla.org/?tree=Try&rev=63935199056b earlier which disables the code that introduced this change, to verify that there weren't any other talos regressions in the meantime, and to make sure the attached patch results in the same behaviour.

Comment 15

[Wesley Johnston (:wesj)]

Comment on attachment 8401005 [details] [diff] [review]
Force-enable double-tap zooming in the talos test

Review of attachment 8401005 [details] [diff] [review]:
-----------------------------------------------------------------

Oh nice!

Comment 16

[Kartikaya Gupta (email:[e-mail])]

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

Comment 17

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/af8cb94d8904

Comment 18

[Kartikaya Gupta (email:[e-mail])]

Comment on attachment 8401005 [details] [diff] [review]
Force-enable double-tap zooming in the talos test

[Approval Request Comment]
Bug caused by (feature/regressing bug #): bug 941995
User impact if declined: this talos regression might hide other real regressions. this regression itself has no direct user impact as it is test-only
Testing completed (on m-c, etc.): on m-c now
Risk to taking this patch (and alternatives if risky): low risk, test only
String or IDL/UUID changes made by this patch: none

Comment 19

[Joel Maher ( :jmaher ) (UTC -8)]

I am not seeing any difference in the results yet, also are we looking to backport this to Aurora, as this is a test only it doesn't seem critical, but we should just disable this test on firefox 30 as it is not yielding anything useful right now.

Comment 20

[Kartikaya Gupta (email:[e-mail])]

I got emails from the bot saying things were better:

http://graphs.mozilla.org/graph.html#tests=[[201,64,20]]&sel=1396800459000,1396973259000&displayrange=7&datatype=running

http://graphs.mozilla.org/graph.html#tests=[[201,64,29]]&sel=1396800843000,1396973643000

Comment 21

[Kartikaya Gupta (email:[e-mail])]

And I would rather uplift the fix so that it doesn't obscure other real regressions from future uplifts.

Comment 22

[Joel Maher ( :jmaher ) (UTC -8)]

oh, you win!

Comment 23

[Lukas Blakk [:lsblakk] use ?needinfo]

Comment on attachment 8401005 [details] [diff] [review]
Force-enable double-tap zooming in the talos test

this can be landed as a=test-only

Comment 24

[Kartikaya Gupta (email:[e-mail])]

https://hg.mozilla.org/releases/mozilla-aurora/rev/77cb0b448bb2