Closed
Bug 1222490
Opened 9 years ago
Closed 9 years ago
Actually remove panorama after warning + migration have landed
Categories
(Firefox Graveyard :: Panorama, defect)
Firefox Graveyard
Panorama
Tracking
(Not tracked)
RESOLVED
FIXED
Firefox 45
People
(Reporter: Gijs, Assigned: Gijs, NeedInfo)
References
Details
(Keywords: addon-compat, dev-doc-needed)
Attachments
(6 files)
(deleted),
text/x-review-board-request
|
ttaubert
:
review+
|
Details |
(deleted),
text/x-review-board-request
|
ttaubert
:
review+
|
Details |
(deleted),
text/x-review-board-request
|
ttaubert
:
review+
|
Details |
(deleted),
text/x-review-board-request
|
ttaubert
:
review+
|
Details |
(deleted),
text/x-review-board-request
|
ttaubert
:
review+
|
Details |
(deleted),
text/x-review-board-request
|
ttaubert
:
review+
|
Details |
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.
Assignee | ||
Comment 1•9 years ago
|
||
Bug 1222490 - part 1: remove browser/components/tabview and its references, r?ttaubert
Attachment #8684273 -
Flags: review?(ttaubert)
Assignee | ||
Comment 2•9 years ago
|
||
Bug 1222490 - part 2: remove traces of tabview from XUL/XBL/JS in other parts of browser/, r?ttaubert
Attachment #8684274 -
Flags: review?(ttaubert)
Assignee | ||
Comment 3•9 years ago
|
||
Bug 1222490 - part 3: update all the tests for tabview's removal, r?ttaubert
Attachment #8684275 -
Flags: review?(ttaubert)
Assignee | ||
Comment 4•9 years ago
|
||
Bug 1222490 - part 4: remove tabview CSS references, r?ttaubert
Attachment #8684276 -
Flags: review?(ttaubert)
Assignee | ||
Comment 5•9 years ago
|
||
Bug 1222490 - Part 5: remove now-unused panorama/tabview strings, r?ttaubert
Attachment #8684277 -
Flags: review?(ttaubert)
Assignee | ||
Comment 6•9 years ago
|
||
Bug 1222490 - part 6: remove miscellaneous other bits and bobs referring to panorama/tabview/tab groups, r?ttaubert
Attachment #8684278 -
Flags: review?(ttaubert)
Assignee | ||
Updated•9 years ago
|
Assignee: nobody → gijskruitbosch+bugs
Status: NEW → ASSIGNED
Assignee | ||
Comment 7•9 years ago
|
||
Keywords: addon-compat
Assignee | ||
Comment 8•9 years ago
|
||
(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 9•9 years ago
|
||
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+
Updated•9 years ago
|
Attachment #8684274 -
Flags: review?(ttaubert)
Comment 10•9 years ago
|
||
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 11•9 years ago
|
||
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+
Updated•9 years ago
|
Attachment #8684276 -
Flags: review?(ttaubert) → review+
Comment 12•9 years ago
|
||
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 13•9 years ago
|
||
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 14•9 years ago
|
||
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+
Assignee | ||
Comment 15•9 years ago
|
||
(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)
Comment 16•9 years ago
|
||
(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)
Assignee | ||
Comment 17•9 years ago
|
||
(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)
Comment 18•9 years ago
|
||
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.) :)
Comment 19•9 years ago
|
||
(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)
Comment 20•9 years ago
|
||
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 hidden (off-topic) |
Assignee | ||
Comment 22•9 years ago
|
||
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/
Assignee | ||
Comment 23•9 years ago
|
||
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)
Assignee | ||
Comment 24•9 years ago
|
||
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/
Assignee | ||
Comment 25•9 years ago
|
||
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/
Assignee | ||
Comment 26•9 years ago
|
||
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/
Assignee | ||
Comment 27•9 years ago
|
||
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/
Assignee | ||
Comment 28•9 years ago
|
||
(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)
Assignee | ||
Comment 29•9 years ago
|
||
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 30•9 years ago
|
||
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+
Comment 31•9 years ago
|
||
https://hg.mozilla.org/integration/fx-team/rev/35b7fa5ebd58
https://hg.mozilla.org/integration/fx-team/rev/29c12e8bf172
https://hg.mozilla.org/integration/fx-team/rev/19faee4224fb
https://hg.mozilla.org/integration/fx-team/rev/7604a019463a
https://hg.mozilla.org/integration/fx-team/rev/0b0f2969428c
https://hg.mozilla.org/integration/fx-team/rev/fbb5323919c3
Assignee | ||
Comment 32•9 years ago
|
||
For posterity: with this and the migration:
250 files changed, 527 insertions(+), 25059 deletions(-)
Comment 33•9 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/35b7fa5ebd58
https://hg.mozilla.org/mozilla-central/rev/29c12e8bf172
https://hg.mozilla.org/mozilla-central/rev/19faee4224fb
https://hg.mozilla.org/mozilla-central/rev/7604a019463a
https://hg.mozilla.org/mozilla-central/rev/0b0f2969428c
https://hg.mozilla.org/mozilla-central/rev/fbb5323919c3
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 45
Comment 34•9 years ago
|
||
Should this be documented in the release notes: https://developer.mozilla.org/Firefox/Releases/45 ?
Assignee | ||
Comment 35•9 years ago
|
||
(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.
Comment 36•9 years ago
|
||
(In reply to :Gijs Kruitbosch from comment #35)
> That's the developer notes...
Right, that's what I meant, thought one thing wrote another. ;)
Updated•9 years ago
|
Product: Firefox → Firefox Graveyard
Comment 37•8 years ago
|
||
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
status-firefox45:
fixed → ---
Keywords: dev-doc-needed
Assignee | ||
Comment 38•8 years ago
|
||
(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.
Description
•