590956 - Random failure into browser_viewport.js

Status: RESOLVED WORKSFORME

(Firefox for Android Graveyard :: General, defect)

Description

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

TEST-UNEXPECTED-FAIL | chrome://mochikit/content/browser/mobile/chrome/browser_viewport.js | Viewport scale=4 [got 1, expected 4] - Got false, expected true

Comment 1

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

Attachment: Patch

I've been obliged to use sendSyncMessage into content.js and to switch for addEventListener("pageshow", this, true); into bindings/browser.js to avoid the random failure.

I've run the tests 10 times without seeing a failure with this patch (before it was failing at the first, second, or third try)

Comment 2

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

Some changes (working_tab -> currentTab, waitFor -> addMessageListener) are mostly (except the Browser:ViewportMetadata) to be similar to what is in the other tests

Comment 3

[Mark Finkle (:mfinkle) (use needinfo?)]

Comment on attachment 469632 [details] [diff] [review]
Patch

>diff -r 0b158275cb7e chrome/content/bindings/browser.js
>--- a/chrome/content/bindings/browser.js	Thu Aug 26 07:14:23 2010 -0700
>+++ b/chrome/content/bindings/browser.js	Fri Aug 27 00:01:59 2010 +0200
>@@ -250,27 +250,24 @@ let DOMEvents =  {
>   init: function() {
>     addEventListener("DOMContentLoaded", this, false);
>     addEventListener("DOMTitleChanged", this, false);
>     addEventListener("DOMLinkAdded", this, false);
>     addEventListener("DOMWillOpenModalDialog", this, false);
>     addEventListener("DOMModalDialogClosed", this, true);

I wonder why this is | true |

>     addEventListener("DOMWindowClose", this, false);
>     addEventListener("DOMPopupBlocked", this, false);
>-    addEventListener("pageshow", this, false);
>+    addEventListener("pageshow", this, true);

If we need to do this we should have a comment

>       case "DOMContentLoaded":
>-        if (document.documentURIObject.spec == "about:blank")
>-          return;
>-
>         sendAsyncMessage("DOMContentLoaded", { });
>         break;

This scares me. It will case a Ts regression like the one I just backed out.

>   resetMetadata: function resetMetadata() {
>     this.metadata = null;
>-    sendAsyncMessage("Browser:ViewportMetadata", {});
>+    sendSyncMessage("Browser:ViewportMetadata", {});
>   },
> 
>   updateMetadata: function updateMetadata() {
>     this.metadata = this.getViewportMetadata();
>-    sendAsyncMessage("Browser:ViewportMetadata", this.metadata);
>+    sendSyncMessage("Browser:ViewportMetadata", this.metadata);
>   },

This might not be too bad, but I don't like doing this just for tests.

Comment 4

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

Comment on attachment 469632 [details] [diff] [review]
Patch

Matt's is fixing the tests for another reason. I guess I can remove r? and just retry later to see if I can fix it at WFM

Comment 5

[Matt Brubeck (:mbrubeck)]

This no longer fails intermittently for me, but you can reopen if you're still seeing this.