Wesley Johnston (:wesj) Assignee
	Description • 10 years ago

See bug 1042194 for the framework piece here. Among the first widgets I wanted to add was a wifi toggle.

	Wesley Johnston (:wesj) Assignee
	Comment 1 • 10 years ago

This implements this using the new infrastructure. I added a little "in-progress" animation when the button is clicked as it can take awhile. Once we hear back from Java that things are connected, we auto-refresh the page.

This just uses a button as opposed to the slider from https://bug1042199.bugzilla.mozilla.org/attachment.cgi?id=8460516 . I'm torn on whether I like that or not. The slider was neat, but made me want to flip it back and forth. I don't think we need to support turning off wifi on these pages. Will attach a screenshot. Curious what UX thinks?

Build at: http://people.mozilla.org/~wjohnston/error.apk
Animation test page at: http://people.mozilla.org/~wjohnston/wifi.html

	Anthony Lam (:antlam) Find me, @antlam on Slack!
	Comment 3 • 10 years ago

I think this has the potential to be very useful to users and that could go a long way especially in these situations of frustration. 

For the time being I don't have much design feedback to give other than maybe sorting out the padding to balance it out a bit.

Let's try having equal padding above and beneath the copy for "1. and 2." and then using the same margin between the 2 buttons.

	Wesley Johnston (:wesj) Assignee
	Comment 4 • 10 years ago

This is simplified somewhat to do its listening for online events entirely in JS. No more timer in Java. Instead, we'll keep "loading" in JS until we get an online event, then we'll reload the page.

It can happen that the wifi indicator turns on a little while before we're really online. I tried just polling the network status instead, and it does say the link is up at that point, but trying to use the network will fail. I wonder if maybe the button should change states more frequently to indicate an ongoing process. i.e. we could change the text as it goes "Connecting..." "Finding networks", etc. Or we could show more of a progress bar...

	:Margaret Leibovic
	Comment 5 • 10 years ago

Comment on attachment 8468902 [details] [diff] [review]
Patch v2

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

::: mobile/android/modules/NetErrorHelper.jsm
@@ +76,5 @@
> +        }
> +      }
> +  },
> +
> +  handleEvent: function(event) {

Drive-by: Maybe we should change this API to just pass the node to the handler, rather than the event.

@@ +84,5 @@
> +    }
> +
> +    if (!node) {
> +      return;
> +    }

Something would be broken if this is ever the case.

	Mark Finkle (:mfinkle) (use needinfo?)
	Comment 6 • 10 years ago

Comment on attachment 8468902 [details] [diff] [review]
Patch v2

>diff --git a/mobile/android/base/GeckoNetworkManager.java b/mobile/android/base/GeckoNetworkManager.java

