Closed Bug 627293 Opened 9 years ago Closed 9 years ago

Fennec not resized correctly after switching between portrait/landscape mode

Categories

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

ARM
All

Tracking

(fennec2.0b4+)

VERIFIED FIXED
fennec2.0b4
Tracking Status
fennec 2.0b4+ ---

People

(Reporter: romaxa, Assigned: vingtetun)

References

()

Details

(Keywords: regression)

Attachments

(2 files, 1 obsolete file)

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
hav not seen this before, seems was broken recently
Keywords: regression
Attached patch Patch (obsolete) — Splinter Review
Assignee: nobody → 21
Attachment #505476 - Flags: review?(mark.finkle)
Comment on attachment 505476 [details] [diff] [review]
Patch

Wait until beta 4 is branched before landing
Attachment #505476 - Flags: review?(mark.finkle) → review+
Whiteboard: [fennec-checkin-postb4]
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
OS: Maemo → All
Severity: normal → major
Priority: -- → P2
tracking-fennec: --- → 2.0b4+
pushed for b4:
http://hg.mozilla.org/mobile-browser/rev/ed270e405bc9
Status: NEW → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Whiteboard: [fennec-checkin-postb4]
Target Milestone: --- → fennec2.0b4
This bug was a regression, do we know what push caused it to happen?
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.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Attached patch Patch v0.2Splinter Review
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).
Attachment #505476 - Attachment is obsolete: true
Attachment #506725 - Flags: review?(mark.finkle)
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.
Attachment #506725 - Flags: review?(mark.finkle) → review+
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.
Attached patch Patch - followupSplinter Review
This followup prevent calling updateDefaultZoomLevel on startup.
Attachment #506775 - Flags: review?(mark.finkle)
Comment on attachment 506775 [details] [diff] [review]
Patch - followup

worth a shot
Attachment #506775 - Flags: review?(mark.finkle) → review+
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.
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
Status: REOPENED → RESOLVED
Closed: 9 years ago9 years ago
Resolution: --- → FIXED
VERIFIED FIX

Build ID: Mozilla /5.0 (Android;Linux armv7l;rv:2.0b11pre) Gecko/20110127
Firefox/4.0b11pre Fennec /4.0b5pre
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.