Closed Bug 976563 Opened 10 years ago Closed 10 years ago

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

Categories

(Firefox for Android Graveyard :: Toolbar, defect)

Product:
Firefox for Android Graveyard
Component:
Toolbar
Platform:
All
Android
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

(firefox29 unaffected, firefox30 fixed, firefox31 fixed, fennec30+)

Status:
RESOLVED FIXED
Milestone:
Firefox 31
Tracking Flags:
Tracking Status
firefox29 --- unaffected
firefox30 --- fixed
firefox31 --- fixed
fennec 30+ ---

People

(Reporter: kats, Assigned: kats)

References

Details

(Whiteboard: [talos_regression])

Attachments

(1 file)

Force-enable double-tap zooming in the talos test
1.42 KB, patch
wesj
: review+
Details | Diff | Splinter Review 
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
The tegra screen width is 1024. (1024 x 768)
Thanks.

Wes, did you have any thoughts on the "escape mechanism" to get back the double-tap delay?
Flags: needinfo?(wjohnston)
Whiteboard: [talos_regression]
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.
(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.
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...
Flags: needinfo?(wjohnston)
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.
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
tracking-fennec: --- → ?
Assignee: nobody → wjohnston
tracking-fennec: ? → 30+
are we concerned about this?  I want to make sure we address this during Aurora instead of waiting for we uplift to Beta.
I am not. I think we should just accept the new baseline value and close this bug.
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?
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?
(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.
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.
Assignee: wjohnston → bugmail.mozilla
Attachment #8401005 - Flags: review?(wjohnston)
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!
Attachment #8401005 - Flags: review?(wjohnston) → review+
https://hg.mozilla.org/mozilla-central/rev/af8cb94d8904
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 31
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
Attachment #8401005 - Flags: approval-mozilla-aurora?
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.
And I would rather uplift the fix so that it doesn't obscure other real regressions from future uplifts.
oh, you win!
Comment on attachment 8401005 [details] [diff] [review]
Force-enable double-tap zooming in the talos test

this can be landed as a=test-only
Attachment #8401005 - Flags: approval-mozilla-aurora?
Product: Firefox for Android → Firefox for Android Graveyard
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: