Bug 825017 - [B2G][FTE] The import Facebook contacts button now works co... #7230

Merged
merged 1 commit into from Jan 7, 2013

Conversation

Projects
None yet
3 participants
Member

michalbe commented Dec 28, 2012

...rrectly

Member

michalbe commented Dec 28, 2012

@fcampo: r?

apps/communications/ftu/js/wifi.js
- this.api = window.navigator.mozWifiManager;
- this.changeStatus();
- this.gCurrentNetwork = this.api.connection.network;
+ this.api = window.navigator.mozWifiManager;
@fcampo

fcampo Dec 28, 2012

Contributor

proper indentation

apps/communications/ftu/js/navigation.js
@@ -198,7 +198,7 @@ var Navigation = {
simOption.classList.add('disabled');
}
// If we have 3G or Wifi activate FB import
- if (WifiManager.isConnected || DataMobile.isDataAvailable) {
+ if (navigator.onLine) {
@fcampo

fcampo Dec 28, 2012

Contributor

Gives faulty values. Not connected to mobile data nor wifi, it gives back true, which is not acceptable.

From documentation of the function "So while you can assume that the browser is offline when it returns a false value, you cannot assume that a true value necessarily means that the browser can access the internet."

@michalbe

michalbe Dec 28, 2012

Member

Oh, my bad. Fixing.

apps/communications/ftu/js/wifi.js
+ this.api = window.navigator.mozWifiManager;
+ this.changeStatus();
+ // if we are connected since the very beginning 'onstatuschange'
+ // event never fires, so we need to switch 'isConnected' to true manually
@fcampo

fcampo Dec 28, 2012

Contributor

we shouldn't be able to be connected from the beginning on FTU, as the app only is launched once, at the first phone startup, and doesn't load any saved values from settings.

@michalbe

michalbe Dec 28, 2012

Member

But it works like this - even if I reset and reboot everything I'm connected on startup. Any thoughts why?

@fcampo

fcampo Dec 28, 2012

Contributor

oh, I didn't think about reboot in the middle of FTU use case. Then it might read the saved wifi if connected on the first launch. But it should respond to 'onstatuschange' anyway, since we pass through the different states ['scanning', 'associated', 'connected']. Frankly, no idea why is ignoring it in that build. I'm still unable to reproduce it

@michalbe

michalbe Dec 29, 2012

Member

Maybe it happen before ftu is launched? I have no idea, but every time I flash I'm already connected to the wifi so this is the first possible workaround in my mind.

Member

michalbe commented Dec 29, 2012

If we don't want user to be connected on startup I can instead of changing 'isConnected' just disconnect the user in init(). @fcampo, what do you think?

Member

michalbe commented Jan 2, 2013

@fcampo: resolved conflicts and add disconnection from the Wifi in case it's connected instead of fake, manual connection.

apps/communications/ftu/js/wifi.js
+
+ // if we are connected since the very beginning 'onstatuschange'
+ // event never fires, so we disconnect from the wifi manually
+ // Bug #825017 https://bugzilla.mozilla.org/show_bug.cgi?id=825017
@alivedise

alivedise Jan 6, 2013

Member

Don't specify a fixed bug number in the comment..especially fixed by this patch. Do you mean to specify another bug?

apps/communications/ftu/js/wifi.js
this.gCurrentNetwork = this.api.connection.network;
+ if (this.gCurrentNetwork !== null) {
+ this.api.forget(this.gCurrentNetwork);
+ this.gCurrentNetwork = null;
@alivedise

alivedise Jan 6, 2013

Member

Looks good but is this expected?
I would say this is O.K. but I preferred to let the platform keep remembering the network.
Why not check this.api.connection.status and set WifiManager.isConnected to true in the very beginning?
Ya, the platform doesn't call statuschange, it's a problem. Vincent Chang is here in Berlin, let's need info f2f from him.

Bug 825017 - [B2G][FTE] The import Facebook contacts button now works…
… correctly

fixes

Fix

Display logic fix in fb-ftu button

Forget the wifi on startup of ftu

cleaning
@@ -198,7 +198,7 @@ var Navigation = {
simOption.classList.add('disabled');
}
// If we have 3G or Wifi activate FB import
- if (WifiManager.isConnected || DataMobile.isDataAvailable) {
+ if (WifiManager.api.connection.status === 'connected' || DataMobile.isDataAvailable) {
@michalbe

michalbe Jan 7, 2013

Member

We don't need to introduce another variable if we check 'isConnected' only here. I use the wifi api instead as proposed by @alivedise.

Contributor

fcampo commented Jan 7, 2013

After speaking with UX (Ayman) we agreed on the behaviour: FTU should start with tabula rasa state, so no remembering networks. This workaround forgetting the wifi if connected works fine. r=me

Member

michalbe commented Jan 7, 2013

Thanks!

michalbe added a commit that referenced this pull request Jan 7, 2013

Merge pull request #7230 from michalbe/fb-button-ftu
Bug 825017 - [B2G][FTE] The import Facebook contacts button now works co...

@michalbe michalbe merged commit 2e33b9e into mozilla-b2g:master Jan 7, 2013

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment