600880 - screen.width / screen.height are 0 on Android

Status: RESOLVED FIXED

(Core Graveyard :: Widget: Android, defect)

Description

[Matt Brubeck (:mbrubeck)]

The DOM window.screen.width and window.screen.height properties do not seem to be implemented on Android.

Comment 1

[Brian Crowder]

Attachment: deliver screen bounds over IPC when they change

This is really super ugly and I don't think high-priority.  The Right Thing to do is for the content process NEVER to use widget code for info like this.  In the meantime, content really shouldn't be using this info anyway.

Comment 2

[Brian Crowder]

Can we re-evaluate why we think this bug needs blocking?  In further testing, it is not the source of our issues w/ Google Reader and friends.

Comment 3

[Brian Crowder]

Damnit, this bug does break media queries, though.  I'll get my patch reviewed, even though it makes me feel dirty.

Comment 4

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Comment on attachment 481563 [details] [diff] [review]
deliver screen bounds over IPC when they change

>diff --git a/dom/ipc/PContent.ipdl b/dom/ipc/PContent.ipdl
>--- a/dom/ipc/PContent.ipdl
>+++ b/dom/ipc/PContent.ipdl
>@@ -52,6 +52,7 @@ using ChromePackage;
> using ResourceMapping;
> using OverrideMapping;
> using IPC::URI;
>+using nsIntSize;
> 
> namespace mozilla {
> namespace dom {
>@@ -81,6 +82,8 @@ child:
> 
>     GeolocationUpdate(GeoPosition somewhere);
> 
>+    SetScreenSize(nsIntSize size);

Nit: I would call this |ScreenSizeChanged()| or something.
|SetScreenSize()| makes it sound like you're changing the size of the
actual screen.

>diff --git a/widget/src/android/nsWindow.cpp b/widget/src/android/nsWindow.cpp
>--- a/widget/src/android/nsWindow.cpp
>+++ b/widget/src/android/nsWindow.cpp
>@@ -38,6 +38,14 @@
> #include <android/log.h>
> #include <math.h>
> 
>+#ifdef MOZ_IPC
>+#include "mozilla/dom/ContentParent.h"
>+#include "mozilla/dom/ContentChild.h"
>+#include "mozilla/unused.h"
>+
>+using mozilla::unused;
>+#endif
>+
> #include "nsAppShell.h"
> #include "nsIdleService.h"
> #include "nsWindow.h"
>@@ -670,6 +678,12 @@ nsWindow::OnGlobalAndroidEvent(AndroidGe
>                     gTopLevelWindows[i]->Resize(gAndroidBounds.width, gAndroidBounds.height, PR_TRUE);
>             }
> 
>+#ifdef MOZ_IPC
>+            if (XRE_GetProcessType() != GeckoProcessType_Content) {

XRE_GetProcessType() == GeckoProcessType_Default

>+                mozilla::dom::ContentParent *cp = mozilla::dom::ContentParent::GetSingleton();

using mozilla::dom::ContentParent;

Also, you want to use |GetSingleton(PR_FALSE)| to not force creation
of a new content process.  This code shouldn't be doing that; it can
launch the content process at unexpected times during startup
(possibly) and cause contention for CPU.  We want to have a firm grasp
on when the content process is created.

>@@ -949,6 +963,12 @@ nsWindow::SetInitialAndroidBounds(const 
> gfxIntSize
> nsWindow::GetAndroidBounds()
> {
>+#ifdef MOZ_IPC
>+    if (XRE_GetProcessType() == GeckoProcessType_Content) {
>+        nsIntSize sz = mozilla::dom::ContentChild::GetSingleton()->GetScreenSize();

using mozilla::dom::ContentChild;

This mostly looks OK, except for two things: how does the first
PContent get its screen size set?  That is, how do you know this code
runs after it's been created?  Second, if another PContent is created
after a crash, say, how would it get its screen size set?

Comment 5

[Doug Turner (:dougt)]

Comment on attachment 481563 [details] [diff] [review]
deliver screen bounds over IPC when they change

misused based on comments.  Brian, can you get a new patch?

Comment 6

[Brad Lassey [:blassey] (use needinfo?)]

Comment on attachment 481563 [details] [diff] [review]
deliver screen bounds over IPC when they change

looks like cjones already reviewed this.

Why don't some people use r- anymore?

Comment 7

[Brian Crowder]

Does anyone know of a real site for which this patch actually matters?  Or even a test?

Comment 8

[Matt Brubeck (:mbrubeck)]

Bug 613076 might need a similar patch.

Comment 9

[Alon Zakai (:azakai)]

Attachment: test html

Comment 10

[Alon Zakai (:azakai)]

Attachment: patch v2

Some cleanup and fixes for the comments from before. Except for the last questions about the PContent, not sure what is meant there - is the question what will happen if we have multiple child processes?

Comment 11

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Comment on attachment 491866 [details] [diff] [review]
patch v2

So, the big problems from comment 4 weren't addressed.

 - the resize event can presumably be delivered during startup.  You can't use ContentProcess::GetSingleton() during startup because that will force creation of the content process, and we need tight control over when that happens.  Need to use GetSingleton(PR_FALSE/*don't force creation*/).
 - given the above, you can't rely on there being a ContentParent when the resize is delivered.  If the getter fails, this patch has no way to notify the eventually-created ContentParent of the screen size (other than a new resize event).
 - related to above, if the process corresponding to the first ContentParent crashes, this patch has not way to notify the "next" ContentParent of the screen size (other than a new resize event).
 - (we would have these same problems with multiple content processes)

Comment 12

[Alon Zakai (:azakai)]

Attachment: patch v3

This should address all the concerns except for the crashing issue, which I'm not sure what to do about - what way do we have for getting notified about crashes?

This patch adds the ability for ContentParents to queue events to be run when they are created. So the resize notification will either run immediately if there is a ContentParent, or later when it is created.

Comment 13

[Alon Zakai (:azakai)]

Comment on attachment 492546 [details] [diff] [review]
patch v3

Working on better version folling IRC discussion.

Comment 14

[Alon Zakai (:azakai)]

Attachment: patch v4

Fixed patch, handles content crashes.

Comment 15

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Comment on attachment 492720 [details] [diff] [review]
patch v4

This approach looks good.  However, I think this would be more cleanly and future-, er, -proofly written as an observer-service notification for "content process created" (sigh), fired after a new ContentParent was created.  This would remove all of the state-tracking code in this patch, I think.  The basic idea would be, in android widget code

  onresize:
    if contentparent: notify parent of screen resize
    else: ignore

  observe("content-process-created"):
    notify parent of screen resize

(To make this work with multiple content processes, we'd replace the "notify" with some kind of "broadcast" interface.)

Also, you should use gfxIntSize instead of nsIntSize for the schlepping here, to save on some ugly translation code.

Comment 16

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Also, you should write a test for non-zero screensize after loading a content process and after reloading one after a crash.  Maybe browser-chrome can handle these?  Not sure.  I don't know if we're running tests on android yet, but hopefully we will eventually, and then can catch regressions here.

Comment 17

[Alon Zakai (:azakai)]

Attachment: patch v5

As requested, rewrote to use the observer service instead, and gfxIntSize.

Comment 18

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Comment on attachment 492860 [details] [diff] [review]
patch v5

+bool
>+ContentChild::RecvScreenSizeChanged(const gfxIntSize& size)
>+{
>+#ifdef ANDROID
>+    mScreenSize = size;
>+#else
>+    NS_RUNTIMEABORT("Only Android needs IPC for screen sizes");

Nit: "Message currently only expected on android"

>+#ifdef ANDROID
>+    gfxIntSize mScreenSize;
>+#endif

gfxIntSize doesn't init its members.  Normally it's OK to leave it
alone and use valgrind to catch use-before-init errors, but on android
that's going to be Hard, so let's init to <0, 0> instead (in the hope
that a future test might find errors).

>+            if (obs)
>+                obs->NotifyObservers(nsnull, "content-process-created", nsnull);

I don't particularly care about this, but there might be value in
naming this consistently with content-shutdown, so
"ipc:content-created" or something.

Nit: braces around the statement.

>+    // Android-specific screen size notification
>+    ScreenSizeChanged(gfxIntSize size);

I'd drop this comment.

>+#ifdef MOZ_IPC
>+static PRBool gContentProcessUpdatesRequested = PR_FALSE;

Instead of this global, how about

  static nsCOMPtr<ContentCreationNotifier> gBlah;

and then check |if (!gBlah)|.  (And make sure it's nulled out on
xpcom-shutdown.)

>+    NS_IMETHOD Observe(nsISupports* aSubject,
>+                       const char* aTopic,
>+                       const PRUnichar* aData)
>+    {
>+        if (!strcmp(aTopic, "content-process-created")) {
>+            ContentParent *cp = ContentParent::GetSingleton(PR_FALSE);
>+            NS_ABORT_IF_FALSE(cp, "Must have content process if notified of its creation");
>+            unused << cp->SendScreenSizeChanged(gAndroidBounds);
>+        } else if (!strcmp(aTopic, "xpcom-shutdown")) {
>+            nsCOMPtr<nsIObserverService>
>+                obs(do_GetService("@mozilla.org/observer-service;1"));
>+            if (obs)
>+                obs->RemoveObserver(static_cast<nsIObserver*>(this),
>+                                    "xpcom-shutdown");

Nit: braces around this statement.

> void
> nsWindow::OnGlobalAndroidEvent(AndroidGeckoEvent *ae)
> {
>+#ifdef MOZ_IPC
>+            if (XRE_GetProcessType() == GeckoProcessType_Default) {
>+                if (!gContentProcessUpdatesRequested) {
>+                    nsCOMPtr<nsIObserverService> obs =
>+                        do_GetService("@mozilla.org/observer-service;1");
>+                    if (obs) {
>+                        ContentCreationNotifier *notifier = new ContentCreationNotifier;

|nsCOMPtr<ContentCreationNotifier>| plz, assign to gBlah on success,
 and drop the explicit deletes on failure.

Looks good, but I'd like to see one more rev.

Comment 19

[Alon Zakai (:azakai)]

Attachment: patch v6

Patch with requested changes.

Comment 20

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Comment on attachment 492881 [details] [diff] [review]
patch v6

>+    NS_IMETHOD Observe(nsISupports* aSubject,
>+                       const char* aTopic,
>+                       const PRUnichar* aData)
>+    {
>+        if (!strcmp(aTopic, "ipc:content-created")) {
>+            ContentParent *cp = ContentParent::GetSingleton(PR_FALSE);
>+            NS_ABORT_IF_FALSE(cp, "Must have content process if notified of its creation");
>+            unused << cp->SendScreenSizeChanged(gAndroidBounds);
>+        } else if (!strcmp(aTopic, "xpcom-shutdown")) {
>+            nsCOMPtr<nsIObserverService>
>+                obs(do_GetService("@mozilla.org/observer-service;1"));
>+            if (obs) {
>+                obs->RemoveObserver(static_cast<nsIObserver*>(this),
>+                                    "xpcom-shutdown");
>+            }
>+            gContentCreationNotifier = nsnull;

Need to unhook from "ipc:content-created" too, I think.

Note here that the assignment is expected to delete |this|, "beware".

Comment 21

[Alon Zakai (:azakai)]

http://hg.mozilla.org/mozilla-central/rev/3bb695b1a788