476186 - Ts regression on 2009-01-28

Status: VERIFIED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

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

http://graphs.mozilla.org/#show=2774468,2774466,2774464&sel=1233156301,1233189526

Looks like this might have been caused by:
http://hg.mozilla.org/mobile-browser/rev/906a16fa0b72

as that is the only m-c or m-b checkin during the regression range.

Comment 1

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

I don't think we want to just backout:
http://hg.mozilla.org/mobile-browser/rev/906a16fa0b72

We could look at why the Ts regression is happening because of the | browser.docShell.isOffScreen = true; | change. There might be a lot of events occurring that weren't previously.

Comment 2

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

Attachment: patch

This patch skips setting the isOffScreenBrowser flag for the first load, then sets the flag when the page loads.

This should save the Ts regression while passing the focus bugs - I did test a manual testcase.

Comment 3

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

Not asking for review yet. I'm not exactly happy we have to hack things for the first load.

Comment 4

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

Comment on attachment 360437 [details] [diff] [review]
patch

sorry for the whitespace in this patch - I need to upgrade to hg 1.1.2 for a better qdiff

Comment 5

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

Comment on attachment 360437 [details] [diff] [review]
patch

Since setting isOffScreenBrowser is very cheap, I'll ask for a review on this.

http://mxr.mozilla.org/mozilla-central/source/docshell/base/nsDocShell.cpp#4061

Comment 6

[:Gavin Sharp [email: [e-mail]]]

Comment on attachment 360437 [details] [diff] [review]
patch

I'd kind of like to understand why this would have so much of a Ts impact; I don't see any obvious reasons. I wonder if delaying whatever events this patch is preventing from firing during startup would be worth it in general (e.g. in Firefox)...

Comment 7

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

http://hg.mozilla.org/mobile-browser/rev/6a63cba37144

Comment 8

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

Ts is back to previous levels:
http://graphs.mozilla.org/#show=2774468,2774466,2774464&sel=1233886948,1233913589

Comment 9

[Joel Maher ( :jmaher ) (UTC -8)]

we have since fixed this...marking as verified