Closed Bug 616511 Opened 9 years ago Closed 9 years ago

browser_sessionstore.js is failing and browser_select.js leave the content navigator bar opened

Categories

(Firefox for Android Graveyard :: General, defect)

x86_64
Linux
defect
Not set

Tracking

(fennec2.0b3+)

RESOLVED FIXED
Tracking Status
fennec 2.0b3+ ---

People

(Reporter: vingtetun, Assigned: vingtetun)

Details

Attachments

(1 file, 1 obsolete file)

Attached patch Patch (obsolete) — Splinter Review
The patch fix both tests issue.
Attachment #495031 - Flags: review?(mark.finkle)
Comment on attachment 495031 [details] [diff] [review]
Patch

>diff --git a/chrome/tests/browser_sessionstore.js b/chrome/tests/browser_sessionstore.js

>   run: function() {
>+    gCurrentTest.loadSecondTab();
>+    /*    
>+
>     Browser.addTab("about:blank", true);
>+    messageManager.addMessageListener("pageshow", function(aMessage) {
>+      messageManager.removeMessageListener(aMessage.name, arguments.callee);
>+      setTimeout(function() { gCurrentTest.loadSecondTab(); }, 0);
>+    });
>+    */

Just remove the commented code

>     // Need to wait until the page is loaded
>     messageManager.addMessageListener("pageshow",
>     function(aMessage) {
>       if (gCurrentTest._currentTab.browser.currentURI.spec != "about:blank") {
>         messageManager.removeMessageListener(aMessage.name, arguments.callee);
>-        gCurrentTest.onPageReady();
>+        setTimeout(function() { gCurrentTest.onPageReady(); }, 400);

Use a comment to explain why you are using a 400ms timeout here. Will 400ms be enough for slow devices? Is this because the session store needs to do something first?


>diff --git a/components/SessionStore.js b/components/SessionStore.js

>   handleEvent: function ss_handleEvent(aEvent) {
>     let window = aEvent.currentTarget.ownerDocument.defaultView;
>     switch (aEvent.type) {
>       case "load":
>-      case "pageshow":
>         this.onTabLoad(window, aEvent.currentTarget, aEvent);
>         break;

Just remove this entire case. The "load" event is never even hooked up. It will never work. "pageshow" message is the only thing we use.

r+ but let me know why 400ms
Attachment #495031 - Flags: review?(mark.finkle) → review+
Attached patch Patch v0.2Splinter Review
This patch:
 * remove the 400ms timeout and use a waitFor
 * make 2 todo works

I've not removed the "load" case into SessionStore.js because I think it is added here: http://mxr.mozilla.org/mobile-browser/source/components/SessionStore.js#121
Attachment #495818 - Flags: review?(mark.finkle)
Comment on attachment 495818 [details] [diff] [review]
Patch v0.2

Looks good, but you really should remove the "load" from "handleEvent". The code here:

http://mxr.mozilla.org/mobile-browser/source/components/SessionStore.js#121

This code calls addEventListener and passes an anonymous function, not "this", so handleEvent will not be called.
Attachment #495818 - Flags: review?(mark.finkle) → review+
Attachment #495031 - Attachment is obsolete: true
(In reply to comment #3)

> This code calls addEventListener and passes an anonymous function, not "this",
> so handleEvent will not be called.

I feel stupid now :)
tracking-fennec: --- → 2.0b3+
Status: NEW → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
bugspam
Assignee: nobody → 21
You need to log in before you can comment on or make changes to this bug.