589337 - Sort out remote frame destroying/recreating and maybe showing/hiding/

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

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

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

(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

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

(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

Comment 3

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

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.

Comment 4

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

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

Comment 5

[Wesley Johnston (:wesj)]

Attachment: Patch v1

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 6

[Wesley Johnston (:wesj)]

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?

Comment 7

[Wesley Johnston (:wesj)]

Attachment: Patch v2

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.

Comment 8

[Benjamin Stover (:stechz)]

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.

Comment 9

[Wesley Johnston (:wesj)]

Attachment: Patch v3

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.

Comment 10

[Mark Finkle (:mfinkle) (use needinfo?)]

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.

Comment 11

[Mark Finkle (:mfinkle) (use needinfo?)]

I guess we would need to set focus in the Browser:Activate message handler though

Comment 12

[Benjamin Stover (:stechz)]

It would be an "active" event, yes?

Comment 13

[Wesley Johnston (:wesj)]

Attachment: Fixes v3

finkle version. also killed off this._selected stuff that I can't find us using anywhere.

Comment 14

[Mark Finkle (:mfinkle) (use needinfo?)]

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.

Comment 15

[Mark Finkle (:mfinkle) (use needinfo?)]

pushed:
http://hg.mozilla.org/mobile-browser/rev/6028c62dfbc0

Comment 16

[Wesley Johnston (:wesj)]

Attachment: Fix naming

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.

Comment 17

[Mark Finkle (:mfinkle) (use needinfo?)]

pushed fix:
http://hg.mozilla.org/mobile-browser/rev/7a1fe06d33af