648250 - image based navigational bar is useless in remote Fennec (window.outerWidth throws js errors)

Status: RESOLVED FIXED

(Core :: IPC, defect)

Description

[Mikhail Sobolev]

User-Agent:       Mozilla/5.0 (Macintosh; Intel Mac OS X 10.5; rv:2.0) Gecko/20100101 Firefox/4.0
Build Identifier: Mozilla/5.0 (Andoid; Linux armv7l; rv:2.1) Gecko/20110318 Firefox/40b13pre Fennec/4.0

The site above has a navigational bar using images.  On my device Firefox shows only vertical separators between the items.

Reproducible: Always

Steps to Reproduce:
1. Open http://www.finnkino.fi/eng/
Actual Results:  
The navigational menu (second gray horizontal bar) has only vertical separators.

Expected Results:  
The navigational menu has menu items.

HW: HTC Incredible S S710e
SW: Android 2.2.1

Screenshots were produced using Firefox 4 on Mac OS X and Firefox for mobile on the same machine.

Comment 1

[Mikhail Sobolev]

Attachment: Screenshot of the actual result

The screenshot is produced using Firefox for mobile on Mac OS X

Comment 2

[Mikhail Sobolev]

Attachment: Screenshot of the expected behaviour

The screenshot is produced using Firefox 4 on Mac OS X (with flash blocker)

Comment 3

[Oleg Romashin (:romaxa)]

Reproducible also on desktop fennec build, might be a recent regression, would be nice to check earlier builds and find regression range

Comment 4

[Martijn Wargers (dead)]

I'm seeing this error in the error console.
uncaught exception: [Exception... "Component returned failure code: 0x80004005 (NS_ERROR_FAILURE) [nsIDOMWindowInternal.outerWidth]"  nsresult: "0x80004005 (NS_ERROR_FAILURE)"  location: "JS frame :: http://www.finnkino.fi/scripts/all.js :: <TOP_LEVEL> :: line 1"  data: no]

This seems to be causing the rest of the scripts not to be run anymore, which then seems to cause the navigational bar to not show up.

Comment 5

[Martijn Wargers (dead)]

Attachment: testcase

Comment 6

[Martijn Wargers (dead)]

This is basically the same as bug 614650.
I don't think window.outerWidth should trigger an error, fwiw.

Comment 7

[Tatiana Meshkova (:tatiana)]

###!!! ASSERTION: TabChild::GetDimensions not supported in TabChild: 'Not Reached', file mozilla-central/dom/ipc/TabChild.cpp, line 263
...
JavaScript error: , line 0: uncaught exception: [Exception... "Component returned failure code: 0x80004005 (NS_ERROR_FAILURE) [nsIDOMWindowInternal.outerWidth]"  nsresult: "0x80004005 (NS_ERROR_FAILURE)"  location: "JS frame :: http://www.finnkino.fi/scripts/all.js :: <TOP_LEVEL> :: line 1"  data: no]

Comment 8

[Martijn Wargers (dead)]

Other window properties that trigger this error that are probably related to this:
outerHeight
screenX
screenY

Comment 9

[Oleg Romashin (:romaxa)]

Attachment: Fix for this testcase

kind of fixing this particular problem, tested and works fine.
I think it would be nice to make popular Finnish site working, and it should be safe to

Comment 10

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 524871 [details] [diff] [review]
Fix for this testcase

Does the patch really make window.outerWidth to return the right value?
Sure, the patch makes it to return some value, isn't is the innerWidth?

Comment 12

[Oleg Romashin (:romaxa)]

Attachment: This should be better

I guess this should be better way of handling outer size... stil need to find where to get browser X/Y position... IIUC this is toplevel nsIWidget screen position is it?

Comment 13

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 526891 [details] [diff] [review]
This should be better

I don't still understand how does this handle outerWidth/Height.
Those should include the size of browser chrome.
Or am I missing something here?

Comment 14

[Oleg Romashin (:romaxa)]

I've checked UI process resize calls, and UI process is sending resize with width/height values equal to browser chrome size (size of toplevel window).
Probably it is better to add resize listener in TabParent, and send changes to child process using new Tab/Parent/Child API.

Comment 15

[Olli Pettay [:smaug][[e-mail]]]

