Closed Bug 880911 Opened 11 years ago Closed 11 years ago

tear-off chat windows

Categories

(Firefox Graveyard :: SocialAPI, defect)

Product:
Firefox Graveyard
Component:
SocialAPI
Version:
23 Branch
Platform:
x86
macOS
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

(firefox24 verified, firefox25 verified, relnote-firefox 24+)

Status:
VERIFIED FIXED
Milestone:
Firefox 24
Tracking Flags:
Tracking Status
firefox24 --- verified
firefox25 --- verified
relnote-firefox --- 24+

People

(Reporter: mixedpuppy, Assigned: mixedpuppy)

References

Details

(Keywords: feature)

Attachments

(2 files, 3 obsolete files)

Icons: Close/minimize/popout for upper right of chat window (default and click state)
(deleted), application/zip
Details
chat tearoff patch
(deleted), patch
Felipe
: review+
Details | Diff | Splinter Review
Attached patch WIP tearoff patch (obsolete) (deleted) — — Splinter Review
Users should be able to drag chat windows out to separate windows. This is separate from and in addition to bug 878852 (resizing attached chat windows). Patch revives the service window for chat window use. Dragging on chat window titlebar will tear off the chat window into it's own window. Each chat window is on it's own. Still need to look into some ux issues (e.g. how to re-attach if necessary), preventing duplicate chat windows opening, provider shutdown, etc. Would also like to support tabs in these windows, have to figure out how to get a tab without having extrachrome (ala about:addons).
Attachment #760022 - Flags: feedback?(mhammond)
Comment on attachment 760022 [details] [diff] [review] WIP tearoff patch Review of attachment 760022 [details] [diff] [review]: ----------------------------------------------------------------- I'm not that comfortable with exposing openServiceWindow to providers without a better idea of how they might use it and how it differs from window.open etc. As you mention, we also need a strategy for re-collapsing. But more generally, I wonder if we shouldn't just "promote" these chats to regular tabs? IOW, just allow them to be dragged to the tabstrip in the existing window, or if dropped outside the window, just have a new browser window created hosting this tab. It looks from the comments that this is almost where you want to end up anyway... So f+ on this code, but I've enough reservations on the general strategy that I'd really want to see more input from others (eg, UX, and gavin/jaws/etc) ::: browser/base/content/socialchat.xml @@ +13,5 @@ > <xul:label class="chat-title" flex="1" xbl:inherits="value=label" crop="center"/> > <xul:toolbarbutton class="chat-close-button chat-toolbarbutton" > oncommand="document.getBindingParent(this).close();"/> > </xul:hbox> > + <xul:browser anonid="iframe" class="chat-frame" flex="1" I think the anonid should change as sticking with "iframe" is misleading. I wonder if an implementation-agnostic name might be good (eg, "content" - that is already overloaded, but probably not in a misleading way so might be ok) @@ +404,5 @@ > <method name="_remove"> > <parameter name="aChatbox"/> > <body><![CDATA[ > + if (aChatbox.iframe.socialErrorListener) > + aChatbox.iframe.socialErrorListener.remove(); how does this patch make it such that the error listener might not exist? @@ +543,5 @@ > + if (chatbox && > + (event.type == "drop" || event.type == "dragover") && > + event.dataTransfer.dropEffect == "link") { > + let boxObject = chatbox.boxObject; > + if (event.screenX < boxObject.screenX + boxObject.width * .25 || why this restriction? It seems arbitrary so should have a comment. @@ +551,5 @@ > + return chatbox; > + ]]></body> > + </method> > + > + <!-- Moves a tab to a new browser window, unless it's already the only tab looks like this comment was copy-pasted but doesn't actually apply here? @@ +573,5 @@ > + .QueryInterface(Ci.nsIInterfaceRequestor) > + .getInterface(Ci.nsIDOMWindow); > + let _gBrowser = chromeWin.gBrowser; > + _gBrowser.stop(); > + _gBrowser.docShell; should comment why this apparent noop is necessary. @@ +643,5 @@ > + Cc["@mozilla.org/gfx/screenmanager;1"] > + .getService(Ci.nsIScreenManager) > + .screenForRect(eX, eY, 1, 1) > + .GetAvailRect(sX, sY, sWidth, sHeight); > + // our desired window size for chat this seems arbitrary - I wonder if the size should be in a pref and updated as the user resizes (ie, so the last size chosen by the user is reused)? ::: toolkit/components/social/MozSocialAPI.jsm @@ +122,5 @@ > let url = targetWindow.document.documentURIObject.resolve(toURL); > openChatWindow(getChromeWindow(targetWindow), provider, url, callback); > } > }, > + openServiceWindow: { I'm not clear on why we are exposing this to social code. If we are trying to expose a way to create chats already "torn-off", then I think exposing this to the existing openChat feature is better in the first instance. For example, what happens if providers start to use this feature for things unrelated to chats? If that's OK, then how will that interact with our use of this API internally for chats? It's easier to add new APIs later than it is to remove them :) @@ +127,5 @@ > + enumerable: true, > + configurable: true, > + writable: true, > + value: function(toURL, name, options) { > + let url = targetWindow.document.documentURIObject.resolve(toURL); We should resolve bug 851336 before we do this (ie, I think I'd r- based on this) @@ +324,5 @@ > // we can call it unconditionally. > chromeWindow.getAttention(); > } > + > +this.openServiceWindow = how is this different from window.open() (apart from the fact window.open isn't available to workers?) If no difference, then why the additional social-specific semantics?
Attachment #760022 - Flags: feedback?(mhammond) → feedback+
Attached patch WIP tearoff patch (obsolete) (deleted) — — Splinter Review
This patch is functionally equivalent to the mockup from Boriss in bug 878852. We agreed to keep the tear-off functionality here, and bug 878852 for resizing an attached chat window. todo: two images (Boriss), css and tests (it only fixes existing tests) otherwise, this should be review ready
Assignee: nobody → mixedpuppy
Attachment #760022 - Attachment is obsolete: true
Attachment #761857 - Flags: feedback?(mhammond)
Comment on attachment 761857 [details] [diff] [review] WIP tearoff patch Review of attachment 761857 [details] [diff] [review]: ----------------------------------------------------------------- A few nits, but lgtm ::: browser/base/content/chatWindow.xul @@ +5,5 @@ > +# License, v. 2.0. If a copy of the MPL was not distributed with this > +# file, You can obtain one at http://mozilla.org/MPL/2.0/. > + > +<?xml-stylesheet href="chrome://browser/content/browser.css" type="text/css"?> > +<?xml-stylesheet href="chrome://browser/skin/" type="text/css"?> a lonely trailing space... @@ +35,5 @@ > + // cargo-culted from browser.js for nonBrowserStartup, but we're slightly > + // different what what we need to leave enabled > + onLoad: function() { > + // Disable inappropriate commands / submenus > + var disabledItems = ['Browser:SavePage', const? @@ +41,5 @@ > + 'viewToolbarsMenu', 'viewSidebarMenuMenu', > + 'viewFullZoomMenu', 'pageStyleMenu', 'charsetMenu', > + 'viewHistorySidebar', 'Browser:AddBookmarkAs', 'Browser:BookmarkAllTabs', > + 'Browser:ToggleTabView', 'Browser:ToggleAddonBar']; > + var element; declare this as 'let' in the loop @@ +45,5 @@ > + var element; > + > + for (let disabledItem of disabledItems) { > + element = document.getElementById(disabledItem); > + if (element) I'd be inclined to drop the check for |element|, else the list might end up getting out of date and referencing elements long after they were deleted. Assuming tests for this window are in place, tests should pickup if an element is removed later and causes this to fail. @@ +49,5 @@ > + if (element) > + element.setAttribute("disabled", "true"); > + } > + > + this._delayedStartupTimeoutId = setTimeout(this.chatWindowDelayedStartup.bind(this), 0); I don't quite understand why this is on a timer - I guess this was copied from browser.js, but it doesn't seem to make sense for chat windows. Session-restore isn't going to re-create them (right?), so the browser must be fully initialized by the time we get here... @@ +71,5 @@ > + BrowserOffline.uninit(); > + } > +} > + > +// XXX popup notification support is still dependent on bug 809085. The I'm not sure what you are saying here, but I'd suggest we just have a new bug for adding this code (or just add it in bug 809085) rather than having and obscure comment and code that doesn't yet work (assuming IIUC) @@ +82,5 @@ > + return new tmp.PopupNotifications(document.getElementById("chatter").content, > + document.getElementById("notification-popup"), > + null); > + } catch (ex) { > + Cu.reportError(ex); maybe start using Console.jsm - bug 877389 is calling for all Cu.reportError() calls to be replaced with that, so we might as well not make that harder. @@ +109,5 @@ > + gContextMenu = null;"> > +#include browser-context.inc > + </menupopup> > + > +<!-- notification popups duplicated from browser.xul --> it's a shame to duplicate all these, and it seems likely they might get out of sync. Is it worth opening another bug to extract them into a .inc file that can be shared? @@ +141,5 @@ > + </popupnotificationcontent> > + </popupnotification> > + </popupset> > + > + <commandset id="editMenuCommands"/> trailing whitespace ::: browser/base/content/socialchat.xml @@ +43,5 @@ > this.removeEventListener("DOMContentLoaded", DOMContentLoaded, true); > this.isActive = !this.minimized; > // process this._callbacks, then set to null so the chatbox creator > // knows to make new callbacks immediately. > + if (this._callbacks) { when can this._callbacks be null? @@ +166,5 @@ > + > + <method name="swapWindows"> > + <body><![CDATA[ > + if (this.chatbar) { > + this.chatbar.detachChatbox(this, { outerWidth: 400, outerHeight: 400}); I'm not comfortable with these hard-coded sizes. It seems we should allow the default size when first opened and allow the persisted attributes in the chatwindow XUL to do the rest. @@ +168,5 @@ > + <body><![CDATA[ > + if (this.chatbar) { > + this.chatbar.detachChatbox(this, { outerWidth: 400, outerHeight: 400}); > + } else { > + // attach this chatbox to the topmost browser window Can we use findChromeWindowForChats() here? There are a few existing bugs around which top-level window is used for chats, so if we ever get around to fixing them, it would be nice to get the same behaviour here automatically. @@ +172,5 @@ > + // attach this chatbox to the topmost browser window > + let provider = Social._getProviderFromOrigin(this.content.getAttribute("origin")); > + let win = Services.wm.getMostRecentWindow("navigator:browser"); > + let chatbar = win.SocialChatBar.chatbar; > + chatbar.openChat(provider, "about:blank", function(win) { I learned the other day that "fat arrow" functions are lexically bound, so, IIUC: chatbar.openChat(provider, "about:blank", win => { }); would work and it would not be necessary to .bind(this) @@ +177,5 @@ > + try { > + chatbar.selectedChat.swapDocShells(this); > + chatbar.focus(); > + this.close(); > + } catch(e) { looks like debug code - should either be removed or formalized (ie, indented, console.error used) @@ +636,5 @@ > + .QueryInterface(Ci.nsIInterfaceRequestor) > + .getInterface(Ci.nsIDOMWindow); > + > + chromeWin.addEventListener("load", function _chatLoad() { > + chromeWin.removeEventListener("load", _chatLoad, true); should check the target is actually the window here? @@ +717,5 @@ > + this.detachChatbox(draggedChat, { screenX: left, > + screenY: top, > + outerWidth: winWidth, > + outerHeight: winHeight > + }); align the closing } with the opening one ::: browser/base/content/test/social/browser_social_chatwindow.js @@ +350,5 @@ > is(chatbar.selectedChat, third, "third chat should be selected"); > ok(!chatbar.selectedChat.hasAttribute("activity"), "third chat should have no activity"); > // send an activity message to the second. > ok(!second.hasAttribute("activity"), "second chat should have no activity"); > + let iframe2 = second.content; might as well rename the vars too @@ +361,5 @@ > chatbar.selectedChat = second; > ok(!second.hasAttribute("activity"), "second chat should no longer have activity"); > // Now try the first - it is collapsed, so the 'nub' also gets activity attr. > ok(!first.hasAttribute("activity"), "first chat should have no activity"); > + let iframe1 = first.content; ditto
Attachment #761857 - Flags: feedback?(mhammond) → feedback+
Comment on attachment 762094 [details] Icons: Close/minimize/popout for upper right of chat window (default and click state) Boriss, the current close icons we have are os themed (linux+osx the same, different colors on windows). Do we need to do that for the new icons, or should I replace the existing windows icon?
Attachment #762094 - Flags: feedback?(jboriss)
Attached patch chat tearoff patch (obsolete) (deleted) — — Splinter Review
along with a try at https://tbpl.mozilla.org/?tree=Try&rev=2bee6f19847d
Attachment #761857 - Attachment is obsolete: true
Attachment #762407 - Flags: review?(mhammond)
Comment on attachment 762407 [details] [diff] [review] chat tearoff patch TBH I'm not that comfortable reviewing this patch - it introduces a new top-level window so probably pushes what I can sneakily review without actually being a peer. So I'll nominate Felipe or Jaws :) If they want to push it back to me, I'll be happy to do it.
Attachment #762407 - Flags: review?(mhammond)
Attachment #762407 - Flags: review?(jaws)
Attachment #762407 - Flags: review?(felipc)
(In reply to Mark Hammond (:markh) from comment #3) > Comment on attachment 761857 [details] [diff] [review] > @@ +109,5 @@ > > + gContextMenu = null;"> > > +#include browser-context.inc > > + </menupopup> > > + > > +<!-- notification popups duplicated from browser.xul --> > > it's a shame to duplicate all these, and it seems likely they might get out > of sync. Is it worth opening another bug to extract them into a .inc file > that can be shared? when I get to this part, I am going to do an inc file. > ::: browser/base/content/socialchat.xml > @@ +43,5 @@ > > this.removeEventListener("DOMContentLoaded", DOMContentLoaded, true); > > this.isActive = !this.minimized; > > // process this._callbacks, then set to null so the chatbox creator > > // knows to make new callbacks immediately. > > + if (this._callbacks) { > > when can this._callbacks be null? when you call detachChatbox we don't have those callbacks. > @@ +166,5 @@ > > + > > + <method name="swapWindows"> > > + <body><![CDATA[ > > + if (this.chatbar) { > > + this.chatbar.detachChatbox(this, { outerWidth: 400, outerHeight: 400}); > > I'm not comfortable with these hard-coded sizes. It seems we should allow > the default size when first opened and allow the persisted attributes in the > chatwindow XUL to do the rest. they are gone (or rather moved to the window, user sizes are now persisted), other than for the attempt at positioning in the window when you tear off via drag/drop.
(In reply to Mark Hammond (:markh) from comment #7) > Comment on attachment 762407 [details] [diff] [review] > chat tearoff patch > > TBH I'm not that comfortable reviewing this patch - it introduces a new > top-level window so probably pushes what I can sneakily review without > actually being a peer. So I'll nominate Felipe or Jaws :) If they want to > push it back to me, I'll be happy to do it. That's cool, I've been asking you since you know the chat stuff well.
Why do we need to support resizable chat windows and tear-off chat windows? I think the resizable chat windows will be sufficient and reduce the complexity significantly.
(In reply to Jared Wein [:jaws] from comment #10) > Why do we need to support resizable chat windows and tear-off chat windows? > I think the resizable chat windows will be sufficient and reduce the > complexity significantly. TBH, if we didn't want to support both, I'd think the tear-offs would be more useful than resizable in a WebRTC context. Eg, you'd likely want the ability to move the video window to one side so you can still use other apps (eg, find an email, etc) while the call is in progress. Even using Fx itself would be somewhat difficult if you tried to have the video window at a reasonable size while not obscuring the content in the browser you are discussing during the call. Or to put it another way: I'd have real trouble replacing the Vidyo/Skype calls I'm involved in with a resizable chat window that can't be torn away from the main browser.
(In reply to Mark Hammond (:markh) from comment #11) > (In reply to Jared Wein [:jaws] from comment #10) > > Why do we need to support resizable chat windows and tear-off chat windows? > > I think the resizable chat windows will be sufficient and reduce the > > complexity significantly. > > TBH, if we didn't want to support both, I'd think the tear-offs would be > more useful than resizable in a WebRTC context. Eg, you'd likely want the > ability to move the video window to one side so you can still use other apps > (eg, find an email, etc) while the call is in progress. Even using Fx > itself would be somewhat difficult if you tried to have the video window at > a reasonable size while not obscuring the content in the browser you are > discussing during the call. > > Or to put it another way: I'd have real trouble replacing the Vidyo/Skype > calls I'm involved in with a resizable chat window that can't be torn away > from the main browser. Mark hit the nail on the head. While resizing might be trivial, I'm not convinced that resizable docked chats are that useful. I find the docked chats get in the way at their current size. If you consider a combination of something like towtruck and talkilla for cobrowsing, the interference becomes substantial if you have to make your video window larger.
Ok, I can see that. A few issues still remain though with the current patch, since there isn't a way to place the detached window back in as a regular chat tab. We also will lack a way to undo-closing the chat window if it is closed on accident. For a v1, can we do as Mark proposed and just promote the chat to a tab within the current browser? That will also give us the benefit of supporting Undo Close Tab and other nice benefits of the platform.
(In reply to Jared Wein [:jaws] from comment #13) > Ok, I can see that. A few issues still remain though with the current patch, > since there isn't a way to place the detached window back in as a regular > chat tab. What do you mean? There is a button to do that. > We also will lack a way to undo-closing the chat window if it is > closed on accident. We didn't have that before either, but I get the point there. I'm not certain if that will work in all cases, since there may be webrtc reconnections, etc. Not saying it wont work, I just don't know if provider-specific issues could crop up if reloading the chat window. I think we could do a followup bug to add undo and examine what if any issues there would be with that. Are there other issues? > For a v1, can we do as Mark proposed and just promote the chat to a tab > within the current browser? That will also give us the benefit of supporting > Undo Close Tab and other nice benefits of the platform. It defeats many use-cases in talkilla, prevents video chat for cobrowsing, etc. IMO chat in a tab is not an alternative, though I'd be happy to do it as an addition.
Chat in a tab would allow that tab to be in a separate window, which should be similar to the serviceWindow in terms of functionality, right?
(In reply to Jared Wein [:jaws] from comment #15) > Chat in a tab would allow that tab to be in a separate window, which should > be similar to the serviceWindow in terms of functionality, right? Oh, I get where you're going, and it reminded me of the earlier approach I tried to take, which was exactly what you are suggesting. I was able to load the chatbox into a tab, however, in this case, the doc shell swapping would not work on the sub frame inside the chatbox. I looked at the doc shell handling, it's pretty extensive and I'd be more worried about changing that than the approach I ended up going with.
(In reply to Shane Caraveo (:mixedpuppy) from comment #16) > I was able to > load the chatbox into a tab, however, in this case, the doc shell swapping > would not work on the sub frame inside the chatbox. I looked at the doc > shell handling, it's pretty extensive and I'd be more worried about changing > that than the approach I ended up going with. Pardon my ignorance then, but how would docshell in the service window not have the same issue? Perhaps Felipe or Tim can help you out here?
(In reply to Jared Wein [:jaws] from comment #17) > (In reply to Shane Caraveo (:mixedpuppy) from comment #16) > > I was able to > > load the chatbox into a tab, however, in this case, the doc shell swapping > > would not work on the sub frame inside the chatbox. I looked at the doc > > shell handling, it's pretty extensive and I'd be more worried about changing > > that than the approach I ended up going with. > > Pardon my ignorance then, but how would docshell in the service window not > have the same issue? Perhaps Felipe or Tim can help you out here? The servicewindow (no longer used in current patch) was loading an html page in the tab and docshell swapping did work. With the ux requirements that Boriss and I went through, I have to use xul, loading the chatbox element. Thus (if I load a xul page into the tab), the browser that needs to be swapped is not top-level, which afaik docshellswap prevents.
Blocks: 883346
Comment on attachment 762407 [details] [diff] [review] chat tearoff patch I'll defer to Felipe for review on this.
Attachment #762407 - Flags: review?(jaws)
Comment on attachment 762407 [details] [diff] [review] chat tearoff patch Review of attachment 762407 [details] [diff] [review]: ----------------------------------------------------------------- Can we rename everything related to the "swap" functionality to "pop"? This mean popin/popout in the CSS, functions, etc.. I think this term will make it more standardly easier to understand ::: browser/base/content/chatWindow.xul @@ +15,5 @@ > +#include browser-doctype.inc > + > +<window id="chat-window" > + windowtype="Social:Chat" > + xmlns:rdf="http://www.w3.org/1999/02/22-rdf-syntax-ns#" rdf namespace not needed @@ +31,5 @@ > + <script type="application/javascript" src="chrome://global/content/globalOverlay.js"/> > + <script type="application/javascript" src="chrome://global/content/contentAreaUtils.js"/> > + <script type="application/javascript" src="chrome://browser/content/nsContextMenu.js"/> > + > +#include global-scripts.inc is everything included here really necessary for this window? including browser.js? I think this window could be more lightweight.. ::: browser/base/content/socialchat.xml @@ +169,5 @@ > + <method name="swapWindows"> > + <body><![CDATA[ > + let provider = Social._getProviderFromOrigin(this.content.getAttribute("origin")); > + if (this.chatbar) { > + this.chatbar.detachChatbox(this, {}, win => { can you add the option centerscreen here or somehow in this path? @@ +630,5 @@ > + for (let name in aOptions) > + options += "," + name + "=" + aOptions[name]; > + > + let otherWin = window.openDialog("chrome://browser/content/chatWindow.xul", null, "chrome,all" + options); > + let chromeWin = otherWin.QueryInterface(Ci.nsIInterfaceRequestor) is it necessary to use chromeWin for the listener below? Being a .xul window I'd imagine that otherWin = chromeWin here.. @@ +687,5 @@ > + let windowUtils = window.getInterface(Ci.nsIDOMWindowUtils); > + let scale = windowUtils.screenPixelsPerCSSPixel / windowUtils.fullZoom; > + let canvas = document.createElementNS("http://www.w3.org/1999/xhtml", "canvas"); > + canvas.mozOpaque = true; > + canvas.width = 160 * scale; these numbers need some 'splainin @@ +717,5 @@ > + // opening at the drop point. If the user has resized the window to > + // something larger (which gets persisted), at least a good portion of > + // the window should still be within the screen. > + let winWidth = 400; > + let winHeight = 420; is it worth the trouble doing these calculations instead of using centerscreen? ::: browser/themes/shared/social/chat.inc.css @@ +53,5 @@ > + -moz-image-region: rect(14px, 56px, 28px, 42px); > +} > + > + > +.chat-swap-button[swapin] { instead of using the swapin attribute, you could do the equivalent of what the JS part does: .chat-swap-button { /* image for pop-in */ } chatbar > chatbox > .chat-swap-button { /* image for pop-out, because it's under a chatbar hierarchy */ }
Attachment #762407 - Flags: review?(felipc) → feedback+
Blocks: 809085
(In reply to :Felipe Gomes from comment #20) > Comment on attachment 762407 [details] [diff] [review] > chat tearoff patch > > Review of attachment 762407 [details] [diff] [review]: > ----------------------------------------------------------------- > > Can we rename everything related to the "swap" functionality to "pop"? This > mean popin/popout in the CSS, functions, etc.. I think this term will make > it more standardly easier to understand There are not separate popin/out functions, it is a toggle operation. I'm not attached to "swap", but I dont think popin/popout work here, especially since the swapin attribute is no longer needed after the css change you suggested. > ::: browser/base/content/chatWindow.xul > @@ +15,5 @@ > > +#include browser-doctype.inc > > + > > +<window id="chat-window" > > + windowtype="Social:Chat" > > + xmlns:rdf="http://www.w3.org/1999/02/22-rdf-syntax-ns#" > > rdf namespace not needed it is used in some include, so is necessary. > @@ +31,5 @@ > > + <script type="application/javascript" src="chrome://global/content/globalOverlay.js"/> > > + <script type="application/javascript" src="chrome://global/content/contentAreaUtils.js"/> > > + <script type="application/javascript" src="chrome://browser/content/nsContextMenu.js"/> > > + > > +#include global-scripts.inc > > is everything included here really necessary for this window? including > browser.js? I think this window could be more lightweight.. we chatted in more detail about this via irc, but yes, it is necessary. > @@ +687,5 @@ > > + let windowUtils = window.getInterface(Ci.nsIDOMWindowUtils); > > + let scale = windowUtils.screenPixelsPerCSSPixel / windowUtils.fullZoom; > > + let canvas = document.createElementNS("http://www.w3.org/1999/xhtml", "canvas"); > > + canvas.mozOpaque = true; > > + canvas.width = 160 * scale; > > these numbers need some 'splainin I added a comment to the dragstart handler, but basically I am duplicating the visual experience of dragging a tab, so much of this, certainly the canvas code, is copied from tabbrowser.xml. > @@ +717,5 @@ > > + // opening at the drop point. If the user has resized the window to > > + // something larger (which gets persisted), at least a good portion of > > + // the window should still be within the screen. > > + let winWidth = 400; > > + let winHeight = 420; > > is it worth the trouble doing these calculations instead of using > centerscreen? When dragging the chat window out, I wanted to have the window open at the drop location (e.g. being able to drag the chat to a different screen). Tabs work the same way, though they duplicate the size of the window they are dragged out of (thus are calculated). Since we open into a larger window than the attached windows, we dont have that luxury. I feel this is a fine compromise.
felipe: mixedpuppy: it'd be nice to test what effect do these new window have on session restore felipe: e.g. if you close the last browser window and that one remains, does it change what session is saved felipe: making it more clear, "and a pop out chat remains" If I quit Firefox when only a chat window is open (no browser windows), session restore opens the last opened browser window (as expected) and does not open any chat windows. I think this is the correct.
(In reply to Shane Caraveo (:mixedpuppy) from comment #22) > felipe: mixedpuppy: it'd be nice to test what effect do these new window > have on session restore > felipe: e.g. if you close the last browser window and that one remains, does > it change what session is saved > felipe: making it more clear, "and a pop out chat remains" > > > If I quit Firefox when only a chat window is open (no browser windows), > session restore opens the last opened browser window (as expected) and does > not open any chat windows. I think this is the correct. correction. It opens a browser window with the configured home page. It does not restore the last browser window and the tabs that window contained. I never have more than one tab really open in my test profile, so I missed that initially. I'll look into this further.
(In reply to Shane Caraveo (:mixedpuppy) from comment #23) > (In reply to Shane Caraveo (:mixedpuppy) from comment #22) > > felipe: mixedpuppy: it'd be nice to test what effect do these new window > > have on session restore > > felipe: e.g. if you close the last browser window and that one remains, does > > it change what session is saved > > felipe: making it more clear, "and a pop out chat remains" > > > > > > If I quit Firefox when only a chat window is open (no browser windows), > > session restore opens the last opened browser window (as expected) and does > > not open any chat windows. I think this is the correct. > > correction. It opens a browser window with the configured home page. It > does not restore the last browser window and the tabs that window contained. > I never have more than one tab really open in my test profile, so I missed > that initially. I'll look into this further. But is the previous session still stored and available in the "Restore session" button in the start page? If so this is the standard behavior. To really test it it's probably better to change the "Options > General > When Firefox starts" to "Show my windows and tabs from last time"
(In reply to :Felipe Gomes from comment #24) > (In reply to Shane Caraveo (:mixedpuppy) from comment #23) > > (In reply to Shane Caraveo (:mixedpuppy) from comment #22) > > > felipe: mixedpuppy: it'd be nice to test what effect do these new window > > > have on session restore > > > felipe: e.g. if you close the last browser window and that one remains, does > > > it change what session is saved > > > felipe: making it more clear, "and a pop out chat remains" > > > > > > > > > If I quit Firefox when only a chat window is open (no browser windows), > > > session restore opens the last opened browser window (as expected) and does > > > not open any chat windows. I think this is the correct. > > > > correction. It opens a browser window with the configured home page. It > > does not restore the last browser window and the tabs that window contained. > > I never have more than one tab really open in my test profile, so I missed > > that initially. I'll look into this further. > > But is the previous session still stored and available in the "Restore > session" button in the start page? If so this is the standard behavior. > > To really test it it's probably better to change the "Options > General > > When Firefox starts" to "Show my windows and tabs from last time" That is how I have it configured, I just didn't have additional tabs opened before. ideally, the last browser window is restored, and no chat windows are. I think that is also what you are saying.
Attached patch chat tearoff patch (deleted) — — Splinter Review
feedback implemented https://tbpl.mozilla.org/?tree=Try&rev=5339b505a40f
Attachment #762407 - Attachment is obsolete: true
Attachment #765082 - Flags: review?(felipc)
Attachment #765082 - Flags: review?(felipc) → review+
another try with test changes to see if this fixes the previous bc oranges. https://tbpl.mozilla.org/?tree=Try&rev=c425c2e68119
https://hg.mozilla.org/mozilla-central/rev/36d494a9b795
Status: NEW → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 24
Depends on: 886072
Depends on: 886086
relnote-firefox: --- → ?
QA Contact: srgohilgnfc
Attachment #762094 - Flags: feedback?(jboriss)
Depends on: 888336
User ID: Mozilla/5.0 (Windows NT 6.3; Win64; x64; rv:25.0) Gecko/20130725 Firefox/25.0 Build ID: 20130725030212 Built from http://hg.mozilla.org/mozilla-central/rev/a4c1961bf723 Tested this on Windows 8.1 preview using latest nightly, I can tear off chat windows into separate windows.I don't see any issue.
status-firefox25: --- → verified
User ID: Mozilla/5.0 (Windows NT 6.2; WOW64; rv:24.0) Gecko/20130725 Firefox/24.0 Build ID: 20130725004004 Built from http://hg.mozilla.org/releases/mozilla-aurora/rev/f61ece780449 Tested this on Windows 8.1 preview using latest Aurora, I can tear off chat windows into separate windows.I don't see any issue.
Status: RESOLVED → VERIFIED
status-firefox24: --- → verified
Depends on: 898161
No longer depends on: 898161
Depends on: 898167
Adding the feature keyword to be included in the new Release Tracking page.
Keywords: feature
Product: Firefox → Firefox Graveyard
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: