622835 - Stacked group display is broken if restored after firefox restart

Status: VERIFIED FIXED

(Firefox Graveyard :: Panorama, defect)

Description

[Tim Taubert [:ttaubert] (inactive)]

Attachment: Screenshot of broken stacked groups display

When TabCandy gets loaded after restarting Firefox all stacked groups that contain more than 6 items are 'broken' (see attached screenshot).

Comment 1

[Tim Taubert [:ttaubert] (inactive)]

Attachment: patch v1

I removed _randRotate() and _stackAngles completely because I couldn't see how they are needed.

1.) 'parseInt((Math.random()-.5)*1)' gives always '0' so _randRotate() is not as 'random' as it is named. parseInt() receives values in the range of [-0.5, 0.5[ so parseInt() truncates it to always '0' - or am I missing something here?

2.) For this simple calculation we don't need to store the angles in _stackAngles, do we?

Comment 2

[(mostly gone) XtC4UaLL [:xtc4uall]]

If i see correctly, already filed Bug 619161 can be duped against this one, no?

Comment 3

[Michael Yoshitaka Erlewine [:mitcho]]

Comment on attachment 501023 [details] [diff] [review]
patch v1

Needs a test, but otherwise f+.

Comment 4

[Michael Yoshitaka Erlewine [:mitcho]]

(In reply to comment #1)
> Created attachment 501023 [details] [diff] [review]
> patch v1
> 
> I removed _randRotate() and _stackAngles completely because I couldn't see how
> they are needed.

Just to elaborate: I agree that randRotate was not as random as it was supposed to be but, even then, I don't think we want to randomly fan out the stack to have a "I just threw these tabs on the desk" look any more.

CC: faaborg, in case he'd like to weigh in from a ux perspective.

Comment 5

[Tim Taubert [:ttaubert] (inactive)]

(In reply to comment #2)
> If i see correctly, already filed Bug 619161 can be duped against this one, no?

I didn't find this bug when I searched for this issue. The terms just differ completely from mine :) I think this bug can be duped against this one because we already have a patch here. Should I add 619385 as blocked by this bug?

Comment 7

[Tim Taubert [:ttaubert] (inactive)]

Attachment: patch v2 (added testcase)

Pushed to try. Passed.

Comment 8

[Dão Gottwald [:dao]]

Comment on attachment 501636 [details] [diff] [review]
patch v2 (added testcase)

>+  let url = getBrowserURL();
>+  let opts = 'chrome,all,dialog=no,height=800,width=800';
>+  let charset = 'charset=' + window.content.document.characterSet;
>+
>+  let win = window.openDialog(url, '_blank', opts, 'about:blank', charset,
>+                              null, null, true);

Why do you specify a height and width and why are you messing with the charset?

Comment 9

[Tim Taubert [:ttaubert] (inactive)]

(In reply to comment #8)
> Why do you specify a height and width and why are you messing with the charset?

Well, you're right this is unnecessary. Copied that code from another test.

Comment 10

[Tim Taubert [:ttaubert] (inactive)]

Attachment: patch v3 (cleaned up test)

Comment 11

[Michael Yoshitaka Erlewine [:mitcho]]

(In reply to comment #9)
> (In reply to comment #8)
> > Why do you specify a height and width and why are you messing with the charset?
> 
> Well, you're right this is unnecessary. Copied that code from another test.

Well, let's not reinvent the wheel. bug 610242 will land newWindowWithTabView into head.js which can be used in any group. Please use that function.

Note, Dāo, this function, newWindowWithTabView, has to specify a window bounds as it's used in tests which have to do with geometric layout and allowing the window size to vary based on the test machine resulted in inconsistent results.

Comment 12

[Tim Taubert [:ttaubert] (inactive)]

Attachment: patch v4 (test prepared for bug 610242)

Comment 13

[Alex Faaborg [:faaborg] (Firefox UX)]

>Just to elaborate: I agree that randRotate was not as random as it was supposed
>to be but, even then, I don't think we want to randomly fan out the stack to
>have a "I just threw these tabs on the desk" look any more.
>
>CC: faaborg, in case he'd like to weigh in from a ux perspective.

What was the advantage to using randRotate?  Just that it looks more natural and organic?

Comment 14

[Tim Taubert [:ttaubert] (inactive)]

v4 + v2.6 from bug 610242 pushed to try today. Passed.

Comment 15

[Michael Yoshitaka Erlewine [:mitcho]]

(In reply to comment #13)
> What was the advantage to using randRotate?  Just that it looks more natural
> and organic?

I think that was the idea.

Comment 16

[Ian Gilman [:iangilman]]

Comment on attachment 501785 [details] [diff] [review]
patch v4 (test prepared for bug 610242)

The fix looks good, but the test doesn't really test the bug as described. We can't actually restart the browser as part of a test, but we can simulate it by creating a window and either stuffing it full of session data as in these 2 tests:

browser_tabview_bug598600.js
browser_tabview_bug589324.js

... or closing it and then restoring it, as in this test:

browser_tabview_bug597248.js

The former is probably the most straight-forward, and should be fine for our purposes.

Note also that we don't need to test that the arrangement doesn't change... just that it's not in a broken state.

Comment 17

[Ian Gilman [:iangilman]]

(In reply to comment #15)
> (In reply to comment #13)
> > What was the advantage to using randRotate?  Just that it looks more natural
> > and organic?
> 
> I think that was the idea.

Yeah, that's leftover from a design exploration from a long time ago.

Comment 18

[Tim Taubert [:ttaubert] (inactive)]

(In reply to comment #16)
> Comment on attachment 501785 [details] [diff] [review]
> patch v4 (test prepared for bug 610242)
> 
> The fix looks good, but the test doesn't really test the bug as described.

Well, when I reported the bug I made a false assumption about the cause of this problem. When fixing this I found out that the first arrange() call for stacked groups after initializing the TabView was buggy. So the test just mimics a restored stacked group and then calls arrange(). So generating session data in the test is not really necessary I think?

> Note also that we don't need to test that the arrangement doesn't change...
> just that it's not in a broken state.

You're right but it's very difficult to check whether the arrangement is broken. So it was much easier to check that the result of the first arrange() call matches the result after the second arrange() call. If you have any good idea how to check this please let me know.

Comment 19

[Ian Gilman [:iangilman]]

Comment on attachment 501785 [details] [diff] [review]
patch v4 (test prepared for bug 610242)

(In reply to comment #18)
> Well, when I reported the bug I made a false assumption about the cause of this
> problem. When fixing this I found out that the first arrange() call for stacked
> groups after initializing the TabView was buggy. So the test just mimics a
> restored stacked group and then calls arrange(). So generating session data in
> the test is not really necessary I think?

Fair enough.

You've got a problem then: the resumeArrange call does an arrange already, so your two arrange calls are too late.

> You're right but it's very difficult to check whether the arrangement is
> broken. So it was much easier to check that the result of the first arrange()
> call matches the result after the second arrange() call. If you have any good
> idea how to check this please let me know.

Wouldn't it be as easy as checking if the rotation value of any of the tabs is above a certain threshold?

At any rate, given what the bug really was, this test is fine (once you fix the resumeArrange/arrange bit).

Comment 21

[Tim Taubert [:ttaubert] (inactive)]

Attachment: patch v5 (all issues fixed)

Fixed the resumeArrange() thing (see bug 624953). Rewrote the test to check if the 7th item is directly positioned under the 6th and therefore hidden.

Comment 22

[Tim Taubert [:ttaubert] (inactive)]

Attachment: patch v5b (minor test cleanup)

Comment 23

[Mike Beltzner [:beltzner, not reading bugmail]]

Comment on attachment 503048 [details] [diff] [review]
patch v5b (minor test cleanup)

a=beltzner

Comment 24

[Tim Taubert [:ttaubert] (inactive)]

Passed try (together with patch from bug 624953).

Comment 25

[Tim Taubert [:ttaubert] (inactive)]

Attachment: patch for checkin

Still waiting for bug 624953 to land.

Comment 26

[Tim Taubert [:ttaubert] (inactive)]

Ready for checkin. Please do only land together with/after bug 624953.

Comment 27

[(no longer active)]

http://hg.mozilla.org/mozilla-central/rev/e95126669d5e

Comment 28

[Tracy Walker [:tracy]]

verified with minefield build of 20110217