Closed Bug 610834 Opened 9 years ago Closed 9 years ago

New windows opened don't get painted until resized

Categories

(Firefox for Android Graveyard :: General, defect)

All
Android
defect
Not set

Tracking

(firefox8 fixed, fennec8+)

VERIFIED FIXED
Firefox 9
Tracking Status
firefox8 --- fixed
fennec 8+ ---

People

(Reporter: fabrice, Assigned: snorp)

References

Details

(Whiteboard: [inbound])

Attachments

(2 files, 1 obsolete file)

On Android, when opening a new chrome window its content is white. Resizing eg. by changing orientation causes the content to be painted.
tracking-fennec: --- → ?
Flags: in-litmus?
tracking-fennec: ? → 2.0-
Attached patch wip (obsolete) — Splinter Review
Android sends resize events only when the surface view change (in GeckoSurfaceView::surfaceChanged()). This method sends a SIZE_CHANGED event to gecko.

The approach here is to remember the last SIZE_CHANGED event and resend it to force a resize when we put a window to front.

The front-end gets events, but ignores them because the event target is not set correctly.

I also tried to simply call OnSizeChanged(gAndroidBounds) in BringToFront(), with no better results.
Assignee: nobody → fabrice
Attachment #494174 - Flags: feedback?(mwu)
Attachment #494174 - Flags: feedback?(doug.turner)
Attachment #494174 - Flags: feedback?(doug.turner) → feedback-
Attached patch wip 2Splinter Review
This new patch makes sure that the child windows of the toplevel one get also resized as they should. But even if their OnResizeHandler() is called, the front-end still tells that the event's target is not the good window.
Attachment #494174 - Attachment is obsolete: true
Attachment #494174 - Flags: feedback?(mwu)
Whiteboard: [fennec-4.1?]
Attachment #494868 - Flags: feedback?(blassey.bugs)
tracking-fennec: - → 7+
Whiteboard: [fennec-4.1?]
Comment on attachment 494868 [details] [diff] [review]
wip 2

Review of attachment 494868 [details] [diff] [review]:
-----------------------------------------------------------------

overall this looks fine, just a few questions about the implementation details

::: widget/src/android/AndroidJavaWrappers.cpp
@@ +355,5 @@
> +    mTime = other->mTime;
> +    mP0.x = other->mP0.x;
> +    mP0.y = other->mP0.y;
> +    mP1.x = other->mP1.x;
> +    mP1.y = other->mP1.y;

why not use a copy constructor and add a method to change the type?

::: widget/src/android/nsAppShell.cpp
@@ +73,4 @@
>  nsAccelerometerSystem *gAccel = nsnull;
>  nsIGeolocationUpdate *gLocationCallback = nsnull;
>  
> +nsAutoPtr<mozilla::AndroidGeckoEvent> gLastSizeChanged;

Can we just construct a new event based on mBounds rather than keep a reference to the old event?
Attachment #494868 - Flags: feedback?(blassey.bugs) → feedback+
Fabrice, what's the plan here?
tracking-fennec: 7+ → 8+
We currently don't plan to open new chromeless windows for apps, so we are not blocked by this bug anymore.
(In reply to comment #5)
> We currently don't plan to open new chromeless windows for apps, so we are
> not blocked by this bug anymore.

But we do open new windows for other things. Currently, we open the Locale Picker in a new window. After the locale is "picked", the browser.xul window is opened.

We have the same hack/fix in browser.js for locale picker as we did for webapps.

We should fix this issue.
Assignee: fabrice → snorp
Comment on attachment 551792 [details] [diff] [review]
Store the last SIZE_CHANGED event, and resend it for new windows, bug 610834

This fixes it for me.
Attachment #551792 - Flags: review?(fabrice)
Attachment #551792 - Flags: review?(fabrice) → review+
http://hg.mozilla.org/mozilla-central/rev/f4acfd3b1ce8
Status: NEW → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Whiteboard: [inbound]
Target Milestone: --- → Firefox 8
Depends on: 678261
Backed out because of a tp4m regression (bug 678261):
https://hg.mozilla.org/integration/mozilla-inbound/rev/6a9dabc3f3d1
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Target Milestone: Firefox 8 → ---
(In reply to Matt Brubeck (:mbrubeck) from comment #11)
> Backed out because of a tp4m regression (bug 678261):
> https://hg.mozilla.org/integration/mozilla-inbound/rev/6a9dabc3f3d1

It's strange that this patch would have regressed Tp. We only open 1 XUL (app) window for the Tp4 test. A set of webpages are then loaded into the browser.

I wonder if this patch was firing a chrome event for triggers from web content DOM windows? Ts wasn't affect so it seems like we were doing a tiny bit of extra work compounded over ~30 pageloads that happen during Tp testing. The single window open that happens during Ts wasn't enough to cause a regression.
Without this patch, the window size is totally busted during the Talos runs.  With the patch it is correct.  So I think the regression must just be a side effect of having a correct window size.  I also checked that no additional resizes are being generated.
Keywords: checkin-needed
Re-landed on inbound:
https://hg.mozilla.org/integration/mozilla-inbound/rev/46b3d16fc427

I posted a message to dev-tree-management about the expected tp4m hit.
Keywords: checkin-needed
Whiteboard: [inbound]
http://hg.mozilla.org/mozilla-central/rev/46b3d16fc427
Status: REOPENED → RESOLVED
Closed: 9 years ago9 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 9
Blocks: 672411
What is a chrome window and how to open one? I am trying to verify this bug :).
The easiest way to make sure things are still working:

1) Set android device locale to something obscure
2) 'clear data' for the fennec application
3) Start fennec and see the locale picker
4) Pick a locale
5) Profit

If in the end the browser window is sized correctly, then things are working as intended.
Verified fixed following the steps from comment 17, build: Mozilla /5.0 (Android;Linux armv7l;rv:8.0a2) Gecko/20110828 Firefox/8.0a2 Fennec/8.0a2
Device: LG Optimus 2X
Status: RESOLVED → VERIFIED
Depends on: 678480
You need to log in before you can comment on or make changes to this bug.