Currently, if the first thing you do after you install Nightly is investigate the search activity, the first-run will be skipped when you launch the browser either via result or just launching Nightly from your application listing. Not sure if intentional. -- LG Nexus 5 (4.4.4) Nightly (08/28)

I believe this is intentional. The UX agreed on a key principle of the first run is "Don't disrupt my flow". If the user is taken to the browser via a search or a external link, we should respect their intents other than standing in their way with a first-run screen.

External links don't bypass the first-run E.g, on a new profile # adb shell am start -a android.intent.action.VIEW -n org.mozilla.fennec/.App -d "http://polygon.com" Will open the first-run as well.

The way the code currently works is that we launch the firstrun screen when we create a new profile. If the search activity creates a Firefox profile if one doesn't exist, that would explain this. I think we could make this so that we launch the firstrun screen the first time users open Firefox, not from an external intent. How does that sound, Yuan? Maybe there are other interactions that I'm not thinking of at the moment.

(In reply to Chenxia Liu [:liuche] from comment #3) > The way the code currently works is that we launch the firstrun screen when > we create a new profile. If the search activity creates a Firefox profile if > one doesn't exist, that would explain this. > > I think we could make this so that we launch the firstrun screen the first > time users open Firefox, not from an external intent. How does that sound, > Yuan? Maybe there are other interactions that I'm not thinking of at the > moment. Thanks for the explanation Chenxia. What you have suggested sounds right to me. External intent is the main use case that first-run should not interrupt.

(In reply to Yuan Wang(:Yuan) – Mobile Firefox Design Lead from comment #4) > Thanks for the explanation Chenxia. What you have suggested sounds right to > me. External intent is the main use case that first-run should not interrupt. Darn. I thought we had the External intent case covered. That seems more important to me than the Search Activity stealing first-run. Two different bugs at least.

Yuan, take a look at the build below, and try out some scenarios. I tested opening a link from GMail, as well as from the search activity (if you have multiple ones installed, it should be one of the Fennec Search activities, I believe), all on a clean Firefox. http://people.mozilla.org/~liuche/bug-1059792/external-intent-1.apk I'm wondering what the onboarding/first run panel behavior should be in the case where the user opens Fennec for the first time through a GMail link but then continues browsing (visiting links, opening tabs, etc). Should we show the onboarding screen next time they launch Fennec from scratch, even though they may have used Fennec fairly extensively? This is a bit of an edge case though.

(In reply to Chenxia Liu [:liuche] from comment #6) > Created attachment 8483899 [details] [diff] [review] > Patch: Launch first run only on non-external intent > > Yuan, take a look at the build below, and try out some scenarios. I tested > opening a link from GMail, as well as from the search activity (if you have > multiple ones installed, it should be one of the Fennec Search activities, I > believe), all on a clean Firefox. > > http://people.mozilla.org/~liuche/bug-1059792/external-intent-1.apk > > I'm wondering what the onboarding/first run panel behavior should be in the > case where the user opens Fennec for the first time through a GMail link but > then continues browsing (visiting links, opening tabs, etc). Should we show > the onboarding screen next time they launch Fennec from scratch, even though > they may have used Fennec fairly extensively? This is a bit of an edge case > though. I just tried the apk. It seems like the browser was showing the onboarding screen next time the users launch it from scratch. Since one big reason of having this screen is to add buffering time for the top sites to load. Do we know if this issue still exist the next time the user launcher the browser from scratch, even though they have opened it from an external intent before? If loading top sites is still an issue, having the screen as the buffer sounds reasonable. If the issue doesn't exist any more, I would be okay to not show the screen, especially given the fact that this is a minimal version. There is not a lot for the users to take away from. As there will be more useful information for the full "Get Started", we will have a more compelling reason to show the tour next time the users launch the browser.

(In reply to Yuan Wang(:Yuan) – Mobile Firefox Design Lead from comment #7) > Since one big reason of having this screen is to add buffering time for the > top sites to load. Do we know if this issue still exist the next time the > user launcher the browser from scratch, even though they have opened it from > an external intent before? It should not. Distribution processing will occur regardless of whether they load about:home or a page from an intent, so the second entry into the browser should be fast.

(In reply to Richard Newman [:rnewman] from comment #8) > (In reply to Yuan Wang(:Yuan) – Mobile Firefox Design Lead from comment #7) > > > Since one big reason of having this screen is to add buffering time for the > > top sites to load. Do we know if this issue still exist the next time the > > user launcher the browser from scratch, even though they have opened it from > > an external intent before? > > It should not. Distribution processing will occur regardless of whether they > load about:home or a page from an intent, so the second entry into the > browser should be fast. Thanks for the clarification. So I would recommend not to show the screen for this minimal version. And for the full "Get Started" tour, show the screens next time the user launch from scratch.

Thanks Yuan! Now we only show the first run pane on the first time Firefox is launched if we are not opening a link from another app.

Comment on attachment 8489654 [details] [diff] [review] Patch: Launch first run only on non-external intent Review of attachment 8489654 [details] [diff] [review]: ----------------------------------------------------------------- ::: mobile/android/base/BrowserApp.java @@ +668,5 @@ > + */ > + private void checkStartPane(Context context, String intentAction) { > + final SharedPreferences prefs = GeckoSharedPrefs.forProfile(this); > + > + if (prefs.contains(PREF_FIRSTRUN_STARTPANE)) { Since this is going to run on the main thread, we should set the StrictMode thread policy like we do for the similar "should we show about:feedback?" pref: http://mxr.mozilla.org/mozilla-central/source/mobile/android/base/BrowserApp.java#2930 Reading the comments from the bug that added that, it actually sounds like we should be using SharedPreferencesHelper: https://bugzilla.mozilla.org/show_bug.cgi?id=1061430#c11 @@ +670,5 @@ > + final SharedPreferences prefs = GeckoSharedPrefs.forProfile(this); > + > + if (prefs.contains(PREF_FIRSTRUN_STARTPANE)) { > + return; > + } else { Nit: else isn't needed since the if statement returns. @@ +672,5 @@ > + if (prefs.contains(PREF_FIRSTRUN_STARTPANE)) { > + return; > + } else { > + if (!Intent.ACTION_VIEW.equals(intentAction)) { > + final Intent startIntent = new Intent(context, StartPane.class); Instead of using a Context parameter, we could just pass `this` here directly. @@ +678,2 @@ > } > + prefs.edit().putBoolean(PREF_FIRSTRUN_STARTPANE, true).apply(); Shouldn't we only set this if we actually start the start pane?

(In reply to :Margaret Leibovic from comment #11) > Comment on attachment 8489654 [details] [diff] [review] > Patch: Launch first run only on non-external intent > > Review of attachment 8489654 [details] [diff] [review]: > ----------------------------------------------------------------- > > ::: mobile/android/base/BrowserApp.java > @@ +668,5 @@ > > + */ > > + private void checkStartPane(Context context, String intentAction) { > > + final SharedPreferences prefs = GeckoSharedPrefs.forProfile(this); > > + > > + if (prefs.contains(PREF_FIRSTRUN_STARTPANE)) { > > Since this is going to run on the main thread, we should set the StrictMode > thread policy like we do for the similar "should we show about:feedback?" > pref: > http://mxr.mozilla.org/mozilla-central/source/mobile/android/base/BrowserApp. > java#2930 Good call, done. > > Reading the comments from the bug that added that, it actually sounds like > we should be using SharedPreferencesHelper: > https://bugzilla.mozilla.org/show_bug.cgi?id=1061430#c11 I looked at the SharedPreferencesHelper, and it uses JSON messages and EventDispatchers, and since this is the only place that we access this SharedPreference and it's all on the same thread, I'd rather not add all that overhead (listeners, JSON object making). > @@ +678,2 @@ > > } > > + prefs.edit().putBoolean(PREF_FIRSTRUN_STARTPANE, true).apply(); > > Shouldn't we only set this if we actually start the start pane? Added a comment - since this is the minimal first run, yuan thinks it's okay to not show it if the user opens Fennec to launch an external link.

(In reply to Chenxia Liu [:liuche] from comment #12) > > > > Reading the comments from the bug that added that, it actually sounds like > > we should be using SharedPreferencesHelper: > > https://bugzilla.mozilla.org/show_bug.cgi?id=1061430#c11 > > I looked at the SharedPreferencesHelper, and it uses JSON messages and > EventDispatchers, and since this is the only place that we access this > SharedPreference and it's all on the same thread, I'd rather not add all > that overhead (listeners, JSON object making). Sounds good. Thanks for looking into it, I just wanted to make sure we weren't propagating a problem :) > > @@ +678,2 @@ > > > } > > > + prefs.edit().putBoolean(PREF_FIRSTRUN_STARTPANE, true).apply(); > > > > Shouldn't we only set this if we actually start the start pane? > Added a comment - since this is the minimal first run, yuan thinks it's okay > to not show it if the user opens Fennec to launch an external link. Ah, that makes sense. A comment is a good idea, though.

Comment on attachment 8491120 [details] [diff] [review] Patch: Launch first run only on non-external intent Approval Request Comment [Feature/regressing bug #]: Initial implementation of first run behavior with external intents was incorrect [User impact if declined]: Users will see first run pane when opening FF from an external app (if first run) [Describe test coverage new/current, TBPL]: local [Risks and why]: low, remove listener path [String/UUID change made/needed]: none

Backed out for blowing up most of Android on fx-team. Please verify that your tests pass on Try before pushing to production trees. https://hg.mozilla.org/integration/fx-team/rev/2119d0833157 https://tbpl.mozilla.org/?tree=Fx-Team&jobname=Android&rev=297008b7e1f6

Comment on attachment 8491120 [details] [diff] [review] Patch: Launch first run only on non-external intent Clearing aurora uplift request.

Picture from one of the test failures. Looks like the lightweight screen is active.

Gah, I didn't try this patch out on 2.3, and it looks like the entry point for launching the onboarding screen isn't correct and launches the onboarding screen after showing parts of the main UI. I'll find a better place to check for launching, and put up another patch.

The problem is that the onboarding screen shows up on the tests and causes failures because it obscures the main UI. rnewman suggested hiding the onboarding screen if we get a request to load a url, but for robocop tests, we set the url by focusing on the url bar - which is not visible. This causes a bunch of other permafails on robocop...Alternatively, I could add a method into BaseTest for "startBrowsing()" to click through the first run screen, but I don't like that because it also adds more time to every test, when our tests aren't meant to simulate "first run." It seems like the simplest way to do this is to mirror the pref from Gecko, and pref it off for most of the tests. It's quick and straightforward - but does involve mixing an Android-only pref into Gecko. Thoughts?

Why not have BaseTest#setUp specify about:home as the URL to load? => Loading an external URL => not showing the onboarding screen, which fixes reftests, and should also fix Robocop, yet still loading about:home.

I wouldn't say that "load an external url" is always the same as "launch Fennec from launcher icon" because I've seen some specific handling of ACTION_VIEW vs ACTION_MAIN - but I guess that if we can consider them to be similar enough cases, that could work.

Can we just configure the default profile we use for tests to not show the first run experience? I know that we can configure default gecko preferences in the profile we use for testing. Is it possible to also set the shared pref that says we already showed the first run experience?

(In reply to Chenxia Liu [:liuche] from comment #20) > It seems like the simplest way to do this is to mirror the pref from Gecko, > and pref it off for most of the tests. It's quick and straightforward - but > does involve mixing an Android-only pref into Gecko. Thoughts? Reading the comments more closely, I see you mentioned my idea here :) If the pref is just for automated tests to work, I say go for it.

Try out both (any) ideas to see if any of them even work. Then we can pick the one that is the best fit.

A different approach that still only uses Android pref but handles the pref/migration problem (of not showing the onboarding screen to users that have already seen it!) that the v1 of this patch didn't address. This should address the 2.3 test problems too, let's see: https://tbpl.mozilla.org/?tree=Try&rev=70451a03c4b3

I'm getting test failures on rc1 for 4.0 tablets, and I can't seem to repro on any devices. Looking at this screenshot, the tablet urlbar seems to be loading or something - do you happen to have any ideas when this might happen? Any guesses on what might be happening so I can try to repro this locally would be helpful!

Oops, didn't include the try run: https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=b4e263358975

(In reply to Chenxia Liu [:liuche] from comment #27) > Created attachment 8497209 [details] > Screenshot: tablet failure > > I'm getting test failures on rc1 for 4.0 tablets, and I can't seem to repro > on any devices. Looking at this screenshot, the tablet urlbar seems to be > loading or something - do you happen to have any ideas when this might > happen? Any guesses on what might be happening so I can try to repro this > locally would be helpful! Seems like a graphics issue: the curve is drawn with a fancy offscreen buffer technique. What patch is causing this issue?

I think the screenshot is unrelated to the issue at hand. This is a strange problem. It makes me ask "How can this not be causing problems right now, with the existing code?" Doesn't the Start Pane appear right now? The patch seems to be trying to stop the Start Pane when an external Intent is passed into Fennec. Aren't there simpler ways to check for a URI from the Intent?

Mid-aired with mfinkle... (In reply to Lucas Rocha (:lucasr) from comment #29) > (In reply to Chenxia Liu [:liuche] from comment #27) > > Created attachment 8497209 [details] > > Screenshot: tablet failure > > > > I'm getting test failures on rc1 for 4.0 tablets, and I can't seem to repro > > on any devices. Looking at this screenshot, the tablet urlbar seems to be > > loading or something - do you happen to have any ideas when this might > > happen? Any guesses on what might be happening so I can try to repro this > > locally would be helpful! > > Seems like a graphics issue: the curve is drawn with a fancy offscreen > buffer technique. What patch is causing this issue? The patch in this bug is the one causing this issue, which seems odd to me... liuche, have you tried moving thing around in the patch to see if we can narrow down what might be causing problems? I'm suspicious that this new checkStartPane method may be called later than the BroadcastReciever was called, creating some timing problems. Maybe we should try keeping the LocalBroadcastManager logic, but then just check the intent in launchStartPane. Did you try that previously?

Ok, I've been digging through this bug and I think I know what's wrong. The StartPane is actually being shown in all the robocop tests in the tree, but since the first test is testGeckoProfile, which doesn't do UI tests, the tests haven't been failing. However, with the new SharedPreferences usage, the StartPane is being delayed until the first test after that and is failing because there are UI tests. I've tried a few things (clearing the preferences after the GeckoProfile test, moving the first run check call around) and that's cleared things up for 2.3 at least. I'm still looking at why that doesn't fix things for 4.0, but I'm waiting on some try runs (because I just can't repro this locally).

Now with clearing SharedPreferences after running testGeckoProfile, which fixes the testing problem with showing StartPane because now none of the tests set the flag to show the StartPane. The handling of when to show the StartPane is also cleaner now. rc1 try run was green, here's the full try run: https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=8a63e27fb8ef

(In reply to Mark Finkle (:mfinkle) from comment #30) > I think the screenshot is unrelated to the issue at hand. > > This is a strange problem. It makes me ask "How can this not be causing > problems right now, with the existing code?" Doesn't the Start Pane appear > right now? Yep, apparently, it does, but only on testGeckoProfile, which creates a new profile. > > The patch seems to be trying to stop the Start Pane when an external Intent > is passed into Fennec. Aren't there simpler ways to check for a URI from the > Intent? To specifically *just* fix this problem, we could just check for the URI in the broadcast and just not launch there, but in the future, if we don't launch on the first external intent, we will want to launch the Onboarding UI at a later point, and we can't rely on the broadcast anymore because it only fires when the profile is created.

(In reply to Chenxia Liu [:liuche] from comment #32) > Ok, I've been digging through this bug and I think I know what's wrong. > > The StartPane is actually being shown in all the robocop tests in the tree, > but since the first test is testGeckoProfile, which doesn't do UI tests, the > tests haven't been failing. However, with the new SharedPreferences usage, > the StartPane is being delayed until the first test after that and is > failing because there are UI tests. I'm a bit confused by this. What type of intent launches the tests? Shouldn't the intent action check catch this? (In reply to Chenxia Liu [:liuche] from comment #33) > Created attachment 8501441 [details] [diff] [review] > Patch: Launch first run only on non-external intent v3 > > Now with clearing SharedPreferences after running testGeckoProfile, which > fixes the testing problem with showing StartPane because now none of the > tests set the flag to show the StartPane. This feels a bit fragile. Does this mean that we still depend on running testGeckoProfile before the other tests? I'm fine with this as a solution to land now, but it feels like we should have a better way to disable this feature in tests.

Comment on attachment 8501441 [details] [diff] [review] Patch: Launch first run only on non-external intent v3 Review of attachment 8501441 [details] [diff] [review]: ----------------------------------------------------------------- ::: mobile/android/base/BrowserApp.java @@ +706,5 @@ > + final SharedPreferences prefs = GeckoSharedPrefs.forProfile(this); > + > + if (!prefs.contains(PREF_STARTPANE_ENABLED)) { > + return; > + } Do we need this check? Won't the getBoolean call below just return the default value (false) if the pref doesn't exist? @@ +756,5 @@ > } > > @Override > + public void onAttachedToWindow() { > + checkStartPane(this, getIntent().getAction()); Why did you decide to put this in onAttachedToWindow? Could you add some comments to these checkStartPane calls to explain the cases they're trying to catch?

> > The StartPane is actually being shown in all the robocop tests in the tree, > > but since the first test is testGeckoProfile, which doesn't do UI tests, the > > tests haven't been failing. However, with the new SharedPreferences usage, > > the StartPane is being delayed until the first test after that and is > > failing because there are UI tests. > > I'm a bit confused by this. What type of intent launches the tests? > Shouldn't the intent action check catch this? The intent ACTION_VIEW is the intent sent by an intent from an external app to open a url - the intent to launch tests is ACTION_MAIN, which is just launching the app. http://mxr.mozilla.org/mozilla-central/source/mobile/android/base/tests/BaseTest.java#104 > > Now with clearing SharedPreferences after running testGeckoProfile, which > > fixes the testing problem with showing StartPane because now none of the > > tests set the flag to show the StartPane. > > This feels a bit fragile. Does this mean that we still depend on running > testGeckoProfile before the other tests? > Nope! The start pane doesn't start for any of the tests, which is why I was having a really hard time repro-ing - it's the interaction between testGeckoProfile (which creates a new GeckoProfile, but didn't clean it up) and subsequent tests that launch BrowserApp that caused the StartPane to show up. Basically, the original approach was to launch the start pane only when we create a new profile, by firing off a broadcast in the GeckoProfile creation code. This had the benefit of not needing to mess around with prefs, or worry about showing onboarding more than once. However, this means we only ever get to check once (when creating a new profile) if we want to display onboarding, so if you happened to launch Fennec for the first time from an external intent, we'd never check if we wanted to launch Onboarding again. The patch for this approach sets the pref for whether to launch onboarding during profile creation, and then checks it (and possibly sets it) later. > I'm fine with this as a solution to land now, but it feels like we should > have a better way to disable this feature in tests. I think this is the right way to do this - it might be useful to do a followup on disabling showing the StartPane in GeckoProfile, maybe. > ::: mobile/android/base/BrowserApp.java > @@ +706,5 @@ > > + final SharedPreferences prefs = GeckoSharedPrefs.forProfile(this); > > + > > + if (!prefs.contains(PREF_STARTPANE_ENABLED)) { > > + return; > > + } > > Do we need this check? Won't the getBoolean call below just return the > default value (false) if the pref doesn't exist? > I guess technically not - for some reason, I feel like contains() is cheaper than getBoolean(), and since this runs every time we start BrowserApp, I just added the check. They're probably the same though, so I can take it out. > @@ +756,5 @@ > > } > > > > @Override > > + public void onAttachedToWindow() { > > + checkStartPane(this, getIntent().getAction()); > > Why did you decide to put this in onAttachedToWindow? Could you add some > comments to these checkStartPane calls to explain the cases they're trying > to catch? Adding this check any earlier than onAttachedToWindow causes crashing, because parts of Gecko haven't been set up (bug 1077583). Good point, I'll add some comments.

Approval Request Comment [Feature/regressing bug #]: First run (bug 1042809) didn't handle launching from an external intent [User impact if declined]: Users might see first run even if they launch Firefox to open a link from another app [Describe test coverage new/current, TBPL]: local testing, green aurora try [Risks and why]: low, changing from Broadcast to class method calls [String/UUID change made/needed]: none Aurora try run: https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=81c6157a74eb

Verified as fixed on Build: Firefox for Android 35.0a1 (2014-10-10) Device: Nexus 4 (Android 4.4.4)

Comment on attachment 8502953 [details] [diff] [review] Aurora patch: First run external intent Good catch. Happy to take this while we're still (barely) on Aurora. Aurora+

Verified as fixed in Build: Firefox for Android 34.0a2 (2014-10-12) Device: Nexus 4 (Android 4.4.4)