Closed
Bug 591705
Opened 14 years ago
Closed 14 years ago
Don't animate on tab view startup
Categories
(Firefox Graveyard :: Panorama, defect, P3)
Firefox Graveyard
Panorama
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.
Assignee | ||
Comment 1•14 years ago
|
||
Attachment #470230 -
Flags: review?(dolske)
Assignee | ||
Comment 2•14 years ago
|
||
Attachment #470230 -
Attachment is obsolete: true
Attachment #470251 -
Flags: review?(dolske)
Attachment #470230 -
Flags: review?(dolske)
Comment 3•14 years ago
|
||
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+
Assignee | ||
Comment 4•14 years ago
|
||
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)
Assignee | ||
Updated•14 years ago
|
Attachment #471496 -
Flags: review?(dolske) → review?(dietrich)
Assignee | ||
Comment 5•14 years ago
|
||
Comment on attachment 471496 [details] [diff] [review] Patch v3 Dietrich, note patch v2 (which is basically the same) received feedback+ from Ian.
Updated•14 years ago
|
Attachment #471496 -
Flags: review?(dietrich)
Attachment #471496 -
Flags: review+
Attachment #471496 -
Flags: approval2.0+
Assignee | ||
Comment 6•14 years ago
|
||
Attachment #471496 -
Attachment is obsolete: true
Assignee | ||
Updated•14 years ago
|
Keywords: checkin-needed
Comment 7•14 years ago
|
||
http://hg.mozilla.org/mozilla-central/rev/66d12a1541f2
Comment 8•14 years ago
|
||
And backed out due to Moth test failures.
Assignee | ||
Comment 9•14 years ago
|
||
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
Comment 10•14 years ago
|
||
This patch fixes the test failure which happened because of this patch.
Attachment #472632 -
Flags: review?(dolske)
Updated•14 years ago
|
Attachment #472632 -
Flags: review?(dolske) → review+
Assignee | ||
Comment 11•14 years ago
|
||
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.
Assignee | ||
Comment 12•14 years ago
|
||
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 13•14 years ago
|
||
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.
Assignee | ||
Comment 14•14 years ago
|
||
(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...
Assignee | ||
Comment 15•14 years ago
|
||
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 16•14 years ago
|
||
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+
Assignee | ||
Comment 17•14 years ago
|
||
(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.
Assignee | ||
Comment 18•14 years ago
|
||
Attachment #473625 -
Attachment is obsolete: true
Assignee | ||
Comment 19•14 years ago
|
||
Botched try run... going to push again...
Comment 20•14 years ago
|
||
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.
Assignee | ||
Updated•14 years ago
|
Priority: -- → P3
Assignee | ||
Comment 21•14 years ago
|
||
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
Comment 22•14 years ago
|
||
So where does this patch stand? Is it ready to go? Does it need to land after bug 595804? Is there more work needed?
Assignee | ||
Comment 23•14 years ago
|
||
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.
Assignee | ||
Comment 24•14 years ago
|
||
Pushed to try: e6d0c2d8bf07 Patch expects bug 595804 patch v4.
Attachment #473662 -
Attachment is obsolete: true
Assignee | ||
Comment 25•14 years ago
|
||
Passed try!
Updated•14 years ago
|
Status: NEW → ASSIGNED
Updated•14 years ago
|
Attachment #478789 -
Flags: approval2.0?
Updated•14 years ago
|
blocking2.0: --- → betaN+
Updated•14 years ago
|
Attachment #478789 -
Flags: approval2.0?
Assignee | ||
Comment 26•14 years ago
|
||
Sent to try again to verify tests before requesting checkin.
Attachment #478789 -
Attachment is obsolete: true
Assignee | ||
Comment 28•14 years ago
|
||
http://hg.mozilla.org/mozilla-central/rev/36dad8649310
Updated•14 years ago
|
Whiteboard: [qa-]
Updated•8 years ago
|
Product: Firefox → Firefox Graveyard
You need to log in
before you can comment on or make changes to this bug.
Description
•