If you think a bug might affect users in the 57 release, please set the correct tracking and status flags for Release Management.

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

RESOLVED FIXED

Status

Fennec Graveyard
General
RESOLVED FIXED
7 years ago
7 years ago

People

(Reporter: cjones, Assigned: wesj)

Tracking

(Depends on: 1 bug)

Dependency tree / graph

Details

Attachments

(2 attachments, 3 obsolete attachments)

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: --- → ?

Updated

7 years ago
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)

Updated

7 years ago
Assignee: nobody → wjohnston
(Assignee)

Comment 5

7 years ago
Created attachment 490281 [details] [diff] [review]
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.
(Assignee)

Comment 6

7 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

7 years ago
Created attachment 491298 [details] [diff] [review]
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.
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+
(Assignee)

Comment 9

7 years ago
Created attachment 491310 [details] [diff] [review]
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.
Attachment #490281 - Attachment is obsolete: true
Attachment #491298 - Attachment is obsolete: true
Attachment #491310 - Flags: review+
Attachment #490281 - Flags: feedback?(ben)
(Assignee)

Updated

7 years ago
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?
(Assignee)

Comment 13

7 years ago
Created attachment 491343 [details] [diff] [review]
Fixes v3

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
Last Resolved: 7 years ago
Resolution: --- → FIXED
(Assignee)

Comment 16

7 years ago
Created attachment 492377 [details] [diff] [review]
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.
Attachment #492377 - Flags: review?(mark.finkle)
Attachment #492377 - Flags: review?(mark.finkle) → review+
pushed fix:
http://hg.mozilla.org/mobile-browser/rev/7a1fe06d33af
Depends on: 638944
You need to log in before you can comment on or make changes to this bug.