627293 - Fennec not resized correctly after switching between portrait/landscape mode

Status: VERIFIED FIXED

(Firefox for Android Graveyard :: General, defect, P2)

Description

[Oleg Romashin (:romaxa)]

Open URL in landscape mode on N900 Gtk fennec
When page loaded rotate device to portrait mode.
Rotate device to landscape mode back.

Result:
page content not resized correctly, right part of the page is white, and page has 480px width

Comment 1

[Oleg Romashin (:romaxa)]

hav not seen this before, seems was broken recently

Comment 2

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

Attachment: Patch

Comment 3

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

Comment on attachment 505476 [details] [diff] [review]
Patch

Wait until beta 4 is branched before landing

Comment 4

[Anna (Waverley)]

I can reproduce this on Motorola Droid 2 (android 2.2) and HTC Desire (android 2.1), but I might say that only when loading pages with heavy content (e.g.: www.nypost.com, www.profm.ro).

Build ID: Mozilla /5.0 (Android;Linux armv7l;rv:2.0b10pre) Gecko/20110120 Firefox/4.0b10pre Fennec /4.0b4pre

Comment 5

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

pushed for b4:
http://hg.mozilla.org/mobile-browser/rev/ed270e405bc9

Comment 6

[Stuart Parmenter]

This bug was a regression, do we know what push caused it to happen?

Comment 7

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

I backed this out due to a Ts regression:

Previous: avg 6902.433 stddev 36.546 of 30 runs up to revision 3fe77ff50b3a
New     : avg 6994.956 stddev 7.257 of 5 runs since revision ed270e405bc9
Change  : +92.523 (1.34% / z=2.532)
Graph   : http://mzl.la/fO3Gva

I verified that this patch is the cause of the regression. We should take a look at the code and see what could be causing the Ts hit.

Comment 8

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

Attachment: Patch v0.2

Could we try that?
This patch has the nice effect of handling the softkb-change case only for Meego since we don't need it at all on Maemo 5.

Also during startup since Maemo is fullscreen this patch does not bother doing twice the work of the resizeHandler (2 events are seen on my device).

Comment 9

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

btw this also fixed bug 627337

Comment 10

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

Comment on attachment 506725 [details] [diff] [review]
Patch v0.2

>diff --git a/app/mobile.js b/app/mobile.js

>+
>+pref("browser.dom.window.dump.enabled", true);

Left over from debugging?

>diff --git a/chrome/content/bindings/browser.xml b/chrome/content/bindings/browser.xml

>+
>+                // XXX Actually this fire a SetCacheViewport event as well as the
>+                // scrolledAreaChanged handler in content/chrome/browser.js
>                 view._updateCacheViewport();

Is this a bad thing? Why the XXX? Should we fix something here?

>diff --git a/chrome/content/browser.js b/chrome/content/browser.js

>       // XXX is this code right here actually needed?
>+      // It was for WinCE iirc (vn)
>       let maximize = (document.documentElement.getAttribute("sizemode") == "maximized");
>       if (maximize && w > screen.width)
>+        return;

Can we just remove this "maximized" bloc of code? We don't seem to need it.

Comment 11

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

http://hg.mozilla.org/mobile-browser/rev/338b337f98af

(In reply to comment #10)
> >+pref("browser.dom.window.dump.enabled", true);
> 
> Left over from debugging?

obviously...

> >diff --git a/chrome/content/bindings/browser.xml b/chrome/content/bindings/browser.xml
 
> >+                // XXX Actually this fire a SetCacheViewport event as well as the
> >+                // scrolledAreaChanged handler in content/chrome/browser.js
> >                 view._updateCacheViewport();
> 
> Is this a bad thing? Why the XXX? Should we fix something here?

There is a need to reorganise a few things here to not send the SetCacheViewport message twice to the content with the same area to cache (imo)

 
> >diff --git a/chrome/content/browser.js b/chrome/content/browser.js
> >+      // It was for WinCE iirc (vn)
> >       let maximize = (document.documentElement.getAttribute("sizemode") == "maximized");

> Can we just remove this "maximized" bloc of code? We don't seem to need it.

Removed.

I'm keeping this bug opened until we are sure there is no Ts regression anymore.

Comment 12

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

Attachment: Patch - followup

This followup prevent calling updateDefaultZoomLevel on startup.

Comment 13

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

Comment on attachment 506775 [details] [diff] [review]
Patch - followup

worth a shot

Comment 14

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

Comment on attachment 506775 [details] [diff] [review]
Patch - followup

pushed:
http://hg.mozilla.org/mobile-browser/rev/3e455d26e502

still no change in Ts. let's do some profiling to see what these patches actually changed. time to stop guessing.

Comment 15

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

Filed bug 628995 for the Ts regression. We'll leave this patch in for beta 4, but we need to fix the Ts regression ASAP

Comment 16

[Anna (Waverley)]

VERIFIED FIX

Build ID: Mozilla /5.0 (Android;Linux armv7l;rv:2.0b11pre) Gecko/20110127
Firefox/4.0b11pre Fennec /4.0b5pre