Bug 962677 changed the customize mode transition so that we animate the content deck and all toolbars (except for the tabs toolbar). We need to update the CART test so that it's monitoring the right things (it's still watching the tab-view-deck, and so our error and half measurements are all wrong).

Comment on attachment 8370095 [details] [diff] [review] Patch v1 This appears to do the right thing. Did I miss a spot?

This looks good, however: - Currently the TART addon (which includes CART) detects animation-ens as whichever comes first: tab-animation-end (with max-width) or customization-end. - Do we want to include the tab animation at the CART result? if yes, then we should probably don't look at customization-end at all, and only look for tab animation end (which is also longer than customize - 230ms vs 150ms). - I noticed that on customize-enter, the tab opens together with customize, but on customize-exit, first customize animates, then the tab closes. Is that by design? If yes, do we want to include the tab animation at the CART report?

Comment on attachment 8370095 [details] [diff] [review] Patch v1 Review of attachment 8370095 [details] [diff] [review]: ----------------------------------------------------------------- And thanks for the typo fix :) However, per comment 3, we still need to decide what do we want to measure, and potentially modify more code based on this decision.

(In reply to Avi Halachmi (:avih) from comment #3) > This looks good, however: > > - Currently the TART addon (which includes CART) detects animation-ens as > whichever comes first: tab-animation-end (with max-width) or > customization-end. > > - Do we want to include the tab animation at the CART result? if yes, then > we should probably don't look at customization-end at all, and only look for > tab animation end (which is also longer than customize - 230ms vs 150ms). > Having thought about this, I think it'd actually be most useful to attempt to capture the transition up until both the "customizationready" event (distinct from customization-transitionend) has fired on the gToolbox *and* the tab has finished opening. > - I noticed that on customize-enter, the tab opens together with customize, > but on customize-exit, first customize animates, then the tab closes. Is > that by design? If yes, do we want to include the tab animation at the CART > report? Yes, this is by design. I think because this happens sequentially like this, capturing the tab animation finish is less important on customize exit. I think waiting for the "aftercustomization" to be fired from the gToolbox is enough. Let me see if I can whip up a patch to do this.

Ok, this patch is a little more substantial, and makes it so that we wait for both the tab transition to finish, along with the "customizationready" event to fire on the gNavToolbox. It uses Promise.jsm and Promise.all to pull that off, and so I've modified _animate to expect one of these Deferred things I've constructed. These Deferred things, once instantiated, resolve their internal promises once a particular event fires and certain conditions are met. Hopefully my documentation clears up their function and purpose. Thoughts?

Comment on attachment 8370880 [details] [diff] [review] Patch v2 Review of attachment 8370880 [details] [diff] [review]: ----------------------------------------------------------------- This works. I'm not terribly happy with adding yet another argument to 'animate' (I'm the one to blame for it having many args to begin with). Also, this generalization lends itself well to an end-trigger which is an AND of few events, but I'm not sure how easy it would be to adapt it to a different logical expression of conditions.

This is another approach. Not sure if it's better than the other one, but we can discuss it.

Few issues: - Customize-enter has a reference duration of 150ms, but since it includes a tab open animation (and the measurement waits until both are done), unless that tab open is set to 150ms, we can't expect to be able to tell the difference if the customize animation finished on time (150ms), or had an accumulated 80ms lag (to 230ms which is the current tab animation duration) or possibly even more - if tab animation takes longer than its designated duration (and it usually does). IMO we can detect the end using only 'customizationready'. This will prevent the tab animation from masking the customize animation duration. I tested it, and currently the results are the same (i.e. customizationready always arrives after the tab animation is done). If in the future it'll finish before the tab animation - even better. - I noticed a discrepancy where the duration from start-trigger to end-trigger is longer than the accumulation of frames intervals. On tab animation it's negligible (2 ms), but with customize animation it can be long - about 20ms on customize-enter and almost 60ms on customize leave. This means that the .error value should be even higher than currently indicated by the measurement (which accumulates the intervals). My current guess is that possibly many things happen around the end-event which take time but not screen updates. This still affects the user, so I think I'll modify the .error value to relate to the overall duration rather than to the sum of intervals. - The numbers I measured on a 2010 MBA are far from the reference durations. Customize-enter takes more than 800ms (where theoretically it should take 150, or 230 with the tab animation), and customize-leave is about 450ms. If we aim at less than 200ms animation but end up with the user waiting for almost a second, then it's something we should be aware of.

