994152 - Loop needs a "do not disturb" control

Status: VERIFIED FIXED

(Hello (Loop) :: Client, defect, P2)

Description

[Adam Roach [:abr]]

With the current design, once a URL is handed out, a remote user can trigger an interruption as long as the browser is running. There is no user-visible way to prevent this from happening. Even for Nightly, this is potentially intrusive enough to cause people to abandon the browser until they can be assured that they have control over whether this happens.

For MLP, the proposal is that this should be a control (e.g., a checkbox or something similar) on the Loop panel (the one that creates call URLs). Its setting should persist across browser sessions.

From a technical perspective, all that this checkbox needs to do is prevent an incoming call from alerting the user. Long term, we want client-side logging of calls (including missed calls), which means the client still wants to know about these calls. I would propose that the most future-proof may of implementing this is by keeping the simple push registration up, but not actually initiating a call when a simple push notification arrives.

Comment 1

[Maire Reavy [:mreavy]]

This bug is to implement the checkbox to disable Loop

Comment 2

[Nicolas Perriault (:NiKo`) — needinfo me if you need my attention]

Blocked on this one as I need a way for communicating from the Panel (which holds the checkbox) and the Loop "service" (which is responsible of opening the chat window). Any hint?

Comment 3

[Mark Banner (:standard8)]

Yes, we're just about to rewrite that service and some of the APIs - bug 1000007. It'll probably be best just to wait a little for that to happen.

Comment 4

[Nicolas Perriault (:NiKo`) — needinfo me if you need my attention]

I think I've got the basics of the patch here: https://github.com/adamroach/gecko-dev/compare/privs-landing...bug-994152-do-not-disturb

It's possibly over engineered atm as I spent many hours messing around with the architecture in place & build system (yay you need to rebuild everytime a jsm changes). Also, it needs tests and (lots of) cleanup. But I'd appreciate some early feedback :)

Comment 5

[Adam Roach [:abr]]

