585672 - Make Panorama use Geometry.jsm

Status: RESOLVED WONTFIX

(Firefox Graveyard :: Panorama, defect, P4)

Description

[Michael Yoshitaka Erlewine [:mitcho]]

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

[Ian Gilman [:iangilman]]

Let's not do this until after our initial landing.

Comment 2

[Michael Yoshitaka Erlewine [:mitcho]]

(In reply to comment #1)
> Let's not do this until after our initial landing.

Indeed.

Comment 3

[(no longer active)]

Mass moving all Tab Candy bugs from Mozilla Labs to Firefox::Tab Candy.  Filter the bugmail spam with "tabcandymassmove".

Comment 4

[Michael Yoshitaka Erlewine [:mitcho]]

Attachment: 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.

Comment 5

[Ian Gilman [:iangilman]]

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.

Comment 6

[Shawn Wilsher :sdwilsh]

Shouldn't this also add tests to the new methods added in Geometry.jsm?

Comment 7

[Ian Gilman [:iangilman]]

Also, how about we add Rect.isRect() and Point.isPoint()?

Comment 8

[Ian Gilman [:iangilman]]

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.

Comment 9

[Dão Gottwald [:dao]]

(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.

Comment 10

[Michael Yoshitaka Erlewine [:mitcho]]

Attachment: Patch v2, now with unit tests and some corrections based on feedback

- 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

Comment 11

[Michael Yoshitaka Erlewine [:mitcho]]

(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

[Ian Gilman [:iangilman]]

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.

Comment 13

[Aza Raskin [:aza]]

Moving this to be in Post 4.0 world, and to be done when we do all-tabs.

Comment 14

[Michael Yoshitaka Erlewine [:mitcho]]

In the post-fx4-future, this should be done regardless of whether we move to all-tabs or not.

Comment 15

[Eric Shepherd [:sheppy]]

Tagging as doc needed, as this will resolve issues with identifier collisions that prevent Geometry.jsm from being particularly usable on desktop.

Comment 16

[Michael Yoshitaka Erlewine [:mitcho]]

(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?

Comment 17

[Kevin Hanes]

bugspam

Comment 18

[Tim Taubert [:ttaubert] (inactive)]

bugspam

Comment 19

[Tim Taubert [:ttaubert] (inactive)]

bugspam

Comment 20

[Tim Taubert [:ttaubert] (inactive)]

bugspam

(Fx7 was branched, removing open bugs from Fx7 meta bug, we won't create new meta bugs for upcoming Fx versions)

Comment 21

[Justin Dolske [:Dolske]]

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.