Last Comment Bug 648250 - image based navigational bar is useless in remote Fennec (window.outerWidth throws js errors)
: image based navigational bar is useless in remote Fennec (window.outerWidth t...
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: IPC (show other bugs)
: Trunk
: Other Linux
: -- major (vote)
: ---
Assigned To: Oleg Romashin (:romaxa)
:
:
Mentors:
http://www.finnkino.fi/eng/
: 614650 671267 671446 (view as bug list)
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2011-04-07 06:28 PDT by Mikhail Sobolev
Modified: 2011-07-21 02:00 PDT (History)
9 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Screenshot of the actual result (359.11 KB, image/png)
2011-04-07 06:30 PDT, Mikhail Sobolev
no flags Details
Screenshot of the expected behaviour (266.38 KB, image/png)
2011-04-07 06:31 PDT, Mikhail Sobolev
no flags Details
testcase (431 bytes, text/html)
2011-04-07 09:12 PDT, Martijn Wargers [:mwargers] (not working for Mozilla)
no flags Details
Fix for this testcase (970 bytes, patch)
2011-04-09 11:32 PDT, Oleg Romashin (:romaxa)
no flags Details | Diff | Splinter Review
This should be better (5.14 KB, patch)
2011-04-18 19:07 PDT, Oleg Romashin (:romaxa)
bugs: feedback-
Details | Diff | Splinter Review
Is it better direction? (7.41 KB, patch)
2011-07-14 14:35 PDT, Oleg Romashin (:romaxa)
no flags Details | Diff | Splinter Review
Outer dimensions impl v3 (7.26 KB, patch)
2011-07-14 15:36 PDT, Oleg Romashin (:romaxa)
no flags Details | Diff | Splinter Review
Outer dimensions impl v4 (7.67 KB, patch)
2011-07-14 17:12 PDT, Oleg Romashin (:romaxa)
bugs: review+
Details | Diff | Splinter Review
Fixed nits, version to push (7.34 KB, patch)
2011-07-15 13:31 PDT, Oleg Romashin (:romaxa)
no flags Details | Diff | Splinter Review
Remove assert (794 bytes, patch)
2011-07-20 14:46 PDT, Oleg Romashin (:romaxa)
bugs: review+
Details | Diff | Splinter Review

Description Mikhail Sobolev 2011-04-07 06:28:03 PDT
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 2011-04-07 06:30:08 PDT
Created attachment 524392 [details]
Screenshot of the actual result

The screenshot is produced using Firefox for mobile on Mac OS X
Comment 2 Mikhail Sobolev 2011-04-07 06:31:15 PDT
Created attachment 524393 [details]
Screenshot of the expected behaviour

The screenshot is produced using Firefox 4 on Mac OS X (with flash blocker)
Comment 3 Oleg Romashin (:romaxa) 2011-04-07 07:23:11 PDT
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 [:mwargers] (not working for Mozilla) 2011-04-07 08:47:43 PDT
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 [:mwargers] (not working for Mozilla) 2011-04-07 09:12:02 PDT
Created attachment 524410 [details]
testcase
Comment 6 Martijn Wargers [:mwargers] (not working for Mozilla) 2011-04-07 09:16:07 PDT
This is basically the same as bug 614650.
I don't think window.outerWidth should trigger an error, fwiw.
Comment 7 Tatiana Meshkova (:tatiana) 2011-04-08 04:44:29 PDT
###!!! 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 [:mwargers] (not working for Mozilla) 2011-04-09 09:50:58 PDT
Other window properties that trigger this error that are probably related to this:
outerHeight
screenX
screenY
Comment 9 Oleg Romashin (:romaxa) 2011-04-09 11:32:16 PDT
Created attachment 524871 [details] [diff] [review]
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] (reviewing overload) 2011-04-09 12:05:58 PDT
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 11 Olli Pettay [:smaug] (reviewing overload) 2011-04-09 12:44:40 PDT
*** Bug 614650 has been marked as a duplicate of this bug. ***
Comment 12 Oleg Romashin (:romaxa) 2011-04-18 19:07:00 PDT
Created attachment 526891 [details] [diff] [review]
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] (reviewing overload) 2011-04-19 04:26:05 PDT
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) 2011-04-19 10:34:42 PDT
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] (reviewing overload) 2011-04-19 11:35:25 PDT
(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 16 Mark Finkle (:mfinkle) (use needinfo?) 2011-07-13 11:23:14 PDT
*** Bug 671267 has been marked as a duplicate of this bug. ***
Comment 17 Mark Finkle (:mfinkle) (use needinfo?) 2011-07-13 17:13:23 PDT
*** Bug 671446 has been marked as a duplicate of this bug. ***
Comment 18 Oleg Romashin (:romaxa) 2011-07-14 13:19:50 PDT
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) 2011-07-14 13:34:01 PDT
Also tested on fennec with remote.tabs disabled and also see window.outerWidth=968
Comment 20 Oleg Romashin (:romaxa) 2011-07-14 14:35:09 PDT
Created attachment 546006 [details] [diff] [review]
Is it better direction?
Comment 21 Oleg Romashin (:romaxa) 2011-07-14 15:36:06 PDT
Created attachment 546019 [details] [diff] [review]
Outer dimensions impl v3

Also added screenX/Y implementation
Comment 22 Olli Pettay [:smaug] (reviewing overload) 2011-07-14 15:48:33 PDT
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] (reviewing overload) 2011-07-14 15:49:49 PDT
Comment on attachment 546019 [details] [diff] [review]
Outer dimensions impl v3

Couldn't you merge Move and UpdateDimensions?
Comment 24 Oleg Romashin (:romaxa) 2011-07-14 17:12:11 PDT
Created attachment 546047 [details] [diff] [review]
Outer dimensions impl v4
Comment 25 Olli Pettay [:smaug] (reviewing overload) 2011-07-15 03:05:51 PDT
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) 2011-07-15 13:31:01 PDT
Created attachment 546215 [details] [diff] [review]
Fixed nits, version to push
Comment 27 Oleg Romashin (:romaxa) 2011-07-15 14:15:58 PDT
http://hg.mozilla.org/mozilla-central/rev/31cbb80c705e
Comment 28 Oleg Romashin (:romaxa) 2011-07-15 14:59:16 PDT
backed out due to Windows red build
Comment 29 Oleg Romashin (:romaxa) 2011-07-15 15:01:20 PDT
nsresult and NS_METHODIMP seems are different on windows
Comment 30 Oleg Romashin (:romaxa) 2011-07-18 10:42:21 PDT
Tested on try, seems looks good
http://hg.mozilla.org/mozilla-central/rev/5c4fe292a240
Comment 31 Oleg Romashin (:romaxa) 2011-07-19 21:11:40 PDT
Can we push it into some beta or release?
Comment 32 Olli Pettay [:smaug] (reviewing overload) 2011-07-20 14:04:01 PDT
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) 2011-07-20 14:46:11 PDT
Created attachment 547255 [details] [diff] [review]
Remove assert
Comment 34 Olli Pettay [:smaug] (reviewing overload) 2011-07-21 02:00:41 PDT
Comment on attachment 547255 [details] [diff] [review]
Remove assert

Thank you. My mistake.

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