>+    public void handleMessage(final String event, final NativeJSObject message,
>+                              final EventCallback callback) {
>+        if (event.equals("Wifi:Enable")) {
>+            final WifiManager mgr = (WifiManager) mApplicationContext.getSystemService(Context.WIFI_SERVICE);
>+            final Timer timer = new Timer();

What does the timer do?

>diff --git a/mobile/android/modules/NetErrorHelper.jsm b/mobile/android/modules/NetErrorHelper.jsm

>+  onPageShown: function(browser) {

>+        var nodes = browser.contentDocument.querySelectorAll("#wifi");

let

>+    // Show indeterminate progress while we wait for the network.
>+    node.disabled = true;
>+    node.classList.add("thinking");

"thinking" ?

>+    node.ownerDocument.addEventListener('online', this.onOnline.bind(this, Cu.getWeakReference(node)));

"online"

>diff --git a/mobile/android/themes/core/netError.css b/mobile/android/themes/core/netError.css
>diff --git a/mobile/locales/en-US/overrides/netError.dtd b/mobile/locales/en-US/overrides/netError.dtd

>+  <li>If you are unable to load any pages, check your device's data or Wi-Fi connection.
>+    <button id='wifi'>Enable wifi</button>

Looks like we use "Wi-Fi" not "wifi"

>+    <button id='wifi'>Enable wifi</button>

Same

>+<!ENTITY proxyResolveFailure.longDesc3 "

>+    <button id='wifi'>Enable wifi</button>

Same

>+  <li>If you are unable to load any pages, check your mobile device's data or Wi-Fi connection.
>+    <button id='wifi'>Enable wifi</button>

Same

Lastly, I think we should add some telemetry to this. Probably in the JS so we know the exact reason for the WiFi toggle.

	Wesley Johnston (:wesj) Assignee
	Comment 7 • 10 years ago

Updated. I switched this over to use "network:link-status-changed" Observer notifications since they seem to fire around the same time that the wifi indicator appears on your phone. Still, reloading at that point fails sometimes, but I think the phone showing WiFi is working and then nothing happening for 5 seconds is a worse experience on our end. It looks like we're just frozen.

I also did some style updates to try and create a more uniform spacing between things vertically throughout the error pages. I'll post some shots of the wifi toggle. This affects other error pages as well though. I went through pretty carefully to make sure they still look fine, but there's some risk there.

	Wesley Johnston (:wesj) Assignee
	Comment 8 • 10 years ago

Updated screenshot. There's also a build with this at:
http://people.mozilla.com/~wjohnston/wifi.apk

	Mark Finkle (:mfinkle) (use needinfo?)
	Comment 9 • 10 years ago

Comment on attachment 8470993 [details] [diff] [review]
Patch

>diff --git a/mobile/android/base/GeckoNetworkManager.java b/mobile/android/base/GeckoNetworkManager.java

>+    public void handleMessage(final String event, final NativeJSObject message,
>+                              final EventCallback callback) {
>+        if (event.equals("Wifi:Enable")) {

I'm fine with it, but "Wifi:Enable" might be a bit vague since the code is more like "Wifi:TryYourHardestToConnect"

>+            final WifiManager mgr = (WifiManager) mApplicationContext.getSystemService(Context.WIFI_SERVICE);
>+
>+            if (!mgr.isWifiEnabled()) {
>+                mgr.setWifiEnabled(message.getBoolean("enabled"));

Do we need the boolean? You can't use the boolean to disable WiFi.

>+                // If Wifi is enabled, maybe you need to select a network
>+                Intent intent = new Intent(android.provider.Settings.ACTION_WIFI_SETTINGS);
>+                intent.addFlags(Intent.FLAG_ACTIVITY_NEW_TASK);
>+                mApplicationContext.startActivity(intent);

Someday, I'd like to add something to check for "connected, but not sending data" - but that is a new bug.

>diff --git a/mobile/android/modules/NetErrorHelper.jsm b/mobile/android/modules/NetErrorHelper.jsm
>+handlers.wifi = {
>+  onPageShown: function(browser) {
>+      // If we have a connection, don't bother showing the wifi toggle.
>+      let network = Cc["@mozilla.org/network/network-link-service;1"].getService(Ci.nsINetworkLinkService);
>+      if (network.isLinkUp && network.linkStatusKnown) {
>+        let nodes = browser.contentDocument.querySelectorAll("#wifi");
>+        for (let i = 0; i < nodes.length; i++) {
>+          nodes[i].style.display = "none";
>+        }
>+      }

I wonder if we should invert this. We could hide the wifi stuff and only show it if needed. I suppose it's fine for now. If we notice any "flashes of wifi" we can address it.

>+  handleClick: function(event) {

>+    UITelemetry.addEvent("wifitoggle.1", "neterror", null);

I want to change this up a bit. I think we will be adding more actions on neterror, so let's make it like this:

      UITelemetry.addEvent("neterror.1", "button", "wifitoggle");

>+    this.node = Cu.getWeakReference(node);
>+    Services.obs.addObserver(this, "network:link-status-changed", false);

Should we "weak ref" this observer? In case the page closes without removing it?

>+  observe: function(subject, topic, data) {
>+    let node = this.node.get();
>+    if (!node) {
>+      return;
>+    }

You should remove the observer though, right? Maybe do it right away regardless of what happens?

>+    if (network.isLinkUp && network.linkStatusKnown) {
>+      // If everything worked, reload the page
>+      UITelemetry.addEvent("wifitoggle.reload.1", "neterror", null);

        UITelemetry.addEvent("neterror.1", null, "wifitoggle.reload");

>+      Services.obs.removeObserver(this, "network:link-status-changed");

As above, maybe we should remove this right away. And also make it a weakref?

>+      // Even at this point, Android sometimes lies about the real state of the network and this reload request fails.
>+      // Add a 500ms delay before refreshing the page.
>+      node.ownerDocument.defaultView.setTimeout(function() {
>+        node.ownerDocument.location.reload(false);

Is there a chance of racing the page closing? I could close the page and 'node' would be bogus. Maybe you should get the node again and check for null?

>diff --git a/mobile/android/themes/core/netError.css b/mobile/android/themes/core/netError.css

>+  --moz-vertical-spacing: 10px;
>+  --moz-background-height: 32px;

Are CSS variables OK to use?

r- to see a new patch and give you a chance to answer some questions

	Wesley Johnston (:wesj) Assignee
	Comment 10 • 10 years ago

(In reply to Mark Finkle (:mfinkle) from comment #9)
> Do we need the boolean? You can't use the boolean to disable WiFi.
No. When this looked like a "toggle" it made more sense. Removed.

> Should we "weak ref" this observer? In case the page closes without removing
> it?
Seems like a good idea to me.

> You should remove the observer though, right? Maybe do it right away
> regardless of what happens?
I was worried we'd get a network-link-status event for some other reason here. It seems smart to wait until we have the one we want, so I left this in.

> Is there a chance of racing the page closing? I could close the page and
> 'node' would be bogus. Maybe you should get the node again and check for
> null?
I'm not sure I understand. We already have the node here since we called get on the WeakRef. Its closured into this function, so there's really no point in getting it again?

> Are CSS variables OK to use?
Should be. This stylesheet isn't applied to normal pages. Just error pages, so there's no leakage of data.

	Mark Finkle (:mfinkle) (use needinfo?)
	Comment 11 • 10 years ago

(In reply to Wesley Johnston (:wesj) from comment #10)

> > Is there a chance of racing the page closing? I could close the page and
> > 'node' would be bogus. Maybe you should get the node again and check for
> > null?
> I'm not sure I understand. We already have the node here since we called get
> on the WeakRef. Its closured into this function, so there's really no point
> in getting it again?

You get the node, then do a setTimeout(..., 500) so there are 500 milliseconds before you use node again, inside the closure. If the page closes in that 500 milliseconds you might have a problem.

	Mark Finkle (:mfinkle) (use needinfo?)
	Comment 12 • 10 years ago

Wes - I guess you could argue that since the setTimeout is tied to the document, if the document is killed/closed, then the setTimeout should be harmless.

	Mark Finkle (:mfinkle) (use needinfo?)
	Comment 13 • 10 years ago

Comment on attachment 8475428 [details] [diff] [review]
Patch

>diff --git a/mobile/android/modules/NetErrorHelper.jsm b/mobile/android/modules/NetErrorHelper.jsm

>+  handleClick: function(event) {

>+    UITelemetry.addEvent("neterror.1", "button", "wifitoggle");
>+    // Show indeterminate progress while we wait for the network.

nit: Add a blank line before the comment

	Wesley Johnston (:wesj) Assignee
	Comment 14 • 10 years ago

https://hg.mozilla.org/integration/fx-team/rev/d53b90e407ce

	Wesley Johnston (:wesj) Assignee
	Comment 15 • 10 years ago

(In reply to Mark Finkle (:mfinkle) from comment #12)
> Wes - I guess you could argue that since the setTimeout is tied to the
> document, if the document is killed/closed, then the setTimeout should be
> harmless.

Yeah, as near as I can tell, all timers are cancelled when the page is closed or changed. That said, we have a strong reference to the node at this point, so I'm not worried about not having it when this closes. You did make me a bit worried that if you navigated to a new page, we'd redirect to it. But as you said, the timer is cancelled in that case so it never actually fires.

	Wesley Johnston (:wesj) Assignee
	Comment 16 • 10 years ago

[Tracking Requested - why for this release]:

	Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)
	Comment 17 • 10 years ago

https://hg.mozilla.org/mozilla-central/rev/d53b90e407ce

	Francesco Lodolo [:flod]
	Comment 18 • 10 years ago

If I'm correct, besides being wrong, this is going to create a nice yellow screen of death in localized builds

    5.12 -<!ENTITY connectionFailure.longDesc "&sharedLongDesc2;">
    5.13 +<!ENTITY connectionFailure.longDesc "&sharedLongDesc3;">

Sorry but I'll ask a back-out, better safe than sorry.

	Francesco Lodolo [:flod]
	Comment 19 • 10 years ago

'besides being wrong" was referring to the patch not changing the string ID.

So we're going to fallback to en-US for sharedLongDesc3, but then we have an existing string connectionFailure.longDesc that references a non existing entity sharedLongDesc2 if locales don't notice the change.

	Nigel Babu [:nigelb]
	Comment 20 • 10 years ago

Backed out in https://hg.mozilla.org/mozilla-central/rev/0b9dd32d1e16 on flod's suggestion.

	Francesco Lodolo [:flod]
	Comment 21 • 10 years ago

Note that there are actually a lot more of them

http://hg.mozilla.org/mozilla-central/diff/d53b90e407ce/mobile/locales/en-US/overrides/netError.dtd

    1.12 -<!ENTITY connectionFailure.longDesc "&sharedLongDesc2;">
    1.13 +<!ENTITY connectionFailure.longDesc "&sharedLongDesc3;">

    1.46 -<!ENTITY netInterrupt.longDesc "&sharedLongDesc2;">
    1.47 +<!ENTITY netInterrupt.longDesc "&sharedLongDesc3;">

    1.77 -<!ENTITY netReset.longDesc "&sharedLongDesc2;">
    1.78 +<!ENTITY netReset.longDesc "&sharedLongDesc3;">

    1.81 -<!ENTITY netTimeout.longDesc "&sharedLongDesc2;">
    1.82 +<!ENTITY netTimeout.longDesc "&sharedLongDesc3;">

	Mark Finkle (:mfinkle) (use needinfo?)
	Comment 22 • 10 years ago

While reviewing this, I did not suggest changing the "connectionFailure.longDesc" (and friends) entity because they themselves do not hold a string. They merely point to another entity and that entity was "bumped".

	Francesco Lodolo [:flod]
	Comment 23 • 10 years ago

It doesn't contain a string but a reference to another string, which is kind of worse, especially for a .DTD

This is the scenario if a localizer doesn't catch up with the change: it will ship with something into sharedLongDesc3, either the updated translation or a fallback to English, but it will have a broken file because connectionFailure.longDesc & C. don't have anything to fall back to.

	Wesley Johnston (:wesj) Assignee
	Comment 24 • 10 years ago

So the issue is locales will pick up the correct sharedLongDesc3 but not update connectionFailure.longDesc (or some other entity that points to sharedLongDesc2)? Do locales actually change connectionFailure.longDesc (I don't think they should be localizing that... they should just be picking up the en-US version...)

How do you recommend we fix it? The locales system is awful enough that I'll need some guidance. I don't think we can settle for "never change the text on error pages".

	Francesco Lodolo [:flod]
	Comment 25 • 10 years ago

At build time, missing strings in a localization will be replaced by English strings: that ensures that a build always has all the strings it needs.

sharedLongDesc3 will be missing in the localization file, so it will be either correctly localized or in English.

connectionFailure.longDesc already exists, tools won't report it as missing, so localizers won't notice that the content has been updated unless the tool they're using has that feature.

To fix this, you simply need to "rev" those entities too.
<!ENTITY connectionFailure.longDesc2 "&sharedLongDesc3;">
etc.

More details about string changes
https://developer.mozilla.org/en-US/docs/Mozilla/Localization/Localization_best_practices#Changing_existing_strings

	Wesley Johnston (:wesj) Assignee
	Comment 26 • 10 years ago

This revs the ids.

	Wesley Johnston (:wesj) Assignee
	Comment 27 • 10 years ago

https://hg.mozilla.org/integration/fx-team/rev/8f00edcf2962

	Ryan VanderMeulen [:RyanVM]
	Comment 28 • 10 years ago

https://hg.mozilla.org/mozilla-central/rev/8f00edcf2962

	Flaviu Cos
	Comment 29 • 10 years ago

Verified as fixed in builds:
- 35.0a1 (2014-09-05);
- 34.0a2 (2014-09-05);
Device: Google Nexus 7 (Android 4.4.4).

	Lawrence Mandel [:lmandel] (use needinfo)
	Comment 30 • 10 years ago

Release noted as "New: Toggle wifi on error pages".

	Stefan Deiac
	Comment 31 • 6 years ago

Based on comment 29 I will remove the qe-verify flag, thanks.