Closed Bug 595395 Opened 9 years ago Closed 9 years ago

Add pin events to AllTabs.jsm

Categories

(Firefox Graveyard :: Panorama, defect, P3)

defect

Tracking

(Not tracked)

RESOLVED FIXED
Future

People

(Reporter: iangilman, Assigned: raymondlee)

References

Details

Attachments

(1 file, 3 obsolete files)

In bug 593871 we're listening to tab pin/unpin events ourselves, rather than going through AllTabs. Maybe we should add those events to AllTabs?
Yes.
Assignee: nobody → edilee
Priority: -- → P3
Blocks: 597324
It looks like Mardak hasn't made much progress, so I am punting this to the future.
Target Milestone: --- → Future
Attached patch v1 (obsolete) — Splinter Review
Re-factoring the code so no test is included.
Assignee: edilee → raymond
Status: NEW → ASSIGNED
Attachment #488831 - Flags: feedback?(ian)
(In reply to comment #3)
> Re-factoring the code so no test is included.
But the alltabs.jsm is changing functionality.
(In reply to comment #4)
> But the alltabs.jsm is changing functionality.

OK, I will add a test for the patch then. :-)
Comment on attachment 488831 [details] [diff] [review]
v1

Looks lovely!
Attachment #488831 - Flags: feedback?(ian) → feedback+
Note that this patch will also fix bug 597324.
Attached patch v1.1 (obsolete) — Splinter Review
With test
Attachment #488831 - Attachment is obsolete: true
Attachment #489095 - Flags: feedback?(ian)
Comment on attachment 489095 [details] [diff] [review]
v1.1

Seems like this test shouldn't access TabView at all... it should just load the AllTabs module and access it directly. 

For that matter, the test might as well be called browser_tabview_alltabs.js.
Attachment #489095 - Flags: feedback?(ian) → feedback-
Attached patch v1.1 (obsolete) — Splinter Review
Updated the test per Ian's comment
Attachment #489095 - Attachment is obsolete: true
Attachment #489401 - Flags: feedback?(ian)
Comment on attachment 489401 [details] [diff] [review]
v1.1

Very nice.
Attachment #489401 - Flags: feedback?(ian) → feedback+
Attachment #489401 - Flags: review?(dolske)
Summary: Add pin events to AllTabs.jsm? → Add pin events to AllTabs.jsm
Attachment #489401 - Flags: review?(dolske)
Attachment #489401 - Flags: review+
Attachment #489401 - Flags: approval2.0+
Attachment #489401 - Attachment is obsolete: true
Keywords: checkin-needed
Does it need a try run?
Sent that to try c19e54733ca2, waiting for the result.
Passed try!
http://hg.mozilla.org/mozilla-central/rev/91465c3b341b
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Product: Firefox → Firefox Graveyard
You need to log in before you can comment on or make changes to this bug.