Last Comment Bug 610834 - New windows opened don't get painted until resized
: New windows opened don't get painted until resized
Status: VERIFIED FIXED
[inbound]
:
Product: Fennec Graveyard
Classification: Graveyard
Component: General (show other bugs)
: Trunk
: All Android
: -- normal (vote)
: Firefox 9
Assigned To: James Willcox (:snorp) (jwillcox@mozilla.com)
:
Mentors:
Depends on: 678261 678480
Blocks: 672411
  Show dependency treegraph
 
Reported: 2010-11-09 16:10 PST by [:fabrice] Fabrice Desré
Modified: 2011-08-29 15:47 PDT (History)
8 users (show)
mozaakash: in‑litmus?
See Also:
QA Whiteboard:
Iteration: ---
Points: ---


Attachments
wip (5.06 KB, patch)
2010-11-30 14:48 PST, [:fabrice] Fabrice Desré
dougt: feedback-
Details | Diff | Review
wip 2 (4.86 KB, patch)
2010-12-02 16:12 PST, [:fabrice] Fabrice Desré
blassey.bugs: feedback+
Details | Diff | Review
Store the last SIZE_CHANGED event, and resend it for new windows, bug 610834 (6.60 KB, patch)
2011-08-09 09:27 PDT, James Willcox (:snorp) (jwillcox@mozilla.com)
fabrice: review+
Details | Diff | Review

Description [:fabrice] Fabrice Desré 2010-11-09 16:10:25 PST
On Android, when opening a new chrome window its content is white. Resizing eg. by changing orientation causes the content to be painted.
Comment 1 [:fabrice] Fabrice Desré 2010-11-30 14:48:08 PST
Created attachment 494174 [details] [diff] [review]
wip

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.
Comment 2 [:fabrice] Fabrice Desré 2010-12-02 16:12:32 PST
Created attachment 494868 [details] [diff] [review]
wip 2

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.
Comment 3 Brad Lassey [:blassey] (use needinfo?) 2011-05-23 12:21:43 PDT
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?
Comment 4 Brad Lassey [:blassey] (use needinfo?) 2011-07-22 10:24:15 PDT
Fabrice, what's the plan here?
Comment 5 [:fabrice] Fabrice Desré 2011-07-22 12:10:01 PDT
We currently don't plan to open new chromeless windows for apps, so we are not blocked by this bug anymore.
Comment 6 Mark Finkle (:mfinkle) (use needinfo?) 2011-07-22 12:14:40 PDT
(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.
Comment 7 James Willcox (:snorp) (jwillcox@mozilla.com) 2011-08-09 09:27:45 PDT
Created attachment 551792 [details] [diff] [review]
Store the last SIZE_CHANGED event, and resend it for new windows, bug 610834
Comment 8 James Willcox (:snorp) (jwillcox@mozilla.com) 2011-08-09 09:28:39 PDT
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.
Comment 9 Brad Lassey [:blassey] (use needinfo?) 2011-08-09 22:11:22 PDT
http://hg.mozilla.org/integration/mozilla-inbound/rev/f4acfd3b1ce8
Comment 10 :Ehsan Akhgari (busy, don't ask for review please) 2011-08-10 08:41:16 PDT
http://hg.mozilla.org/mozilla-central/rev/f4acfd3b1ce8
Comment 11 Matt Brubeck (:mbrubeck) 2011-08-11 16:34:23 PDT
Backed out because of a tp4m regression (bug 678261):
https://hg.mozilla.org/integration/mozilla-inbound/rev/6a9dabc3f3d1
Comment 12 Mark Finkle (:mfinkle) (use needinfo?) 2011-08-11 20:18:35 PDT
(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.
Comment 13 James Willcox (:snorp) (jwillcox@mozilla.com) 2011-08-18 16:37:01 PDT
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.
Comment 14 Matt Brubeck (:mbrubeck) 2011-08-18 17:06:38 PDT
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.
Comment 15 Marco Bonardo [::mak] 2011-08-19 03:21:51 PDT
http://hg.mozilla.org/mozilla-central/rev/46b3d16fc427
Comment 16 Andreea Pod 2011-08-24 06:51:49 PDT
What is a chrome window and how to open one? I am trying to verify this bug :).
Comment 17 James Willcox (:snorp) (jwillcox@mozilla.com) 2011-08-24 06:55:57 PDT
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.
Comment 18 Andreea Pod 2011-08-29 05:04:05 PDT
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

Note You need to log in before you can comment on or make changes to this bug.