Closed Bug 589337 Opened 11 years ago Closed 10 years ago
Sort out remote frame destroying/recreating and maybe showing/hiding/
Currently, nsFrameLoader uses Show() to tell the content process to create its fake widget. Hide() isn't forwarded over IPC. This isn't necessarily what we want; Show/Hide are possibly sort of different from widget+layer creation/destruction. When tabs go to the background, we do need an API for destroying shadow layers, because they use a lot more memory than a thumbnail would. That implies we also need an API for recreating the shadow layers. I don't think it makes sense to forward Show/Hide over IPC, since the content process doesn't care. mfinkle says "we could use collapse instead of hidden", but I don't know what this means.
(In reply to comment #0) > mfinkle says "we could use collapse instead of hidden", but I don't know what > this means. I think he was speaking about the css visibility attribute.
(In reply to comment #1) > (In reply to comment #0) > > mfinkle says "we could use collapse instead of hidden", but I don't know what > > this means. > > I think he was speaking about the css visibility attribute. oups, sorry it was not on my plan to add this comment but well... https://developer.mozilla.org/en/CSS/visibility
Suggest blocking-betaN. We'll need some kind of Freeze/Thaw API so that we can throw away background tabs' layers in order to conserve memory.
tracking-fennec: --- → ?
Assignee: nobody → jones.chris.g
tracking-fennec: ? → 2.0b3+
10 years ago
Assignee: jones.chris.g → nobody
Component: DOM → General
Product: Core → Fennec
QA Contact: general → general
Version: unspecified → Trunk
I poked at a platform implementation of this, but the way things are set up, it's going to be easier to do in the frontend. What needs to happen is - set displayport to 0x0 to throw away layers - set docShell.isActive = false to throttle animations
Putting this up for testing. If I understand correctly, this implements comment 4. Switching between tabs seems fine most of the time (less than a second). It can get slow, especially when I switch tabs while pages are loading, but that problem seems to persist even without the patch.
Comment on attachment 490281 [details] [diff] [review] Patch v1 Can you give me any feedback on the stechz? Is this sorta what we are looking for here?
Adds a new message "Browser:UpdateViewport" that is returned from "Browser:Focus". So we destroy layers when the browser is blurred, and restore when it is focused.
Attachment #491298 - Flags: review?(ben)
Comment on attachment 491298 [details] [diff] [review] Patch v2 > case "MozScrolledAreaChanged": > self._contentDocumentWidth = aMessage.json.width; > self._contentDocumentHeight = aMessage.json.height; > > // Recalculate whether the visible area is actually in bounds > self.scrollBy(0, 0); >+ >+ case "Browser:UpdateViewport": > self._updateCacheViewport(); > break; > } > } > }) > ]]></field> > Although this code may be equivalent, it makes me nervous because I doubt anyone will notice the absent "break". Please add _updateCacheViewport call back to MozScrolledAreaChanged and then break. As discussed on IRC, please also rename Browser:Focus/Blur to Browser:Activate/Deactivate.
Attachment #491298 - Flags: review?(ben) → review+
I am still a bit worried about the performance of this on slower devices. If anyone wants to test before we check in, I'd appreciate it.
I don't like some of this patch. We listen for a message "Browser:UpdateViewport" in browser.xml (toolkit code), but this message is sent from content.js (application code). We need to find a way around this. We could listen for a "focus" event in bindings/browser.js and send a message back to browser.xml.
I guess we would need to set focus in the Browser:Activate message handler though
It would be an "active" event, yes?
finkle version. also killed off this._selected stuff that I can't find us using anywhere.
Comment on attachment 491343 [details] [diff] [review] Fixes v3 I just remembered that custom messages in browser.xml use the "Content:" prefix. I can change that and land.
Attachment #491343 - Flags: review?(mark.finkle) → review+
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
We reversed the order of naming here. AFAIK, nothing should be currently drawing at all. So now I'm not even sure we are hitting this code, but here's the quick fix for the naming.
Attachment #492377 - Flags: review?(mark.finkle)
Attachment #492377 - Flags: review?(mark.finkle) → review+
You need to log in before you can comment on or make changes to this bug.