Closed Bug 886496 Opened 8 years ago Closed 8 years ago

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

Categories

(Firefox for Android Graveyard :: General, defect)

All
Android
defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED
Firefox 25

People

(Reporter: bnicholson, Assigned: mfinkle)

References

Details

Attachments

(2 files)

See bug 776928. We should be doing the same thing on Android.
See Also: → 776928
Assignee: nobody → ckitching
Stealing from Chris
Assignee: ckitching → mark.finkle
Attached patch patchSplinter Review
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)
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.
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?
(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
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 25
Attached patch missing-partSplinter Review
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)
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.
Product: Firefox for Android → Firefox for Android Graveyard
You need to log in before you can comment on or make changes to this bug.