Closed
Bug 585672
Opened 14 years ago
Closed 9 years ago
Make Panorama use Geometry.jsm
Categories
(Firefox Graveyard :: Panorama, defect, P4)
Firefox Graveyard
Panorama
Tracking
(Not tracked)
RESOLVED
WONTFIX
People
(Reporter: mitcho, Unassigned)
References
(Depends on 1 open bug)
Details
(Whiteboard: [cleanup][API])
Attachments
(2 obsolete files)
Use the Rect/Point that's provided (or, will soon be provided) with Geometry.jsm in lieu of those we create in our own utils.js
This may involve requesting a patch against the toolkit Geometry.jsm (bug 582865), if we need to add methods, etc.
Comment 1•14 years ago
|
||
Let's not do this until after our initial landing.
Reporter | ||
Comment 2•14 years ago
|
||
(In reply to comment #1)
> Let's not do this until after our initial landing.
Indeed.
Updated•14 years ago
|
Whiteboard: b5
Comment 3•14 years ago
|
||
Mass moving all Tab Candy bugs from Mozilla Labs to Firefox::Tab Candy. Filter the bugmail spam with "tabcandymassmove".
Product: Mozilla Labs → Firefox
Target Milestone: -- → ---
Reporter | ||
Comment 4•14 years ago
|
||
Attachment #466909 -
Flags: review?(gavin.sharp)
Attachment #466909 -
Flags: feedback?(ian)
Updated•14 years ago
|
QA Contact: tabcandy → tabcandy
Comment 5•14 years ago
|
||
Comment on attachment 466909 [details] [diff] [review]
Patch to use Geometry.jsm in tab view, getting rid of the Point and Rect classes provided in tabview. Includes a patch to Geometry.jsm to add a few additional methods.
Looks great! One small comment:
* You added .toString to the rect inside the assert at the top of GroupItem.setBounds; at that point we know it's not an actual Rect, so I figure letting the assert expand the object will give us more information relying on whatever .toString this mystery object may have.
Attachment #466909 -
Flags: feedback?(ian) → feedback+
Comment 6•14 years ago
|
||
Shouldn't this also add tests to the new methods added in Geometry.jsm?
Flags: in-testsuite?
Comment 7•14 years ago
|
||
Also, how about we add Rect.isRect() and Point.isPoint()?
Comment 8•14 years ago
|
||
Comment on attachment 466909 [details] [diff] [review]
Patch to use Geometry.jsm in tab view, getting rid of the Point and Rect classes provided in tabview. Includes a patch to Geometry.jsm to add a few additional methods.
Another topic of interest: we have objects in Tab Candy that are Rect-like, but not actual Rects (same with Points), and this needs to be taken into account. This happens in particular when saving Rects to sessionstore; their properties are saved to the JSON, but not their methods. When we get the Rect back out of sessionstore, it's just an object with 4 properties and no methods. It's important to note that you can use Rect.fromRect(fakeRect) to do this, but you can't use fakeRect.clone(). So:
* You (mitcho) have changed a number of new Rect(foo) calls to foo.clone() calls; please only use .clone in situations where you know it's a real Rect and not just Rect-like.
* Maybe we should do an audit and make sure that the only time we have Rect-like objects is when loading from JSON, and they get converted right away.
* Utils.isRect() (which I'm proposing we add to the new Rect) is actually a test to see whether an object is Rect-like; perhaps the name should be changed accordingly? isRectLike? isRecty? To test if it's a true Rect, we can use instanceof.
* On a related topic, all of the Rects in people's existing sessionstores are left, top, width, height. The new Rects are left, top, right, bottom. We need a little code at load time (for GroupItem, TabItem, UIManager) to check for the old style and convert to the new style.
That last point definitely needs to be fixed (if it isn't already... I didn't see anything for it though) before this patch lands; otherwise we'll break everyone's existing layouts.
Attachment #466909 -
Flags: feedback+ → feedback-
Comment 9•14 years ago
|
||
(In reply to comment #8)
> That last point definitely needs to be fixed (if it isn't already... I didn't
> see anything for it though) before this patch lands; otherwise we'll break
> everyone's existing layouts.
We can break stuff for beta testers if it doesn't leave things in an unmanageable or permanently bogus state.
Reporter | ||
Comment 10•14 years ago
|
||
- now uses instanceof Rect + Point in most places, explicitly calling Utils.isRecty and Utils.isPointy otherwise. .clone() is used where we know that we are dealing with a Real rect. Rect.fromRect is only used when we explicitly know it is Recty.
- storage data rects are converted from previous (width + height) style to the new (right + bottom) style
- added unit tests for new methods to Rect + Point
Attachment #466909 -
Attachment is obsolete: true
Attachment #467441 -
Flags: feedback?(ian)
Attachment #466909 -
Flags: review?(gavin.sharp)
Reporter | ||
Comment 11•14 years ago
|
||
(In reply to comment #10)
> Created attachment 467441 [details] [diff] [review]
> Patch v2, now with unit tests and some corrections based on feedback
FYI Ian, I have not asked for review of this patch because I believe it contains a bug whereby some (but not all!) tab item rects from storage are not used properly, resulting in degenerate tab items on startup. If you could look into why this is happening, I'd appreciate it.
Comment 12•14 years ago
|
||
Comment on attachment 467441 [details] [diff] [review]
Patch v2, now with unit tests and some corrections based on feedback
Mitcho, as far as I can tell in the new patch you're converting pageBounds, but not the bounds for TabItems or GroupItems. Perhaps this is what's causing the degenerate TabItems?
Also, from running in this partial state, you may have some leftover gunk in storage now; just something to keep in mind while testing.
Otherwise, the patch looks good.
Attachment #467441 -
Flags: feedback?(ian) → feedback-
Updated•14 years ago
|
Priority: -- → P4
Comment 13•14 years ago
|
||
Moving this to be in Post 4.0 world, and to be done when we do all-tabs.
Target Milestone: --- → Firefox 4.0
Updated•14 years ago
|
Target Milestone: Firefox 4.0 → Future
Reporter | ||
Updated•14 years ago
|
Assignee: mitcho → nobody
Whiteboard: b5 → [cleanup][API]
Reporter | ||
Updated•14 years ago
|
Blocks: 603789
Summary: Make tab view use Geometry.jsm → Make Panorama use Geometry.jsm
Reporter | ||
Updated•14 years ago
|
Attachment #467441 -
Attachment is obsolete: true
Reporter | ||
Comment 14•14 years ago
|
||
In the post-fx4-future, this should be done regardless of whether we move to all-tabs or not.
Comment 15•14 years ago
|
||
Tagging as doc needed, as this will resolve issues with identifier collisions that prevent Geometry.jsm from being particularly usable on desktop.
Keywords: dev-doc-needed
Reporter | ||
Comment 16•14 years ago
|
||
(In reply to comment #15)
> Tagging as doc needed, as this will resolve issues with identifier collisions
> that prevent Geometry.jsm from being particularly usable on desktop.
What exactly is the identifier issues that are forseen for using this on desktop?
Updated•13 years ago
|
Status: ASSIGNED → NEW
Comment 20•13 years ago
|
||
bugspam
(Fx7 was branched, removing open bugs from Fx7 meta bug, we won't create new meta bugs for upcoming Fx versions)
No longer blocks: 660175
Comment 21•9 years ago
|
||
Panorama has been removed from Firefox 45, currently in Beta and scheduled for release on March 7th. As such, I'm closing all existing Panorama bugs.
If you are still using Panorama, you will see a deprecation message in Firefox 44, and when 45 is released your tab group data will be migrated to bookmarks, with a folder for each group. There are also a few addons offering similar functionality.
See https://support.mozilla.org/en-US/kb/tab-groups-removal for more info.
We're removing Panorama because it has extremely low usage (about 0.01% of users), and has a large number of bugs and usability issues. The cost of fixing all those issues is far too high to justify, and so we'll instead be focusing our time and energy on improving other parts of Firefox.
Status: NEW → RESOLVED
Closed: 9 years ago
Resolution: --- → WONTFIX
Updated•9 years ago
|
Keywords: dev-doc-needed
Assignee | ||
Updated•9 years ago
|
Product: Firefox → Firefox Graveyard
You need to log in
before you can comment on or make changes to this bug.
Description
•