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)

defect
Not set
normal

Tracking

(fennec2.0b3+)

RESOLVED FIXED
Tracking Status
fennec 2.0b3+ ---

People

(Reporter: cjones, Assigned: wesj)

References

Details

Attachments

(2 files, 3 obsolete files)

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.
Blocks: 596969
(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
No longer blocks: 596969
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+
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
Status: NEW → ASSIGNED
Assignee: nobody → wjohnston
Attached patch Patch v1 (obsolete) — Splinter Review
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?
Attachment #490281 - Flags: feedback?(ben)
Attached patch Patch v2 (obsolete) — Splinter Review
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+
Attached patch Patch v3 (obsolete) — Splinter 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.
Attachment #490281 - Attachment is obsolete: true
Attachment #491298 - Attachment is obsolete: true
Attachment #491310 - Flags: review+
Attachment #490281 - Flags: feedback?(ben)
Whiteboard: [checkin-needed]
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.
Whiteboard: [checkin-needed]
I guess we would need to set focus in the Browser:Activate message handler though
It would be an "active" event, yes?
Attached patch Fixes v3Splinter Review
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 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+
pushed:
http://hg.mozilla.org/mobile-browser/rev/6028c62dfbc0
Status: ASSIGNED → RESOLVED
Closed: 14 years ago
Resolution: --- → FIXED
Attached patch Fix namingSplinter Review
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+
Depends on: 638944
You need to log in before you can comment on or make changes to this bug.