Closed Bug 591705 Opened 9 years ago Closed 9 years ago

Don't animate on tab view startup

Categories

(Firefox Graveyard :: Panorama, defect, P3)

defect

Tracking

(blocking2.0 betaN+)

RESOLVED FIXED
Tracking Status
blocking2.0 --- betaN+

People

(Reporter: mitcho, Assigned: mitcho)

References

Details

(Whiteboard: [qa-])

Attachments

(1 file, 9 obsolete files)

34.90 KB, patch
Details | Diff | Splinter Review
We should not be animating anything on tab view iframe startup.
Comment on attachment 470251 [details] [diff] [review]
Patch v2: found more instances where animate was being called and thwarted them with "immediately". Also, fixed a bug in how I tried to make the call to reconnect earlier.

Looks good, except that in positionNewTabAtBottom() you need a:

  if (!options)
    options = {};

...before you access options.immediately.
Attachment #470251 - Flags: feedback+
Attached patch Patch v3 (obsolete) — Splinter Review
Touched up based on comments from Ian on v2. v2 received feedback+ from Ian.
Attachment #470251 - Attachment is obsolete: true
Attachment #471496 - Flags: review?(dolske)
Attachment #470251 - Flags: review?(dolske)
Attachment #471496 - Flags: review?(dolske) → review?(dietrich)
Comment on attachment 471496 [details] [diff] [review]
Patch v3

Dietrich, note patch v2 (which is basically the same) received feedback+ from Ian.
Attachment #471496 - Flags: review?(dietrich)
Attachment #471496 - Flags: review+
Attachment #471496 - Flags: approval2.0+
Attached patch Patch for checkin (obsolete) — Splinter Review
Attachment #471496 - Attachment is obsolete: true
And backed out due to Moth test failures.
This patch was/is the culprit of failing tests. I just verified that the series of patches which were committed together, *without* this patch (bug 591167, bug 591706, bug 592586, bug 591147, bug 587040, bug 588999) passes tests.

I will rerequest ride-along checkins for those other bugs.
Keywords: checkin-needed
Attached patch Test fix (obsolete) — Splinter Review
This patch fixes the test failure which happened because of this patch.
Attachment #472632 - Flags: review?(dolske)
Attachment #472632 - Flags: review?(dolske) → review+
There is indeed an issue with this patch which makes it so groups from storage can't be dragged etc., as Dietrich noted. Currently investigating.
The fix for groups from storage was to setZ on the item, even if we don't .snap it.
Attachment #471930 - Attachment is obsolete: true
Attachment #472632 - Attachment is obsolete: true
Attachment #473369 - Flags: review?(dietrich)
Comment on attachment 473369 [details] [diff] [review]
Patch with Ehsan's test fix as well as a fix to the issue where groups from storage couldn't be interacted with

looks fine, however i don't see how the test confirms that there's no animation when that option is used. can you add a test for that?

