If you think a bug might affect users in the 57 release, please set the correct tracking and status flags for Release Management.

screen.width / screen.height are 0 on Android

RESOLVED FIXED

Status

()

Core
Widget: Android
RESOLVED FIXED
7 years ago
7 years ago

People

(Reporter: mbrubeck, Assigned: azakai)

Tracking

(Blocks: 1 bug)

Trunk
All
Android
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(fennec2.0b3+)

Details

(URL)

Attachments

(2 attachments, 5 obsolete attachments)

(Reporter)

Description

7 years ago
The DOM window.screen.width and window.screen.height properties do not seem to be implemented on Android.
(Reporter)

Updated

7 years ago
tracking-fennec: --- → ?
(Reporter)

Updated

7 years ago
Blocks: 600890

Updated

7 years ago
Assignee: nobody → crowderbt
tracking-fennec: ? → 2.0+
tracking-fennec: 2.0+ → 2.0b2+

Comment 1

7 years ago
Created attachment 481563 [details] [diff] [review]
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

7 years ago
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.
tracking-fennec: 2.0b2+ → ?

Comment 3

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

Updated

7 years ago
Attachment #481563 - Flags: review?(jones.chris.g)
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?
Attachment #481563 - Flags: review?(jones.chris.g)
tracking-fennec: ? → 2.0b3+
Attachment #481563 - Flags: review?(doug.turner)
Whiteboard: [has-patch]

Comment 5

7 years ago
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?
Attachment #481563 - Flags: review?(doug.turner) → review-
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

7 years ago
Does anyone know of a real site for which this patch actually matters?  Or even a test?
(Assignee)

Updated

7 years ago
Assignee: crowderbt → azakai
(Reporter)

Comment 8

7 years ago
Bug 613076 might need a similar patch.
Blocks: 613076
(Assignee)

Comment 9

7 years ago
Created attachment 491680 [details]
test html
(Assignee)

Comment 10

7 years ago
Created attachment 491866 [details] [diff] [review]
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?
Attachment #481563 - Attachment is obsolete: true
Attachment #491866 - Flags: review?(jones.chris.g)
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)
Attachment #491866 - Flags: review?(jones.chris.g) → review-
(Assignee)

Comment 12

7 years ago
Created attachment 492546 [details] [diff] [review]
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.
Attachment #491866 - Attachment is obsolete: true
Attachment #492546 - Flags: review?(jones.chris.g)
(Assignee)

Comment 13

7 years ago
Comment on attachment 492546 [details] [diff] [review]
patch v3

Working on better version folling IRC discussion.
Attachment #492546 - Flags: review?(jones.chris.g)
(Assignee)

Comment 14

7 years ago
Created attachment 492720 [details] [diff] [review]
patch v4

Fixed patch, handles content crashes.
Attachment #492546 - Attachment is obsolete: true
Attachment #492720 - Flags: review?(jones.chris.g)
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.
Attachment #492720 - Flags: review?(jones.chris.g)
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.
(Assignee)

Comment 17

7 years ago
Created attachment 492860 [details] [diff] [review]
patch v5

As requested, rewrote to use the observer service instead, and gfxIntSize.
Attachment #492720 - Attachment is obsolete: true
Attachment #492860 - Flags: review?(jones.chris.g)
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.
Attachment #492860 - Flags: review?(jones.chris.g)
(Assignee)

Comment 19

7 years ago
Created attachment 492881 [details] [diff] [review]
patch v6

Patch with requested changes.
Attachment #492860 - Attachment is obsolete: true
Attachment #492881 - Flags: review?(jones.chris.g)
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".
Attachment #492881 - Flags: review?(jones.chris.g) → review+
(Assignee)

Comment 21

7 years ago
http://hg.mozilla.org/mozilla-central/rev/3bb695b1a788
Status: NEW → RESOLVED
Last Resolved: 7 years ago
Resolution: --- → FIXED
Whiteboard: [has-patch]
You need to log in before you can comment on or make changes to this bug.