When doing lazy tab restoration, set the <browser>s in question to display:none until restored

RESOLVED FIXED in Firefox 25

Status

()

RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: bnicholson, Assigned: mfinkle)

Tracking

Trunk
Firefox 25
All
Android
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments)

(Reporter)

Description

5 years ago
See bug 776928. We should be doing the same thing on Android.
(Reporter)

Updated

5 years ago
See Also: → bug 776928

Updated

5 years ago
Assignee: nobody → ckitching
Stealing from Chris
Assignee: ckitching → mark.finkle
Created attachment 770637 [details] [diff] [review]
patch

I tried to follow the desktop patch in this one:
* Use an attribute on the <browser> to control CSS for display:none
* Any time we are delaying the load or zombifying the tab, we set the attribute
* Any time we are restoring the tab, we remove the attribute

I tried testing on a low-mem phone to exercise the restore. Things looked OK, but we'll need more testing. I don't know off-hand of a good automated test for this. If the display is stuck at none, nothing appears. Maybe a test using the PixelTest code could work? We could try to force a zombie.
Attachment #770637 - Flags: review?(bnicholson)
(Reporter)

Comment 3

5 years ago
Comment on attachment 770637 [details] [diff] [review]
patch

Review of attachment 770637 [details] [diff] [review]:
-----------------------------------------------------------------

The session restore changes look fine, but why are you removing the scroller styles in browser.css?
(In reply to Brian Nicholson (:bnicholson) from comment #3)
> Comment on attachment 770637 [details] [diff] [review]
> patch
> 
> Review of attachment 770637 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> The session restore changes look fine, but why are you removing the scroller
> styles in browser.css?

Good question. Turns out, browser.css was no longer being used. We never included it in browser.xul or loaded it dynamically. We do still use content.css to hide scrollbars in content and do lots of other stuff.

But browser.css was a remnant from XUL Fennec. I removed the unused code and added the browser["pending"] part. I also load it in browser.xul now.
(In reply to Mark Finkle (:mfinkle) from comment #2)
> I tried testing on a low-mem phone to exercise the restore. Things looked
> OK, but we'll need more testing. I don't know off-hand of a good automated
> test for this. If the display is stuck at none, nothing appears. Maybe a
> test using the PixelTest code could work? We could try to force a zombie.

You can simulate memory pressure and trigger zombification of tabs by broadcasting the org.mozilla.gecko.FORCE_MEMORY_PRESSURE intent.
(Reporter)

Updated

5 years ago
Attachment #770637 - Flags: review?(bnicholson) → review+
(In reply to Kartikaya Gupta (email:kats@mozilla.com) from comment #5)
> (In reply to Mark Finkle (:mfinkle) from comment #2)
> > I tried testing on a low-mem phone to exercise the restore. Things looked
> > OK, but we'll need more testing. I don't know off-hand of a good automated
> > test for this. If the display is stuck at none, nothing appears. Maybe a
> > test using the PixelTest code could work? We could try to force a zombie.
> 
> You can simulate memory pressure and trigger zombification of tabs by
> broadcasting the org.mozilla.gecko.FORCE_MEMORY_PRESSURE intent.

I thought about the testing part a bit more. This would be really easy to test, if I could run chrome JS code. I can easily create a new delayed Tab. I really want to then see if that Tab's <browser> has the "pending" attribute set. Then I would select the delayed Tab, making it fully load. Again, I would test the Tab's <browser> to see that the "pending" attribute has been removed.

I am stuck on finding a way to run the chrome JS to read the attribute. Anyone have ideas?
Nick: any input to Comment 6?
You can use https://developer.mozilla.org/en-US/docs/SpecialPowers in JS loaded from robocop...does that help? See http://mxr.mozilla.org/mozilla-central/source/mobile/android/base/tests/robocop_testharness.js, or perhaps bug 855146, part 5, for a simpler example.
(In reply to Mark Finkle (:mfinkle) from comment #6)
> (In reply to Kartikaya Gupta (email:kats@mozilla.com) from comment #5)
> > (In reply to Mark Finkle (:mfinkle) from comment #2)
> > > I tried testing on a low-mem phone to exercise the restore. Things looked
> > > OK, but we'll need more testing. I don't know off-hand of a good automated
> > > test for this. If the display is stuck at none, nothing appears. Maybe a
> > > test using the PixelTest code could work? We could try to force a zombie.
> > 
> > You can simulate memory pressure and trigger zombification of tabs by
> > broadcasting the org.mozilla.gecko.FORCE_MEMORY_PRESSURE intent.
> 
> I thought about the testing part a bit more. This would be really easy to
> test, if I could run chrome JS code. I can easily create a new delayed Tab.
> I really want to then see if that Tab's <browser> has the "pending"
> attribute set. Then I would select the delayed Tab, making it fully load.
> Again, I would test the Tab's <browser> to see that the "pending" attribute
> has been removed.
> 
> I am stuck on finding a way to run the chrome JS to read the attribute.
> Anyone have ideas?

mfinkle, the javascript test code run by TestJavascript from Bug 870908 runs in a sandboxed chrome context.  You have access to only Components at the moment, but you could provide an accessor to the global browser or something similar.  The sandbox is set up at http://mxr.mozilla.org/mozilla-central/source/mobile/android/base/tests/robocop_testharness.js#53.  See Bug 870908 for the patches and testSharedPreferences.js for examples.
https://hg.mozilla.org/mozilla-central/rev/d218deb5ac96
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 25
Created attachment 774781 [details] [diff] [review]
missing-part

Not sure how I missed adding this part. It's kinda crucial to making it all work. I even said I was doing it in a comment.
Attachment #774781 - Flags: review?(bnicholson)
(Reporter)

Comment 13

5 years ago
Comment on attachment 774781 [details] [diff] [review]
missing-part

Oh, I just assumed this was already there since browser.css already existed.
Attachment #774781 - Flags: review?(bnicholson) → review+
Wes, Geoff and Nick offered solutions to my testing issue. I think I'll look into adding a "chromeWindow" reference into the scope of the JS tests like Nick suggested. This might be the cheapest, easiest way to get something working. The build parts are already in place if I use this approach.
You need to log in before you can comment on or make changes to this bug.