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

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[Brian Nicholson (:bnicholson)]

See bug 776928. We should be doing the same thing on Android.

Comment 1

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

Stealing from Chris

Comment 2

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

Attachment: 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.

Comment 3

[Brian Nicholson (: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?

Comment 4

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

(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.

Comment 5

[Kartikaya Gupta (email:[e-mail])]

(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.

Comment 6

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

(In reply to Kartikaya Gupta (email:[e-mail]) 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?

Comment 7

[Richard Newman [:rnewman]]

Nick: any input to Comment 6?

Comment 8

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

https://hg.mozilla.org/integration/mozilla-inbound/rev/d218deb5ac96

Comment 9

[Geoff Brown [:gbrown]]

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.

Comment 10

[Nick Alexander :nalexander [he/him]]

(In reply to Mark Finkle (:mfinkle) from comment #6)
> (In reply to Kartikaya Gupta (email:[e-mail]) 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.

Comment 11

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/d218deb5ac96

Comment 12

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

Attachment: 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.

Comment 13

[Brian Nicholson (:bnicholson)]

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

Oh, I just assumed this was already there since browser.css already existed.

Comment 14

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

part 2: https://hg.mozilla.org/integration/mozilla-inbound/rev/8eddb7c2ca23

Comment 15

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

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.

Comment 16

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/8eddb7c2ca23