Closed Bug 1222490 Opened 9 years ago Closed 8 years ago

Actually remove panorama after warning + migration have landed

Categories

(Firefox Graveyard :: Panorama, defect)

defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED
Firefox 45

People

(Reporter: Gijs, Assigned: Gijs, NeedInfo)

References

Details

(Keywords: addon-compat, dev-doc-needed)

Attachments

(6 files)

Separate bug so mozreview can deal, and we can do trypushes and so on that we can use to test add-ons.

NB: not intended to land until we have the warning + migration in place.
Bug 1222490 - part 1: remove browser/components/tabview and its references, r?ttaubert
Attachment #8684273 - Flags: review?(ttaubert)
Bug 1222490 - part 2: remove traces of tabview from XUL/XBL/JS in other parts of browser/, r?ttaubert
Attachment #8684274 - Flags: review?(ttaubert)
Bug 1222490 - part 3: update all the tests for tabview's removal, r?ttaubert
Attachment #8684275 - Flags: review?(ttaubert)
Bug 1222490 - part 4: remove tabview CSS references, r?ttaubert
Attachment #8684276 - Flags: review?(ttaubert)
Bug 1222490 - Part 5: remove now-unused panorama/tabview strings, r?ttaubert
Attachment #8684277 - Flags: review?(ttaubert)
Bug 1222490 - part 6: remove miscellaneous other bits and bobs referring to panorama/tabview/tab groups, r?ttaubert
Attachment #8684278 - Flags: review?(ttaubert)
Assignee: nobody → gijskruitbosch+bugs
Status: NEW → ASSIGNED
(In reply to :Gijs Kruitbosch from comment #7)
> remote:  
> https://treeherder.mozilla.org/#/jobs?repo=try&revision=a0eba022ca5f

Hrmpf. Well, 1 test failure is not so bad considering this was my first pass...

remote:   https://treeherder.mozilla.org/#/jobs?repo=try&revision=75cedc2dbdd4

(also rebased on top of bug 1221500...)
Comment on attachment 8684273 [details]
MozReview Request: Bug 1222490 - part 1: remove browser/components/tabview and its references, r?ttaubert

https://reviewboard.mozilla.org/r/24515/#review22069
Attachment #8684273 - Flags: review?(ttaubert) → review+
Attachment #8684274 - Flags: review?(ttaubert)
Comment on attachment 8684274 [details]
MozReview Request: Bug 1222490 - part 2: remove traces of tabview from XUL/XBL/JS in other parts of browser/, r?ttaubert

https://reviewboard.mozilla.org/r/24517/#review22071

::: browser/base/content/browser.js:6566
(Diff revision 1)
> -  if (TabView.isVisible()) {
> -    TabView.hide();
> +  let event = new CustomEvent("WindowIsClosing", {bubbles: false, cancelable: true});
> +  if (!window.dispatchEvent(event)) {
>      return false;
>    }

What exactly do we need that for? Can't we just remove the four lines?

::: browser/base/content/sanitize.js:573
(Diff revision 1)
> -          // about TabView or the regular 'warn me before closing windows with N tabs'
> -          // stuff here, and more importantly, we want to set aCallerClosesWindow to true
> +          // the regular 'warn me before closing windows with N tabs' stuff here,
> +          // and more importantly, we want to set aCallerClosesWindow to true

"we don't care about"

::: browser/components/sessionstore/SessionStore.jsm:2360
(Diff revision 1)
> -    // Step 1 of processing:
> -    // Inspect extData for Panorama identifiers. If found, then we want to
> -    // inspect further. If there is a single group, then we can use this
> -    // window. If there are multiple groups then we won't use this window.
> +    let event = aWindow.CustomEvent("SSRestoreIntoWindow",
> +                                    {bubbles: false, cancelable: true});
> +    // Check if we can use the window.
> +    if (!aWindow.dispatchEvent(event))

What's that event for?

I don't think we should offer new APIs/events just in case someone might want to use them. Or did I miss specific use cases?
Comment on attachment 8684275 [details]
MozReview Request: Bug 1222490 - part 3: update all the tests for tabview's removal, r?ttaubert

https://reviewboard.mozilla.org/r/24519/#review22073
Attachment #8684275 - Flags: review?(ttaubert) → review+
Attachment #8684276 - Flags: review?(ttaubert) → review+
Comment on attachment 8684276 [details]
MozReview Request: Bug 1222490 - part 4: remove tabview CSS references, r?ttaubert

https://reviewboard.mozilla.org/r/24521/#review22077
Comment on attachment 8684277 [details]
MozReview Request: Bug 1222490 - Part 5: remove now-unused panorama/tabview strings, r?ttaubert

https://reviewboard.mozilla.org/r/24523/#review22079
Attachment #8684277 - Flags: review?(ttaubert) → review+
Comment on attachment 8684278 [details]
MozReview Request: Bug 1222490 - part 6: remove miscellaneous other bits and bobs referring to panorama/tabview/tab groups, r?ttaubert

https://reviewboard.mozilla.org/r/24525/#review22081

I don't think I can r+ all of the changes, but they're pretty safe...
Attachment #8684278 - Flags: review?(ttaubert) → review+
(In reply to Tim Taubert [:ttaubert] from comment #10)
> Comment on attachment 8684274 [details]
> MozReview Request: Bug 1222490 - part 2: remove traces of tabview from
> XUL/XBL/JS in other parts of browser/, r?ttaubert
> 
> https://reviewboard.mozilla.org/r/24517/#review22071
> 
> ::: browser/base/content/browser.js:6566
> (Diff revision 1)
> > -  if (TabView.isVisible()) {
> > -    TabView.hide();
> > +  let event = new CustomEvent("WindowIsClosing", {bubbles: false, cancelable: true});
> > +  if (!window.dispatchEvent(event)) {
> >      return false;
> >    }
> 
> What exactly do we need that for? Can't we just remove the four lines?

...

> ::: browser/components/sessionstore/SessionStore.jsm:2360
> (Diff revision 1)
> > -    // Step 1 of processing:
> > -    // Inspect extData for Panorama identifiers. If found, then we want to
> > -    // inspect further. If there is a single group, then we can use this
> > -    // window. If there are multiple groups then we won't use this window.
> > +    let event = aWindow.CustomEvent("SSRestoreIntoWindow",
> > +                                    {bubbles: false, cancelable: true});
> > +    // Check if we can use the window.
> > +    if (!aWindow.dispatchEvent(event))
> 
> What's that event for?
> 
> I don't think we should offer new APIs/events just in case someone might
> want to use them. Or did I miss specific use cases?

Both of these were added by you in the original removal code to make it possible to use them from a panorama-like add-on. I figured we ought to be keeping them. Do you think we shouldn't?
Flags: needinfo?(ttaubert)
(In reply to :Gijs Kruitbosch from comment #15)
> Both of these were added by you in the original removal code to make it
> possible to use them from a panorama-like add-on. I figured we ought to be
> keeping them. Do you think we shouldn't?

Iiiinteresting. I don't remember doing that at all, obviously :) I think we should simply remove them and add potentially better APIs only if we need to.

If someone wants to revive Tab Groups with an add-on then it seems that something like |WindowIsClosing| isn't too important, the UI could be adapted to make exiting more obvious - rather than trying to close the window. |SSRestoreIntoWindow| equally seems like an edge case, that would only be fired when someone uses Panorama but doesn't restore sessions automatically, i.e. restores a deferred session with tab groups.
Flags: needinfo?(ttaubert)
(In reply to Tim Taubert [:ttaubert] from comment #16)
> (In reply to :Gijs Kruitbosch from comment #15)
> > Both of these were added by you in the original removal code to make it
> > possible to use them from a panorama-like add-on. I figured we ought to be
> > keeping them. Do you think we shouldn't?
> 
> Iiiinteresting. I don't remember doing that at all, obviously :) I think we
> should simply remove them and add potentially better APIs only if we need to.
> 
> If someone wants to revive Tab Groups with an add-on then it seems that
> something like |WindowIsClosing| isn't too important, the UI could be
> adapted to make exiting more obvious - rather than trying to close the
> window. |SSRestoreIntoWindow| equally seems like an edge case, that would
> only be fired when someone uses Panorama but doesn't restore sessions
> automatically, i.e. restores a deferred session with tab groups.

OK. Then do you feel the same way about freezeTitle in tabbrowser.xml, or does that seem generically-useful-enough to bother having ? :-)
Flags: needinfo?(ttaubert)
Shouldn't the changes in browser/themes/windows/browser-aero.css be in part4 and not part3? (Not that it makes much difference, but I noticed it, so mentioning it.) :)
(In reply to :Gijs Kruitbosch from comment #17)
> OK. Then do you feel the same way about freezeTitle in tabbrowser.xml, or
> does that seem generically-useful-enough to bother having ? :-)

I'd remove it.
Flags: needinfo?(ttaubert)
https://reviewboard.mozilla.org/r/24517/#review22285

::: browser/components/sessionstore/SessionMigration.jsm:32
(Diff revision 1)
>     *     - with tab group info (hidden + group id)

Line no longer relevant?
Comment on attachment 8684273 [details]
MozReview Request: Bug 1222490 - part 1: remove browser/components/tabview and its references, r?ttaubert

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/24515/diff/1-2/
Comment on attachment 8684274 [details]
MozReview Request: Bug 1222490 - part 2: remove traces of tabview from XUL/XBL/JS in other parts of browser/, r?ttaubert

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/24517/diff/1-2/
Attachment #8684274 - Flags: review?(ttaubert)
Comment on attachment 8684275 [details]
MozReview Request: Bug 1222490 - part 3: update all the tests for tabview's removal, r?ttaubert

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/24519/diff/1-2/
Comment on attachment 8684276 [details]
MozReview Request: Bug 1222490 - part 4: remove tabview CSS references, r?ttaubert

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/24521/diff/1-2/
Comment on attachment 8684277 [details]
MozReview Request: Bug 1222490 - Part 5: remove now-unused panorama/tabview strings, r?ttaubert

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/24523/diff/1-2/
Comment on attachment 8684278 [details]
MozReview Request: Bug 1222490 - part 6: remove miscellaneous other bits and bobs referring to panorama/tabview/tab groups, r?ttaubert

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/24525/diff/1-2/
(these are now updated to be based on bug 1221050, and with the feedback from earlier comments addressed. I also found some more comments in SessionStore.jsm that needed updating, see part 2)
https://reviewboard.mozilla.org/r/24517/#review22741

::: browser/components/sessionstore/SessionStore.jsm:1672
(Diff revision 2)
> -    // Default delay of 2 seconds gives enough time to catch multiple TabHide
> +    // Default delay of 2 seconds gives enough time to catch multiple TabShow

Gah, silly copy-paste mistake here - this should remain "TabHide", of course.
Comment on attachment 8684274 [details]
MozReview Request: Bug 1222490 - part 2: remove traces of tabview from XUL/XBL/JS in other parts of browser/, r?ttaubert

https://reviewboard.mozilla.org/r/24517/#review23609
Attachment #8684274 - Flags: review?(ttaubert) → review+
For posterity: with this and the migration:

 250 files changed, 527 insertions(+), 25059 deletions(-)
Depends on: 1229695
(In reply to Luís Miguel [:quicksaver] from comment #34)
> Should this be documented in the release notes:
> https://developer.mozilla.org/Firefox/Releases/45 ?

That's the developer notes... it *is* in the release notes: https://www.mozilla.org/en-US/firefox/45.0a2/auroranotes/ . I added a note in the developer notes as well.
(In reply to :Gijs Kruitbosch from comment #35)
> That's the developer notes...

Right, that's what I meant, thought one thing wrote another. ;)
Product: Firefox → Firefox Graveyard
(In reply to Dão Gottwald [:dao] from comment #37)
> These pages need to be removed:
> 
> https://developer.mozilla.org/en-US/docs/Web/Events/tabviewsearchenabled
> https://developer.mozilla.org/en-US/docs/Web/Events/tabviewsearchdisabled
> https://developer.mozilla.org/en-US/docs/Web/Events/tabviewframeinitialized
> https://developer.mozilla.org/en-US/docs/Web/Events/tabviewshown
> https://developer.mozilla.org/en-US/docs/Web/Events/tabviewhidden

We can't remove pages from MDN. :-(

I've added disclaimers and removed them from the "Events" page, so they shouldn't be findable anymore "soon". The only think to do to get them "more" removed would be to move them to the "Archive" section, but AFAICT I can't actually do that myself. Sheppy, can you help?
Flags: needinfo?(eshepherd)
You need to log in before you can comment on or make changes to this bug.