(In reply to Nicolas Perriault (:NiKo`) from comment #4)
> I think I've got the basics of the patch here:
> https://github.com/adamroach/gecko-dev/compare/privs-landing...bug-994152-do-
> not-disturb
> 
> It's possibly over engineered atm as I spent many hours messing around with
> the architecture in place & build system (yay you need to rebuild everytime
> a jsm changes). Also, it needs tests and (lots of) cleanup. But I'd
> appreciate some early feedback :)

I think we have an issue here in that you're manipulating an "enabled" flag, which isn't really what this is. I would expect that setting something called "enabled" to false would disable the button, the panel, and everything else. I would expect a "do not disturb" control to disable incoming notifications and leave all other functionality intact.

So, while we do probably need an "enabled" flag at some point, this isn't it. This should be something like "loop.do_not_disturb".

Comment 6

[Dan Mosedale (:dmosedale, :dmose)]

(In reply to Nicolas Perriault (:NiKo`) from comment #4)

> build system (yay you need to rebuild everytime a jsm changes).

I believe if you add the flag -purgecaches onto the end of your "mach run" line, you won't need to rebuild.

Comment 7

[Nicolas Perriault (:NiKo`) — needinfo me if you need my attention]

So this is the current state of the patch: https://github.com/adamroach/gecko-dev/compare/privs-landing...bug-994152-do-not-disturb

It works, but I'm struggling writing xpcshell tests atm, mostly because MozLoopService hides a lot of its implementation (which is good) but hence why hooking into the actual behavior when setting the `doNotDisturb` flag is harder to test, because I can't simply stub the internal `openChatWindow` method to check it's not called when the flag is enabled. That implies I should be stubbing the real `openChat` method, and after a few hours investigating I still don't know how to do that.

To be honest, I feel like we could expose a bit more to the public API, as MozLoopService is still something intended for gecko internal use anyway. The injected mozLoop object won't ever expose more than what we define explicitely, so I can't see a major issue here (but maybe I'm missing something obvious, I'm still very new to all this).

So, I'd love some feedback and possibly help writing these missing tests. I've commented the test code appropriately.

Comment 8

[Nicolas Perriault (:NiKo`) — needinfo me if you need my attention]

Actually the most difficult part is probably not stubbing `Chat.open`, but rather simulating an incming call notification so `MozLoopServiceInternal#onHandleNotification` is actually called, which itself then calls `openChatWindow`… 

Looking at what's being done currently for bug 994151 (https://github.com/adamroach/gecko-dev/compare/privs-landing...bug-994151), it appears we're setting up a real test HTTP server and a mock WebSocket service provider to simulate the whole ecosystem; to me it feels like we're moving towards functional testing here.

I'm challenging that idea; I feel like while this is probably needed, we could also expose a bit more to the MozLoopService object, so that would both ease testing and be more explicit about it's actual scope of action. Indeed, MozLoopService is responsible of opening chat windows, I feel like this responsibility should be reflected in its public API, hence in its own state. What should be stubbed is obviously the *actual* opening of a real chat window.

Right now, MozLoopService feels more like a big black box, and is painful to unit test, which is a bit of shame.

Thoughts?

Comment 9

[Nicolas Perriault (:NiKo`) — needinfo me if you need my attention]

s/hence in its own state/hence in its own unit tests/

Comment 10

[Mark Banner (:standard8)]

Attachment: Example notification test

I've just knocked this together as an example test for notification based on the current structure.

It would need more work (sharing of code with the other tests, tidy up of function names etc), but it does work.

I'll add some more comments in a moment, but I thought it useful to add this first.

Comment 11

[Mark Banner (:standard8)]

(In reply to Nicolas Perriault (:NiKo`) from comment #8)
> I'm challenging that idea; I feel like while this is probably needed, we
> could also expose a bit more to the MozLoopService object, so that would
> both ease testing and be more explicit about it's actual scope of action.
> Indeed, MozLoopService is responsible of opening chat windows, I feel like
> this responsibility should be reflected in its public API, hence in its own
> state. What should be stubbed is obviously the *actual* opening of a real
> chat window.

I can't see much that we can sensibly expose in the public API. Whilst we could expose opening of the chat window, this doesn't help with getting a push notification into the service, and I don't see a valid way to expose an API that would do that.

However, in thinking about the possible future, one thing we could do is to move the PushHandlerHack to a separate module.

This would at least cut down on some of the complexity, (we'd stub the push handler for MozLoopService tests, not the websocket). This would also be similar to what we're likely to end up if/when we move to the main gecko push handler service.

> Right now, MozLoopService feels more like a big black box, and is painful to
> unit test, which is a bit of shame.

Like we discussed on irc, I think it depends at what level we want to unit test things. I think the API for MozLoopService is currently the right level, we have to remember that unlike the more web content parts, we can't necessarily test each individual function because of true "private" functions.

I think moving out the PushHandlerHack is probably the best way to go. It can then easily be stubbed for the testing of the loop service itself.

Comment 12

[Nicolas Perriault (:NiKo`) — needinfo me if you need my attention]

> I don't see a valid way to expose an API that would do that.

Exposing `onHandleNotification` (we could also rename it to `incoming()`) would solve just that. Would there be any reason for people to use it? Well:

- if this service is aimed at being used as a call platform controller, being able to hook in what happen on incoming calls seems like a fair expectation.
- if this service is not aimed at being used by anybody else than Loop, why do we bother restricting that much its public API?

If exposing more of the service responsibility to the public API solves testing issues and helps move faster (and I suspect tests would also run faster in a unit fashion), why don't we want to do that?

Don't get me wrong, I love the idea of having func tests for the whole loop tech stack, but I keep feeling there's room for a more granular MozLoopService API.

Comment 13

[Dan Mosedale (:dmosedale, :dmose)]

A few things that jump out at me:

* it seems highly likely that, over time, we're going to want to do both integration/functional testing here as well as unit testing

* The fact that is PushHandlerHack is its own object is great, because it's effectively an untested implementation of the Humble Object pattern <http://xunitpatterns.com/Humble%20Object.html>

Instead of waiting for a future where we move PushHandlerHack into its own module, could we thread the needle here by exposing PushHandlerHack either as property on MozLoopService, or as another EXPORTED_SYMBOL now, possibly prefixed by an _.
It could move to a separate JSM once having it here really becomes a problem.

This would seem to allow us to have our cake (keep the integration-style tests bug 994151 intact) and eat it too (allowing the simpler, finer-granularity unit tests as proposed here).

Thoughts?

Comment 14

[Alexis Metaireau (:alexis)]

Any reason we're using a config flag for this? Cannot we just persist this information into the storage?

Comment 15

[Mark Banner (:standard8)]

(In reply to Alexis Metaireau (:alexis) from comment #14)
> Any reason we're using a config flag for this? Cannot we just persist this
> information into the storage?

This has to be accessible from privileged gecko space, as well as the content space. The simplest option for that is to use the config flag. We could try and reach into about:loop*'s local storage from privileged space, or set up an extra store locally, but that seems unnecessary extra work - especially as things like this tend to get saved in config anyway.

Comment 16

[Nicolas Perriault (:NiKo`) — needinfo me if you need my attention]

(In reply to Dan Mosedale (:dmose) from comment #13)
> * it seems highly likely that, over time, we're going to want to do both
> integration/functional testing here as well as unit testing

Sure :)

> * The fact that is PushHandlerHack is its own object is great, because it's
> effectively an untested implementation of the Humble Object pattern
> <http://xunitpatterns.com/Humble%20Object.html>

Yes, I wish we could have the same for the service internal event system.

> could we thread the needle here by exposing PushHandlerHack either
> as property on MozLoopService

Yes that would certainly help unit testing, but it would also expose something larger than just appropriate hooks… The more I think about it, the more I think that exposing a Singleton instance is what makes things actually hard to test here.

If we had a proper MozLoopService constructor, we could easily inject dependencies (eg. the push hack) in a test context. Also, using an observer would possibly help controling what's being sent to the service as notification events a bit more agnostically.

The MozLoopService.jsm file would create its own instance by using the constructor, and would export that constructor so the test environment could build its own, injecting its own dependencies.

Comment 17

[Mark Banner (:standard8)]

As discussed with Nicolas on irc, I think its worth looking at what the observer mechanism would look like - taking a look at the current core push service implementation, it seems they favour events there. So if we can use something lightweight, then we'll hopefully end up with something similar.

Re dependency injection, I'm cautious about that as I think it adds unnecessary complexity to production code (especially as gecko allows us to replace services/components with mocks), but I'm happy to see what it looks like - especially for deps internal in that file.

I believe Nicolas is now working on a mock-up for this.

Comment 18

[Maire Reavy [:mreavy]]

I believe the work for "do not disturb" is done and we're discussing how to write the tests for it.  I'd like to split off the tests for this feature into a new bug.   I'd be fine with landing this feature and landing the tests for it a few days later.

Niko -- Could you create a new bug for the tests and get the patch for "do not disturb" ready for review on this bug?

Comment 19

[Nicolas Perriault (:NiKo`) — needinfo me if you need my attention]

Sure, I'll update the patch to fit latest changes in the tree and submit it for review asap. I'll then file a new bug for missing tests, if any.

Comment 20

[Nicolas Perriault (:NiKo`) — needinfo me if you need my attention]

Attachment: bug-994152-do-not-disturb.diff

This patch adds a new `loop.do_not_disturb` pref, set to false by default, which allows the user to disable incoming call notifications through a checkbox control available in the panel.

A `doNotDisturb` controlled property is exposed to the mozLoop injected object, so it's possible to toggle its value from content.

Comment 21

[Nicolas Perriault (:NiKo`) — needinfo me if you need my attention]

Please note I didn't add the missing tests to ensure the chat window isn't opened on incoming notifications when the doNotDisturb flag is enabled. I'll create a separate bug for adding these, as suggested by :mreavy.

Comment 22

[Mark Hammond [:markh] [:mhammond]]

Comment on attachment 8431565 [details] [diff] [review]
bug-994152-do-not-disturb.diff

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

LGTM (although I didn't really look at the loop/content parts)

Comment 23

[Mark Banner (:standard8)]

Comment on attachment 8431565 [details] [diff] [review]
bug-994152-do-not-disturb.diff

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

::: browser/components/loop/content/js/panel.js
@@ +150,5 @@
> +
> +    render: function() {
> +      this.$el.html(this.template());
> +      // Do not Disturb sub view
> +      new DoNotDisturbView({el: this.$(".dnd")}).render();

The memory model is slightly strange here. What is keeping the view alive once it has been created, and what is releasing it if/when the render function is called again?

Do we even need to re-initialise it on a second render call?

::: browser/components/loop/test/xpcshell/test_loopservice_dnd.js
@@ +20,5 @@
> +}
> +
> +function test_set_do_not_disturb() {
> +  Services.prefs.setBoolPref("loop.do_not_disturb", false);
> +  MozLoopService.doNotDisturb = true;

nit: blank line before this one please.

Comment 24

[Nicolas Perriault (:NiKo`) — needinfo me if you need my attention]

Attachment: bug-994152-do-not-disturb-2.diff

New patch addressing :standard8 comments:

(In reply to Mark Banner (:standard8) from comment #23)
>
> The memory model is slightly strange here. What is keeping the view alive
> once it has been created, and what is releasing it if/when the render
> function is called again?

Yes. I attached the created view to a property, so I think it's easier to be garbage collected.

> Do we even need to re-initialise it on a second render call?

Yes, because it's safe to ensure the dnd view is recomputed everytime the panel is rendered; this allows ensuring that the displayed state is under control and always matches the current doNotDisturb value.

We should really have this discussion about React ;)

> nit: blank line before this one please.

Fixed.

Comment 25

[Mark Banner (:standard8)]

Comment on attachment 8432629 [details] [diff] [review]
bug-994152-do-not-disturb-2.diff

Changes look good. r=Standard8

Comment 26

[Mark Banner (:standard8)]

https://hg.mozilla.org/projects/elm/rev/ea932c5d8fc3

Closing for tracking purposes.

Comment 27

[Mark Banner (:standard8)]

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

Comment 28

[Stefan Plewako [:stef]]

Please use meaningful string ids in the future.

Comment 29

[u279076]

Verified fixed through basic dogfooding.