Closed Bug 715388 Opened 8 years ago Closed 8 years ago

Don't show telemetry doorhanger for session restore

Categories

(Firefox for Android :: General, defect, P2)

All
Android
defect

Tracking

()

VERIFIED FIXED
Firefox 12
Tracking Status
firefox11 --- verified
firefox12 --- verified
fennec 11+ ---

People

(Reporter: bnicholson, Assigned: bnicholson)

Details

Attachments

(1 file, 1 obsolete file)

If doing a session restore, the UIReady and Gecko:Ready events are fired before any tabs are created. This breaks any code that assumes at least one tab exists (such as doorhangers).
Attached patch patch (obsolete) — Splinter Review
Attachment #585942 - Flags: review?(mark.finkle)
Attachment #585942 - Flags: approval-mozilla-aurora?
Comment on attachment 585942 [details] [diff] [review]
patch


>diff --git a/mobile/android/chrome/content/browser.js b/mobile/android/chrome/content/browser.js

>     // restore the previous session
>     let ss = Cc["@mozilla.org/browser/sessionstore;1"].getService(Ci.nsISessionStore);
>     if (ss.shouldRestore()) {
>       // A restored tab should not be active if we are loading a URL
>       let restoreToFront = false;

Flip this now so we can remove the | else | block below:

        // Let the session make a restored tab active
        let restoreToFront = true;

>+      let isCmdUrl = url && url != "about:home";

Use some parens and rename:

        let isExternalUrl = (url && url != "about:home");

>+      if (isCmdUrl) {

Add the boolean back here:

          // A restored tab should not be active if we are loading a URL
          restoreToFront = false;

>+        BrowserApp.startupFinished();

I don't like needing to call this from 3 places. I don't like the name much either. I need to think about this.

>       } else {
>         // Let the session make a restored tab active
>         restoreToFront = true;

Remove the | else | block now. We don't need it
Comment on attachment 585942 [details] [diff] [review]
patch

One way to clean this up a little is to do one of two things we did in XUL Fennec:
1. Make a dummy tab while we were restoring (not my favorite)
2. Use a one-shot "pageshow" event to call "startupDelayed" method similar to your "startupFinished" method. This has the benefit of being called regardless of the code path. We could use "TabOpen" instead of "pageshow" if we want a quicker notification.

Yeah, I like approach #2 best. Let's try it.
Attachment #585942 - Flags: approval-mozilla-aurora?
Comment on attachment 585942 [details] [diff] [review]
patch

needs new patch
Attachment #585942 - Flags: review?(mark.finkle) → review-
Priority: -- → P2
Summary: Ready events fired before tab exists during session restore → Don't show telemetry doorhanger for session restore
Attached patch patch v2Splinter Review
It looks like the telemetry door hanger might actually be the only thing affected here. I changed the patch to simply show the doorhanger only if we aren't doing a session restore. Now, we don't delay any startup messages during a restore (blassey's suggestion), and it also makes more sense IMO to not show this door hanger for a restore (as it makes the restore more seamless).
Attachment #585942 - Attachment is obsolete: true
Attachment #586218 - Flags: review?(mark.finkle)
Comment on attachment 586218 [details] [diff] [review]
patch v2

Can we move the telemtry code into a BrowserApp._showTelemetryPrompt method and do a | this._showTelemetryPrompt | in BrowserApp.startup ?
Attachment #586218 - Flags: review?(mark.finkle) → review+
tracking-fennec: --- → 11+
One of three changesets backed out of inbound due to test coalescing making it hard to identify which caused the native android test failures:
https://hg.mozilla.org/integration/mozilla-inbound/rev/1927c7905f5e
https://hg.mozilla.org/mozilla-central/rev/1da3bc5e4131
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 12
Comment on attachment 586218 [details] [diff] [review]
patch v2

[Approval Request Comment]
Bug fix for new tab race condition. Low risk.
Attachment #586218 - Flags: approval-mozilla-aurora?
Attachment #586218 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.