(In reply to comment #14)
> I've checked UI process resize calls, and UI process is sending resize with
> width/height values equal to browser chrome size (size of toplevel window).
Hmm, I don't understand how it gets the size of the chrome window.
Am I reading wrong code or something...

Comment 18

[Oleg Romashin (:romaxa)]

I'm trying to understand what is the right object which is returning right values for test https://bugzilla.mozilla.org/attachment.cgi?id=524410...

When I'm running it with Gtk FF: window.outerWidth=1353
When I'm opening same test in Fennec(with attached patch), Opera, Google Chrome: I have: window.outerWidth=968

So where is the true? is all browser except FF returning by default size of inner window?

Comment 19

[Oleg Romashin (:romaxa)]

Also tested on fennec with remote.tabs disabled and also see window.outerWidth=968

Comment 20

[Oleg Romashin (:romaxa)]

Attachment: Is it better direction?

Comment 21

[Oleg Romashin (:romaxa)]

Attachment: Outer dimensions impl v3

Also added screenX/Y implementation

Comment 22

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 546019 [details] [diff] [review]
Outer dimensions impl v3

>+nsFrameLoader::GetOuterDimensions(nsRect& aRect)
>+{
>+  // Need to get outer window position here
>+  nsIDocument* doc = mOwnerContent->GetDocument();
>+  if (!doc) {
>+    return NS_ERROR_FAILURE;
>+  }
>+  if (doc->GetDisplayDocument()) {
>+    // Don't allow subframe loads in external reference documents
Copy-pasted comment from somewhere.


>+
>+  nsAutoString value;
>+  mOwnerContent->GetAttr(kNameSpaceID_None, nsGkAtoms::type, value);
>+
>+  if (!value.LowerCaseEqualsLiteral("content") &&
>+      !StringBeginsWith(value, NS_LITERAL_STRING("content-"),
>+                        nsCaseInsensitiveStringComparator())) {
>+    return NS_ERROR_FAILURE;
>+  }
You could just assert is !mIsTopLevelContent


> TabChild::GetDimensions(PRUint32 aFlags, PRInt32* aX,
>                              PRInt32* aY, PRInt32* aCx, PRInt32* aCy)
> {
>-  NS_NOTREACHED("TabChild::GetDimensions not supported in TabChild");
>+  if (aX)
>+    *aX = mOuterRect.x;
>+  if (aY)
>+    *aY = mOuterRect.y;
>+  if (aCx)
>+    *aCx = mOuterRect.width;
>+  if (aCy)
>+    *aCy = mOuterRect.height;

Coding style should be
if (expr) {
  stmt;
}

(I know TabChild's coding style isn't really consistent)


>+TabChild::RecvUpdateDimensions(const nsRect& rect)
>+{
>+    printf("[TabChild] RESIZE to (x,y,w,h)= (%ud, %ud, %ud, %ud)\n", rect.x, rect.y, rect.width, rect.height);
Make the printf #ifdef DEBUG

>+++ b/dom/ipc/TabParent.h
>@@ -124,11 +124,12 @@ public:
>     void LoadURL(nsIURI* aURI);
>     // XXX/cjones: it's not clear what we gain by hiding these
>     // message-sending functions under a layer of indirection and
>     // eating the return values
>     void Show(const nsIntSize& size);
>-    void Move(const nsIntSize& size);
>+    void Move( const nsIntSize& size);
?

Comment 23

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 546019 [details] [diff] [review]
Outer dimensions impl v3

Couldn't you merge Move and UpdateDimensions?

Comment 24

[Oleg Romashin (:romaxa)]

Attachment: Outer dimensions impl v4

Comment 25

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 546047 [details] [diff] [review]
Outer dimensions impl v4


> NS_IMETHODIMP
>+nsFrameLoader::GetOuterDimensions(nsRect& aRect)
Actually, could you call this
GetWindowDimensions

>+  PRInt32 parentType;
>+  parentAsItem->GetItemType(&parentType);
>+
>+  if (parentType != nsIDocShellTreeItem::typeChrome) {
>+    return NS_ERROR_FAILURE;
>+  }
>+
>+  if (!mOwnerContent->IsXUL()) {
>+    return NS_ERROR_FAILURE;
>+  }
No need for these checks.


>+++ b/dom/ipc/PBrowser.ipdl
>@@ -237,11 +237,11 @@ child:
>      */
>     Show(nsIntSize size);
> 
>     LoadURL(nsCString uri);
> 
>-    Move(nsIntSize size);
>+    UpdateOuterDimensionsAndResize(nsIntSize size, nsRect rect);
Perhaps UpdateDimensions(nsRect aWindowRect, nsIntSize aTabSize)

Comment 26

[Oleg Romashin (:romaxa)]

Attachment: Fixed nits, version to push

Comment 27

[Oleg Romashin (:romaxa)]

http://hg.mozilla.org/mozilla-central/rev/31cbb80c705e

Comment 28

[Oleg Romashin (:romaxa)]

backed out due to Windows red build

Comment 29

[Oleg Romashin (:romaxa)]

nsresult and NS_METHODIMP seems are different on windows

Comment 30

[Oleg Romashin (:romaxa)]

Tested on try, seems looks good
http://hg.mozilla.org/mozilla-central/rev/5c4fe292a240

Comment 31

[Oleg Romashin (:romaxa)]

Can we push it into some beta or release?

Comment 32

[Olli Pettay [:smaug][[e-mail]]]

Oops, NS_ASSERTION(mIsTopLevelContent, "Outer dimensions must be taken only from TopLevel content"); is wrong after all.
mIsTopLevelContent is set only in non-remote frameloaders.

Could you please just remove that assertion.

Comment 33

[Oleg Romashin (:romaxa)]

Attachment: Remove assert

Comment 34

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 547255 [details] [diff] [review]
Remove assert

Thank you. My mistake.