Closed
Bug 589337
Opened 14 years ago
Closed 14 years ago
Sort out remote frame destroying/recreating and maybe showing/hiding/
Categories
(Firefox for Android Graveyard :: General, defect)
Firefox for Android Graveyard
General
Tracking
(fennec2.0b3+)
RESOLVED
FIXED
Tracking | Status | |
---|---|---|
fennec | 2.0b3+ | --- |
People
(Reporter: cjones, Assigned: wesj)
References
Details
Attachments
(2 files, 3 obsolete files)
(deleted),
patch
|
mfinkle
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
mfinkle
:
review+
|
Details | Diff | Splinter Review |
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.
Comment 1•14 years ago
|
||
(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.
Comment 2•14 years ago
|
||
(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
Reporter | ||
Comment 3•14 years ago
|
||
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: --- → ?
Updated•14 years ago
|
Assignee: nobody → jones.chris.g
tracking-fennec: ? → 2.0b3+
Reporter | ||
Updated•14 years ago
|
Assignee: jones.chris.g → nobody
Component: DOM → General
Product: Core → Fennec
QA Contact: general → general
Version: unspecified → Trunk
Reporter | ||
Comment 4•14 years ago
|
||
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
Updated•14 years ago
|
Status: NEW → ASSIGNED
Assignee | ||
Updated•14 years ago
|
Assignee: nobody → wjohnston
Assignee | ||
Comment 5•14 years ago
|
||
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.
Assignee | ||
Comment 6•14 years ago
|
||
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?
Attachment #490281 -
Flags: feedback?(ben)
Assignee | ||
Comment 7•14 years ago
|
||
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 8•14 years ago
|
||
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+
Assignee | ||
Comment 9•14 years ago
|
||
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.
Attachment #490281 -
Attachment is obsolete: true
Attachment #491298 -
Attachment is obsolete: true
Attachment #491310 -
Flags: review+
Attachment #490281 -
Flags: feedback?(ben)
Assignee | ||
Updated•14 years ago
|
Whiteboard: [checkin-needed]
Comment 10•14 years ago
|
||
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.
Updated•14 years ago
|
Whiteboard: [checkin-needed]
Comment 11•14 years ago
|
||
I guess we would need to set focus in the Browser:Activate message handler though
Comment 12•14 years ago
|
||
It would be an "active" event, yes?
Assignee | ||
Comment 13•14 years ago
|
||
finkle version. also killed off this._selected stuff that I can't find us using anywhere.
Attachment #491310 -
Attachment is obsolete: true
Attachment #491343 -
Flags: review?(mark.finkle)
Comment 14•14 years ago
|
||
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+
Comment 15•14 years ago
|
||
Status: ASSIGNED → RESOLVED
Closed: 14 years ago
Resolution: --- → FIXED
Assignee | ||
Comment 16•14 years ago
|
||
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)
Updated•14 years ago
|
Attachment #492377 -
Flags: review?(mark.finkle) → review+
Comment 17•14 years ago
|
||
You need to log in
before you can comment on or make changes to this bug.
Description
•