(In reply to Avi Halachmi (:avih) from comment #9) > - The numbers I measured on a 2010 MBA are far from the reference durations. > Customize-enter takes more than 800ms (where theoretically it should take > 150, or 230 with the tab animation), and customize-leave is about 450ms. If > we aim at less than 200ms animation but end up with the user waiting for > almost a second, then it's something we should be aware of. Right, that *is* pretty slow - but remember that the CSS transition is only one part of a pretty major shift in the UI, and there's a bunch of other stuff happening, such as reparenting a bunch of toolbar items in order to make them customizable, etc. The customizationready event waits until all of those things are done. I think the reparenting is where most of the time is going there - and we can work on reducing it over time.

Comment on attachment 8371867 [details] [diff] [review] Different generalization approach (also with the new customize triggers) Review of attachment 8371867 [details] [diff] [review]: ----------------------------------------------------------------- I'm fine with this approach as well.

This is the same approach as comment 8, with the following differences: - Enhanced TART to support sop-recording-but-still-wait-before-next-animation. - Added another sub-test for customize-enter where it only measures the CSS animation (and then waits for customizationready). - Modified the .error measurement to take into account the absolute duration rather than the sum of intervals (the latter could be shorter). This will also make TART .error values more accurate (and slightly higher). We'll also need to add the new test names to the graphserver DB.

Comment on attachment 8374868 [details] [diff] [review] Split CART into two tests: one full, one for the CSS animation only Review of attachment 8374868 [details] [diff] [review]: ----------------------------------------------------------------- I think this is better than what we had before, and gives us the numbers we need, so let's roll with it. Thanks Avi! ::: talos/page_load_test/tart/addon/content/tart.js @@ +258,5 @@ > sumMost += intervals[i]; > countMost++; > } > } > + dump("overall: " + sum + "\n"); Should remove this. @@ +424,5 @@ > var fadeout = this.fadeOutCurrentTab.bind(this); > > var addSomeTab = this.addSomeChromeUriTab.bind(this); > var cutsomizeEnter = this.triggerCustomizeEnter.bind(this); > + var cutsomizeEnterCss = this.triggerCustomizeEnterCss.bind(this); Might as well change cutsomize -> customize too.

Comment on attachment 8374868 [details] [diff] [review] Split CART into two tests: one full, one for the CSS animation only Review of attachment 8374868 [details] [diff] [review]: ----------------------------------------------------------------- this is great- some general questions/comments as I am not an expert in this code. ::: talos/page_load_test/tart/addon/content/tart.js @@ +50,5 @@ > + }, > + > + customizeEnterDetector: { > + arm: function(handler, win) { > + win.gNavToolbox.addEventListener("customizationready", handler); I assume we are supposed to use gNavToolbox here vs gBrowser? @@ +289,5 @@ > + return; > + } > + } else { > + // No event == timeout > + dump("TART: TIMEOUT\n"); not sure if this will be useful in automation. @@ +620,5 @@ > ], > > customize: [ > + // Test australis customize mode animation with default DPI. > + function(){Services.prefs.setCharPref("layout.css.devPixelsPerPx", "-1"); next();}, would we want to set this in the test.py file? I like doing it there so we know what preferences are required at configure time.

(In reply to Joel Maher (:jmaher) from comment #14) > I assume we are supposed to use gNavToolbox here vs gBrowser? Yes. According to fx-team (mconley) this is what we should be listening on. Each animation has its own listeners setup, and this is required and works for this animation. > > + dump("TART: TIMEOUT\n"); > > not sure if this will be useful in automation. It will be at the logs (if we have a timeout), and hurts nothing otherwise. TART logs have already proven useful in the past since they convey more info than the numbers-only which are the test result. > > + function(){Services.prefs.setCharPref("layout.css.devPixelsPerPx", > > would we want to set this in the test.py file? I like doing it there so we > know what preferences are required at configure time. Not black and white here, but overall I think it's more consistent if prefs which apply to all sub tests (like ASAP mode, or in the past OMTC off) are set at test.py, while prefs which differ between subtests are setup (and restored) within the addon. This division also lets us run different subtests at the same browser instance, instead of launching a new instance with one/few pref modified for each subtest (e.g. TART runs about 6 different subtests, with this or other prefs modified). This division also makes it easier to use the TART addon as stand-alone, since other than manually setting ASAP mode (which requires restart anyway), the addon takes care of required prefs and restores them to their original state when done.

I was not thinking about the standalone case. Possibly a comment in test.py where we define cart to indicate that cart will set its own preferences as needed? thanks for the good patch.

Carrying r+. @jmaher: this should land after bug 974336.

landed on talos: http://hg.mozilla.org/build/talos/rev/fc8a25eabc6a testing on try- planning to deploy on inbound tomorrow.

deployed on inbound.