aside: i'd prefer some kind of global option for toggling animations in panorama, but that doesn't need to happen here.
(In reply to comment #13)
> looks fine, however i don't see how the test confirms that there's no animation
> when that option is used. can you add a test for that?

You're right. There's no test for that right now. I'll see if I can add one... I'm not entirely certain how that should work...
Attached patch Patch with test (obsolete) — Splinter Review
Added a test as requested.

This pointed out a few other places where we are animating on firstrun startup, so I also gave the "immediately" treatment to the group created during firstTime and to the InfoItem for the welcome video. Mochitest-other passes locally. Sending to try right now.
Attachment #473369 - Attachment is obsolete: true
Attachment #473625 - Flags: review?(dietrich)
Attachment #473369 - Flags: review?(dietrich)
Comment on attachment 473625 [details] [diff] [review]
Patch with test

>     // ___ Position
>-    this.snap();
>-
>-    // ___ Push other objects away
>     if (!options.dontPush)
>-      this.pushAway();
>+			this.snap(options.immediately);

fix indent

>@@ -92,16 +95,20 @@ function TabItem(tab) {
>   this.sizeExtra.y = parseInt($div.css('padding-top'))
>       + parseInt($div.css('padding-bottom'));
> 
>   this.bounds = $div.bounds();
> 
>   // ___ superclass setup
>   this._init($div[0]);
> 
>+  // ___ attempt to reconnect to data from Storage
>+  this._hasBeenDrawn = false;
>+  let reconnected = TabItems.reconnect(this);
>+

what could cause this to fail? how is it handled?

>@@ -1002,17 +1010,17 @@ let TabItems = {
>           setTimeout(function() {
>             if (item && item.isShowingCachedData) {
>               item.hideCachedData();
>             }
>           }, 15000);
>         }
> 
>         item.reconnected = true;
>-        found = true;
>+        found = {addedToGroup: tabData.groupID};
>       } else
>         item.reconnected = item.tab.linkedBrowser.currentURI.spec != 'about:blank';

how's this change related?

> _BROWSER_FILES = \
>                  browser_tabview_launch.js \
>                  browser_tabview_dragdrop.js \
>                  browser_tabview_group.js \
>                  browser_tabview_search.js \
>+                 browser_tabview_bug591705.js \
>                  $(NULL)

i find that directories full of "test_bug14321324.js" makes finding things harder. i recommend a naming scheme like "browser_bugXXXXXX_description.js", or maybe even leaving out the bug # (putting it in the test comments intead). i don't think you need the "tabview" because they're in the tabview directory structure. up to y'all though.

r=me otherwise
Attachment #473625 - Flags: review?(dietrich) → review+
(In reply to comment #16)
> Comment on attachment 473625 [details] [diff] [review]
> Patch with test
> >@@ -92,16 +95,20 @@ function TabItem(tab) {
> >   this.sizeExtra.y = parseInt($div.css('padding-top'))
> >       + parseInt($div.css('padding-bottom'));
> > 
> >   this.bounds = $div.bounds();
> > 
> >   // ___ superclass setup
> >   this._init($div[0]);
> > 
> >+  // ___ attempt to reconnect to data from Storage
> >+  this._hasBeenDrawn = false;
> >+  let reconnected = TabItems.reconnect(this);
> >+
> 
> what could cause this to fail? how is it handled?

It wasn't added here anew, as much as moved a tad earlier. This lets us know if the tab is a top-level element or within a group.

> >@@ -1002,17 +1010,17 @@ let TabItems = {
> >           setTimeout(function() {
> >             if (item && item.isShowingCachedData) {
> >               item.hideCachedData();
> >             }
> >           }, 15000);
> >         }
> > 
> >         item.reconnected = true;
> >-        found = true;
> >+        found = {addedToGroup: tabData.groupID};
> >       } else
> >         item.reconnected = item.tab.linkedBrowser.currentURI.spec != 'about:blank';
> 
> how's this change related?

We later check the return value of this to see, not just did we reconnect to storage, but did the storage assign this tab to a group. If we did, we don't need to do things like setResizeable and droppable, just to have it undone by the groupitem. setResizeable and droppable set up handlers with animations, so that's why it's related.

> > _BROWSER_FILES = \
> >                  browser_tabview_launch.js \
> >                  browser_tabview_dragdrop.js \
> >                  browser_tabview_group.js \
> >                  browser_tabview_search.js \
> >+                 browser_tabview_bug591705.js \
> >                  $(NULL)
> 
> i find that directories full of "test_bug14321324.js" makes finding things
> harder. i recommend a naming scheme like "browser_bugXXXXXX_description.js", or
> maybe even leaving out the bug # (putting it in the test comments intead). i
> don't think you need the "tabview" because they're in the tabview directory
> structure. up to y'all though.

Renamed to browser_tabview_startup_transitions.js

> r=me otherwise

Thanks! Will package it up now and wait for try results to request landing.
Attached patch Patch for checkin (obsolete) — Splinter Review
Attachment #473625 - Attachment is obsolete: true
Botched try run... going to push again...
Comment on attachment 473662 [details] [diff] [review]
Patch for checkin

wm.getMostRecentWindow("navigator:browser") doesn't seem to be necessary in the test because win is the same object as newWindow.
Priority: -- → P3
I think for my test to be able to detect this correctly, we have to remove the zoom in/out animation factor, which bug 595804 would let us do.
Depends on: 595804
Blocks: 597043
So where does this patch stand? Is it ready to go? Does it need to land after bug 595804? Is there more work needed?
Yes, 595804 will be required. Right now this greatly reduces the number of animate calls, but doesn't kill those in the zoomout so it doesnt pass it's own test.
Pushed to try: e6d0c2d8bf07

Patch expects bug 595804 patch v4.
Attachment #473662 - Attachment is obsolete: true
Passed try!
Status: NEW → ASSIGNED
Attachment #478789 - Flags: approval2.0?
blocking2.0: --- → betaN+
Attachment #478789 - Flags: approval2.0?
Sent to try again to verify tests before requesting checkin.
Attachment #478789 - Attachment is obsolete: true
Passed try. Checkin needed.
Keywords: checkin-needed
http://hg.mozilla.org/mozilla-central/rev/36dad8649310
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Whiteboard: [qa-]
Product: Firefox → Firefox Graveyard
You need to log in before you can comment on or make changes to this bug.