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

RESOLVED FIXED

Status

()

Core
IPC
--
major
RESOLVED FIXED
6 years ago
6 years ago

People

(Reporter: Mikhail Sobolev, Assigned: romaxa)

Tracking

Trunk
Other
Linux
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

(URL)

Attachments

(6 attachments, 4 obsolete attachments)

(Reporter)

Description

6 years ago
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.
(Reporter)

Comment 1

6 years ago
Created attachment 524392 [details]
Screenshot of the actual result

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

Comment 2

6 years ago
Created attachment 524393 [details]
Screenshot of the expected behaviour

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

Comment 3

6 years ago
Reproducible also on desktop fennec build, might be a recent regression, would be nice to check earlier builds and find regression range
Status: UNCONFIRMED → NEW
Ever confirmed: true
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.
Created attachment 524410 [details]
testcase
This is basically the same as bug 614650.
I don't think window.outerWidth should trigger an error, fwiw.
###!!! 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]
Component: General → IPC
Product: Fennec → Core
QA Contact: general → ipc
Summary: image based navigational bar on the site is completely unusable → image based navigational bar is useless in remote Fennec
Other window properties that trigger this error that are probably related to this:
outerHeight
screenX
screenY

Updated

6 years ago
Summary: image based navigational bar is useless in remote Fennec → image based navigational bar is useless in remote Fennec (window.outerWidth throws js errors)
(Assignee)

Comment 9

6 years ago
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
Attachment #524871 - Flags: review?(Olli.Pettay)
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?

Updated

6 years ago
Duplicate of this bug: 614650

Updated

6 years ago
Attachment #524871 - Flags: review?(Olli.Pettay)
(Assignee)

Comment 12

6 years ago
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?
Attachment #524871 - Attachment is obsolete: true
Attachment #526891 - Flags: feedback?(Olli.Pettay)
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?
Attachment #526891 - Flags: feedback?(Olli.Pettay) → feedback-
(Assignee)

Comment 14

6 years ago
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.
(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...
Duplicate of this bug: 671267
Duplicate of this bug: 671446
(Assignee)

Comment 18

6 years ago
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?
(Assignee)

Comment 19

6 years ago
Also tested on fennec with remote.tabs disabled and also see window.outerWidth=968
(Assignee)

Comment 20

6 years ago
Created attachment 546006 [details] [diff] [review]
Is it better direction?
Attachment #526891 - Attachment is obsolete: true
Attachment #546006 - Flags: review?(Olli.Pettay)
(Assignee)

Comment 21

6 years ago
Created attachment 546019 [details] [diff] [review]
Outer dimensions impl v3

Also added screenX/Y implementation
Assignee: nobody → romaxa
Attachment #546006 - Attachment is obsolete: true
Status: NEW → ASSIGNED
Attachment #546006 - Flags: review?(Olli.Pettay)
Attachment #546019 - Flags: review?(Olli.Pettay)
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 on attachment 546019 [details] [diff] [review]
Outer dimensions impl v3

Couldn't you merge Move and UpdateDimensions?
(Assignee)

Comment 24

6 years ago
Created attachment 546047 [details] [diff] [review]
Outer dimensions impl v4
Attachment #546019 - Attachment is obsolete: true
Attachment #546019 - Flags: review?(Olli.Pettay)
Attachment #546047 - Flags: review?(Olli.Pettay)
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)
Attachment #546047 - Flags: review?(Olli.Pettay) → review+
(Assignee)

Comment 26

6 years ago
Created attachment 546215 [details] [diff] [review]
Fixed nits, version to push
(Assignee)

Updated

6 years ago
Keywords: checkin-needed
(Assignee)

Comment 27

6 years ago
http://hg.mozilla.org/mozilla-central/rev/31cbb80c705e
Status: ASSIGNED → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
(Assignee)

Updated

6 years ago
Keywords: checkin-needed
(Assignee)

Comment 28

6 years ago
backed out due to Windows red build
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
(Assignee)

Comment 29

6 years ago
nsresult and NS_METHODIMP seems are different on windows
(Assignee)

Comment 30

6 years ago
Tested on try, seems looks good
http://hg.mozilla.org/mozilla-central/rev/5c4fe292a240
Status: REOPENED → RESOLVED
Last Resolved: 6 years ago6 years ago
Resolution: --- → FIXED
(Assignee)

Comment 31

6 years ago
Can we push it into some beta or release?
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.
(Assignee)

Comment 33

6 years ago
Created attachment 547255 [details] [diff] [review]
Remove assert
Attachment #547255 - Flags: review?(Olli.Pettay)
Comment on attachment 547255 [details] [diff] [review]
Remove assert

Thank you. My mistake.
Attachment #547255 - Flags: review?(Olli.Pettay) → review+
You need to log in before you can comment on or make changes to this bug.