817259 - ftuStarting not removed from screen.class

Status: RESOLVED FIXED

(Firefox OS Graveyard :: Gaia::System, defect)

Description

[Myk Melez [:myk] [@mykmelez]]

Attachment: patch v1: removes ftuStarting if FTU is skipped for any reason

There are four conditions under which the FTU is skipped, but the ftuStarting class is only removed from the screen for one of them, which causes visual (and behavioral?) glitches when the FTU is skipped for one of the other reasons.

(I found this because Firefox OS Simulator unsets ftu.manifestURL in order to skip the FTU when B2G is run in the Simulator, which causes those glitches to appear, since ftuStarting never gets removed in that case.)

ftuStarting should be removed from the screen if the FTU is skipped for any reason.  Here's a patch that does that.

Comment 1

[Etienne Segonzac (:etienne)]

Comment on attachment 687364 [details] [diff] [review]
patch v1: removes ftuStarting if FTU is skipped for any reason

Review of attachment 687364 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks for the fix!

::: apps/system/js/window_manager.js
@@ +779,4 @@
>    // reference to the app and launch it.
>    function retrieveFTU() {
>      window.asyncStorage.getItem('ftu.enabled', function getItem(launchFTU) {
> +      document.getElementById('screen').classList.add('ftuStarting');

Probably no harm in keeping this line in place...

Comment 2

[Etienne Segonzac (:etienne)]

Comment on attachment 687364 [details] [diff] [review]
patch v1: removes ftuStarting if FTU is skipped for any reason

Potential developer blocker, safe change.

Comment 3

[Daniel Coloma:dcoloma]

As FTU has been developed mostly by Borja and Fernando, I'd suggest they review this patch and further patches for FTU

Comment 4

[Myk Melez [:myk] [@mykmelez]]

Comment on attachment 687364 [details] [diff] [review]
patch v1: removes ftuStarting if FTU is skipped for any reason

This is not really a patch for FTU, it's a flaw in the logic that determines whether or not to start the FTU, and almost none of that code is written by Borja and Fernando.  See the blame for lines 778-810 in window_manager.js <https://github.com/mozilla-b2g/gaia/blame/master/apps/system/js/window_manager.js> for the details.

Nevertheless, Borja did write the one line of code in that block that caused the regression, so requesting feedback from him on this fix.

Borja: note that moving that line isn't strictly necessary, as Etienne notes in comment 1.  However, it does align the addition of ftuStarting with its removal, making it so the class is added in all of the four cases in which it's also removed early because the FTU doesn't end up starting.

Comment 5

[Etienne Segonzac (:etienne)]

(In reply to Myk Melez [:myk] [@mykmelez] from comment #4)
> However, it does align the addition of ftuStarting with its
> removal, making it so the class is added in all of the four cases in which
> it's also removed early because the FTU doesn't end up starting.

Good point, now I like it, please keep it this way :)

Comment 6

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

Comment on attachment 687364 [details] [diff] [review]
patch v1: removes ftuStarting if FTU is skipped for any reason

Review of attachment 687364 [details] [diff] [review]:
-----------------------------------------------------------------

I have seen a very annoying bug where there is no status bar and I have been told this is the root cause. a+.

Comment 7

[Myk Melez [:myk] [@mykmelez]]

I don't have commit privileges to the Gaia repository.  Could someone merge pull 6770 <https://github.com/mozilla-b2g/gaia/pull/6770> on my behalf?

Comment 8

[Etienne Segonzac (:etienne)]

https://github.com/mozilla-b2g/gaia/commit/b0e12772116ba00c78a9727b59059712198f62e3