Use a pref to keep track of Panorama "first run" experience

VERIFIED FIXED in Firefox 4.0b7

Status

defect
P3
normal
VERIFIED FIXED
9 years ago
3 years ago

People

(Reporter: mitcho, Assigned: mitcho)

Tracking

Trunk
Firefox 4.0b7
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 4 obsolete attachments)

Right now I have "first run" set to go off every time you open a new  window and you don't have any groups. We need a better rule.  Store a "we've shown firstrun" flag in Prefs; if it's not there, assume we haven't yet.
Posted patch Proposed patch (obsolete) — Splinter Review
Attachment #471648 - Flags: review?(dolske)
Attachment #471648 - Flags: feedback?(ian)
Comment on attachment 471648 [details] [diff] [review]
Proposed patch

Canceling review. We will commit this to tabcandy-central and get it reviewed and landed as part of bug 578512.
Attachment #471648 - Flags: review?(dolske)
Attachment #471648 - Flags: feedback?(ian)
Blocks: 594792
Comment on attachment 471648 [details] [diff] [review]
Proposed patch

We are no longer doing a t-c to m-c merge. Requesting feedback so we can review it.
Attachment #471648 - Flags: feedback?(ian)
Comment on attachment 471648 [details] [diff] [review]
Proposed patch

looks great.
Attachment #471648 - Flags: review?(dolske)
Attachment #471648 - Flags: feedback?(ian)
Attachment #471648 - Flags: feedback+
No longer depends on: 578512
Attachment #471648 - Flags: review?(dolske)
Attachment #471648 - Flags: review+
Attachment #471648 - Flags: approval2.0+
I'm going to write a test for this before landing.
Comment on attachment 471648 [details] [diff] [review]
Proposed patch

Random thought: what if someone sets the pref to false manually, after they already have a number of groups? As written, we will create a new group and pull all of their tabs into it, but leave the other groups around (as long as they have titles). 

I wouldn't say this scenario warrants a lot of effort, but maybe we could do something simple to address it.
Blocks: 597043
Priority: -- → P3
Target Milestone: --- → Firefox 4.0
Posted patch Patch for checkin (obsolete) — Splinter Review
Patch for checkin, with a test... BUT the try tree is closed so I can't send it for a sanity test. Will push to try whenever I can.
Attachment #471648 - Attachment is obsolete: true
Blocks: 595804
Posted patch Proposed patch v2 (obsolete) — Splinter Review
We need to ensure that we create a new group and put all orphan tab items in if no group data is available even after the first run.

Pushed to try -> 35bb5ccd05d6
Attachment #476693 - Attachment is obsolete: true
Attachment #476855 - Flags: feedback?(mitcho)
Posted patch Patch v3 (obsolete) — Splinter Review
Thanks Raymond. I didn't like how we split up the grouping code and the first-run welcome infoitem code, so I pulled it all out into a utility function, UI.reset(), which perhaps some of our tests can benefit from as well.

Also, in the unlikely scenario it was firstTime but there's group info, it would display the infoitem but not do the grouping, so I fixed that.
Attachment #476855 - Attachment is obsolete: true
Attachment #476985 - Flags: feedback?(ian)
Attachment #476855 - Flags: feedback?(mitcho)
Pushed to try: dca4fb9b939b
Comment on attachment 476985 [details] [diff] [review]
Patch v3

Looks good, but let's destroy any existing groups in reset. Also, we can kill _reset to avoid confusion; it's out of date now anyway.
Attachment #476985 - Flags: feedback?(ian) → feedback+
Re-requesting review as the patch has changed sufficiently from the previously r+'ed patch.
Attachment #476985 - Attachment is obsolete: true
Attachment #476993 - Flags: review?(dolske)
Patch v3, test-interaction-wise exactly the same, passed try. Patch v4 also passes locally (as does patch v3). Just waiting on review, then.
Attachment #476993 - Flags: review?(dolske)
Attachment #476993 - Flags: review+
Attachment #476993 - Flags: approval2.0+
Pushed http://hg.mozilla.org/mozilla-central/rev/c9bf3375b202
Status: NEW → RESOLVED
Closed: 9 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: Firefox 4.0 → Firefox 4.0b7
Thanks Dolske! :D
verified with recent nightly trunk builds
Status: RESOLVED → VERIFIED
Product: Firefox → Firefox Graveyard
You need to log in before you can comment on or make changes to this bug.