Closed Bug 988212 Opened 10 years ago Closed 10 years ago

Change the attention screen permission from |certified| to |privileged|

Categories

(Core :: DOM: Device Interfaces, defect)

Product:
Core
Component:
DOM: Device Interfaces
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla32
Project Flags:
feature-b2g 2.0

People

(Reporter: vingtetun, Assigned: vingtetun)

References

Details

(Whiteboard: ft:loop)

Attachments

(2 files, 6 obsolete files)

bug988212.patch
1.03 KB, patch
sicking
: review+
Details | Diff | Splinter Review
multiple.attention.screens.patch
48.97 KB, patch
etienne
: review+
swilkes
: ui-review?
rmacdonald
Details | Diff | Splinter Review 
In order to allow third party apps to implement dialer-like application or alarm-like application it would be nice to allow privileged app to use an attention screen.
Attached patch bug988212.patch — — Splinter Review 
It should not be too many security concerns with that. One thing that would be nice is to ensure an attention screen can not be fullscreen for now, but I can investigate that in a followup.
Attachment #8396982 - Flags: review?(fabrice)
If Paul is ok with the change I'll rubberstamp the patch.
Flags: needinfo?(ptheriault)
What happens if one app is using the attention screen, and another asks to use it? Does the latest request replace the content?  Is the dialer, for example, able to handle the case when another app takes over the attention screen?

Do we need to introduce some kind of hierarchy here (e.g. certified gets priority over privileged)? Or maybe we just need to make sure that certified apps that use the attention screen take into account the situation when another app uses attention screen while they are.

So overall, i think this should be fine but we should probably do some security testing once its implemented just to make sure. (especially with all the changes to window manager).
Flags: needinfo?(ptheriault)
(In reply to Paul Theriault [:pauljt] from comment #3)
> What happens if one app is using the attention screen, and another asks to
> use it? Does the latest request replace the content?  Is the dialer, for
> example, able to handle the case when another app takes over the attention
> screen?

There is already the case of others attention screen with the clock app. IIRC the call screen have the priority and is always displayed on top of other attention screen.

Others are stacked I think.

> 
> Do we need to introduce some kind of hierarchy here (e.g. certified gets
> priority over privileged)? Or maybe we just need to make sure that certified
> apps that use the attention screen take into account the situation when
> another app uses attention screen while they are.

We will likely figure this out with the WebRTC app. For the moment we should just let the WebRTC app use the attention screen and figure out what happens and fix it.

> So overall, i think this should be fine but we should probably do some
> security testing once its implemented just to make sure. (especially with
> all the changes to window manager).

Are you OK if the change to the window manager are done as a separate issue as this one is mostly in order to be able to go forward with dialer like application.
(In reply to Paul Theriault [:pauljt] from comment #3)
> What happens if one app is using the attention screen, and another asks to
> use it? Does the latest request replace the content?  Is the dialer, for
> example, able to handle the case when another app takes over the attention
> screen?

For the specific case of the dialer, IIRC the call screen is always placed on top of other screens loaded within the attention screen.
Jaime, can you find someone from your team to help figure out how we should handle multiple attention screen at the same time ?

For example the Call Screen is an attention screen, and so it can be on top of any window in the system. Same thing for the Alarm. Applications using WebRTC should be able to have such windows.

The question is how do we deal with a WebRTC attention window, when the call screen is already opened ?
Flags: needinfo?(jachen)
As attention screen is a Gaia concept, I was thinking about how it's working and how could we extend it to other communications services as Loop or others in the future.

Currently, for showing an Attention Screen with the appearance of a call-screen, being the one with the highest priority, we are checking the 'telephony' permission:

https://github.com/mozilla-b2g/gaia/blob/master/apps/system/js/attention_screen.js#L131

However, Attention Screen should be a resource not tied with a Communications permission, because we have several ways of making a call (Loop, Skype...). So there is no direct relation between the 'telephony' permission and the capability of calling (actually the current solution is only working for an specific scenario).

On the other hand, as we are working in Gaia level, we could handle all 'voice communications services' in the same way as we are doing currently with customizable 'Homescreen', just adding a new role 'voice-service' in the Manifest.

What does it means?
As the role 'homescreen' or 'lockscreen', this is not tied to any permission. It's only a way of telling us explicitely which type of apps you are, and telling our Core Apps that you are a candidate for having special privileges in the Firefox OS UI.

For example, telling that you are a 'Homescreen' app, means that you are not part of the Apps grid in our Homescreen, and it's part of an specific panel in Settings.

Using the same formula, an app which is defined as 'voice-service' (or 'voice-communication', the name is only an example) must have the same level of priority (in Gaia level) as the CallScreen App (which should have 'role': 'voice-service' as well). This could be the key to unlock other features like showing all 'voice-services' within the Contact Detail (imagine that we show there the capability of calling directly with Loop or CallScreen, only taking into account that they are the same type of role 'voice-service').

With this solution we can open the model to new apps as Loop, and we should define an interaction when a current call is happening, and a new one appears. There are several ways of doing it (showing a prompt/banner to choose the one we want to use, and hold the one which is not selected for example, or keeping the current priority model), but this is a UX proposal more than a technical one.

Wdyt?
Per comment 8, setting ni to Vivien
Flags: needinfo?(21)
I believe an alternative to use a role for voice services could be to check for the audio-telephony-channel permission if bug 990552 is finally done.
I am proposing to implement attention-window (https://bugzilla.mozilla.org/show_bug.cgi?id=attention-window) before we do this. Currently we didn't manage multiple attention screens very well.
In attention-window I plan to put attention-window on top of the appWindow who launches it as 'special' popup. Another problem I want to resolve is the 'slide-down' transition callscreen introduces. The causes us a 3 sec delay visibilitychange in current foreground app. Please see the duped bugs for more info.

I also am curious how we allow something like facebook messenger bubbles(https://gigaom2.files.wordpress.com/2013/04/facebook-home-e1365098570477.jpg) to occur on firefox OS:
In current use case, attention screen covers the screen entirely, we don't have someway for attention screen callers to say: hey I just want to cover somewhere in the screen. I propose to refine this also before we open it to the 3rd party app developer.

(In reply to Borja Salguero [:borjasalguero] from comment #8)
> As attention screen is a Gaia concept, I was thinking about how it's working
> and how could we extend it to other communications services as Loop or
> others in the future.
> 
> Currently, for showing an Attention Screen with the appearance of a
> call-screen, being the one with the highest priority, we are checking the
> 'telephony' permission:
> 
> https://github.com/mozilla-b2g/gaia/blob/master/apps/system/js/
> attention_screen.js#L131

This code is old-fashion and does not compatible with new window management.
In new window management I plan to put attention window together with appWindow so we don't need to check the telephony permission to decide 'who should be front'.

> 
> On the other hand, as we are working in Gaia level, we could handle all
> 'voice communications services' in the same way as we are doing currently
> with customizable 'Homescreen', just adding a new role 'voice-service' in
> the Manifest.
> 
> What does it means?
> As the role 'homescreen' or 'lockscreen', this is not tied to any
> permission. It's only a way of telling us explicitely which type of apps you
> are, and telling our Core Apps that you are a candidate for having special
> privileges in the Firefox OS UI.
> 
> For example, telling that you are a 'Homescreen' app, means that you are not
> part of the Apps grid in our Homescreen, and it's part of an specific panel
> in Settings.
> 
> Using the same formula, an app which is defined as 'voice-service' (or
> 'voice-communication', the name is only an example) must have the same level
> of priority (in Gaia level) as the CallScreen App (which should have 'role':
> 'voice-service' as well). This could be the key to unlock other features
> like showing all 'voice-services' within the Contact Detail (imagine that we
> show there the capability of calling directly with Loop or CallScreen, only
> taking into account that they are the same type of role 'voice-service').
> 
> With this solution we can open the model to new apps as Loop, and we should
> define an interaction when a current call is happening, and a new one
> appears. There are several ways of doing it (showing a prompt/banner to
> choose the one we want to use, and hold the one which is not selected for
> example, or keeping the current priority model), but this is a UX proposal
> more than a technical one.
> 
> Wdyt?

All of this doesn't really matter... the codes you mentioned above for telephony is an old design failure.
(In reply to Fernando Jiménez Moreno [:ferjm] from comment #10)
> I believe an alternative to use a role for voice services could be to check
> for the audio-telephony-channel permission if bug 990552 is finally done.

No we don't need...we don't need the permission check anymore if bug attention-window is finished(i.e., move attention screen into appWindow and let appWindow to manage its own attention window.)
(In reply to Vivien Nicolas (:vingtetun) (:21) (NOT reading bugmails, needinfo? please) from comment #7)
> Jaime, can you find someone from your team to help figure out how we should
> handle multiple attention screen at the same time ?
> 
> For example the Call Screen is an attention screen, and so it can be on top
> of any window in the system. Same thing for the Alarm. Applications using
> WebRTC should be able to have such windows.
> 
> The question is how do we deal with a WebRTC attention window, when the call
> screen is already opened ?

Another strange behavior of current attention screen:
When home button is pressed we resized it to 40px from top.
But for facebook messanger bubbles they may not need this behavior. I wonder we need new API to tell the opened and active attention window to do some 'custom closing' but not constraint all attention window to top of the screen. I mean, it's not customizable for now. It's not a friendly API...
(In reply to Vivien Nicolas (:vingtetun) (:21) (NOT reading bugmails, needinfo? please) from comment #4)
> (In reply to Paul Theriault [:pauljt] from comment #3)
> > What happens if one app is using the attention screen, and another asks to
> > use it? Does the latest request replace the content?  Is the dialer, for
> > example, able to handle the case when another app takes over the attention
> > screen?
> 
> There is already the case of others attention screen with the clock app.
> IIRC the call screen have the priority and is always displayed on top of
> other attention screen.
> 
> Others are stacked I think.
> 

IMO we should not stack. Attention window should go with its window opener. It doesn't make sense to compete each other for the most top one... it's not a scoring game, we don't want to introduce some scoring mechanism to let system app decide: all right although you all are telephony-like apps, but according to blahblahblah rules I determines skxxx app is the top, fbxxx is the second.... :)

Some UX improvement might be: have a column in notification area on top of all other notifications to tell the user there are so many apps having attention window, you could click one of them to open it. Otherwise we just relay the new comer on the older one.
Needs info to Rob and Gordon for UX input. 

Rob/ Gordon- for any questions, please ping Alive.
Flags: needinfo?(rmacdonald)
Flags: needinfo?(jachen)
Flags: needinfo?(gbrander)
Flags: in-moztrap?(jsmith)
I reviewed this bug with Gord today and it definitely requires a bit more analysis from UX. We're in a workshop until next week but I was hoping that, Alive, you'd be able to explore your proposal with our UX colleagues in Taipei. I'll NI Mike to see if anyone from his team is available to put together a proposal.

Also, Gord and I generally agree that a scoring system is problematic as it's difficult to assume which alerts users would see as priority. Recency is probably a better model, but, again, we need to do some analysis.

Rob
Flags: needinfo?(rmacdonald)
Flags: needinfo?(mtsai)
Flags: needinfo?(gbrander)
feature-b2g: --- → 2.0
Hi Rob, I will have someone will check with Alive on what's his idea (It's only idea, nothing to review). Let's sync in this week's Framework meeting. Thanks.
Flags: needinfo?(mtsai)
Just to answer the needinfo?

We discussed this issue with Alive face to face in Mountain View last week, and I believed we are on the same line for how to solve the multiple attention screen issue.

One think I'm not sure after a second round if thougts with myself, is that do we really need to couple the attention window to it's opener. Alive, that means that you won't be able to see the UI of the underlying app if there is an attention window on top of it right ?

If yes that sounds bad, it can be cases where you want to use the app even when the attention screen is opened.

As I said the visibility delay is really a platform issue that should be solved by bug 1002430.

I wonder if just basing the attention window display on a recency mechanism + a UI to retrieve it on the notification tray (on top of the list) is not enough for this bug ? The tricky part is UX here but something simple should be good enough for now and will greatly improve the usability of attention screen, while allowing us to not resize window on the fly, which is costly and kills some animations.

Also in order to solve the permission issue we need to have a clean way to couple the permission prompt request to it's opener. Alive have you already opened a bug for that ?
Flags: needinfo?(21) → needinfo?(alive)
So I was assigned a couple actions from last weeks discussion around loop. It was basically to create the hack so that attention-screen can be used by loop as a privileged app (and only loop)
1. change the permission to allow attention to privileged
2. add a check so that certified apps get priority over privileged apps
3. talk to marketplace to make sure only loop gets this permission.

But after reading the above comments I am not sure if I should be pushing ahead with this. I'll try to catch alive today to discuss - I think bottom line is that we need _something_ by the end of next week.
So after making my own patch, I realized the permission change is already attached. Derp. And I don't think we need the certified app priority since we already give priority to apps with the telephony permission [1]. So assuming that we can get marketplace to ensure that only loop gets the permission then I think we have hacky way of exposing attention-screen to loop. 

But I would still like to see UX and developer input to continue this discussion from comments 16-18. It would be much preferable to solve this "properly" than exposing attention-screen in its current state I think. 

[1] https://github.com/mozilla-b2g/gaia/blob/master/apps/system/js/attention_screen.js#L131
Mike, Harly and I have synced with Alive, and we come out some rules of Attention Window for further discussion.

1) An app can show an Attention Window.
2) Another app can show an Attention Window and overlay on the previous one (new one overlays the old one)
3) Each Attention Window has a corresponding item (Ongoing Event) in Notification Tray (we should design a new area at top of Notification Tray for the ongoing events)
4) On status bar, we should design a new icon for ongoing event number next to notification number. (Maybe the status bar could have another background color if there are ongoing events.) In the following example, the status bar indicates that there are 2 ongoing events (⊛) and 5 notifications (⊜).
   [⊛2 ⊜5          ◢ ▬ 2:30PM]    
5) Tapping an Ongoing Event, it switches to the corresponding Attention Window.
6) An app can decide when to remove its Attention Window and Ongoing Event. (Attention Window and Ongoing Event should be removed together.)
(In reply to Vivien Nicolas (:vingtetun) (:21) (NOT reading bugmails, needinfo? please) from comment #18)
> One think I'm not sure after a second round if thougts with myself, is that
> do we really need to couple the attention window to it's opener. Alive, that
> means that you won't be able to see the UI of the underlying app if there is
> an attention window on top of it right ?

If the 'couple' means "open app means open the attention window": I don't think so, it's an old hack. I think app might want to close attention window when app is launched since we think attention window is a special notification and notification disappears when app is launched but it depends and app should close the attention window on its own on demand.

> 
> If yes that sounds bad, it can be cases where you want to use the app even
> when the attention screen is opened.

I think so, if the app want the opened attention window to be opened again when app is launching, it should window.open again..or use some other API.

> 
> As I said the visibility delay is really a platform issue that should be
> solved by bug 1002430.

\O/

> I wonder if just basing the attention window display on a recency mechanism
> + a UI to retrieve it on the notification tray (on top of the list) is not
> enough for this bug ? The tricky part is UX here but something simple should
> be good enough for now and will greatly improve the usability of attention
> screen, while allowing us to not resize window on the fly, which is costly
> and kills some animations.

We need to represent 'there is an attention window' in statusbar or somewhere. instead of using the ' attention bar'...

> 
> Also in order to solve the permission issue we need to have a clean way to
> couple the permission prompt request to it's opener. Alive have you already
> opened a bug for that ?

Maybe I am missing something, but are we going to allow app to request attention window in runtime? Does window.open(..., 'attention') triggers the permission prompt? Do we need some platform work in permission manager? Is this needed to be remembered?
(In reply to Vivien Nicolas (:vingtetun) (:21) (NOT reading bugmails, needinfo? please) from comment #18)
> 
> Also in order to solve the permission issue we need to have a clean way to
> couple the permission prompt request to it's opener. Alive have you already
> opened a bug for that ?

BTW there's an old bug for binding permission request to the requester
gaia https://bugzilla.mozilla.org/show_bug.cgi?id=853711
gecko https://bugzilla.mozilla.org/show_bug.cgi?id=852013
Flags: needinfo?(alive)
After talking to many of you offline, I am not sure if all of us are on the same page about waht should be implemented for 2.0.

Vivien, is the solution you described in Comment 18 targetted for 2,0 or is it a longer term approach?
Flags: needinfo?(21)
Whiteboard: ft:loop
Although I already sent an e-mail to some of the people involved in the bug, I think it's better to comment it here in the bug.

* So it seems that the current planned solution is:

1/ Expose Attention Screen (AS) permission to any privileged app
2/ Continue with the current conflict policy, apps with telephony API permission prevail over the rest:

     - If a Loop call is ongoing (with an AS) and a regular call is received, the regular call AS will be shown on top of the Loop AS.
     - PROBLEM: If a regular call is ongoing (with an AS) and a Loop all is received, the loop call AS won’t be shown.

* And we all agree that the ideal solution was:

1/ Expose Attention Screen (AS) permission to any privileged app
2/ Implement what is described in https://bugzilla.mozilla.org/show_bug.cgi?id=988212#c21 and give user full control of AS.

After talking to Vivien last week we had the impression that this was going to be implemented for 2.0. Based on Paul and Alive comments I am not sure about that anymore so maybe this is something to be done for 2.1

* In order to mitigate the problem describe with the currently planned solution we were thinking in another solution:

1/ Expose Attention Screen (AS) permission only to Loop in 2.0 (hopefully, when the ideal solution for handling AS is implemented it can be opened to any Privileged App)
2/ Change the current conflict policy for AS to use the telephony-channel permission instead of the telephony API.

     - If a Loop call is ongoing (with an AS) and a regular call is received, the regular call AS will be shown on top of the Loop AS.
     - If a regular call is ongoing (with an AS) and a Loop all is received, the loop call AS will be shown on top of the Regular Call AS.

     As incoming call screens always have the option to reject call, users would always an easy way to get rid of new AS if they don’t want to be disturbed.

What do you think?

Please note that this discussion is related to Bug 1016277 - "If two different apps try to use the 'telephony' channel at the same time both apps can play audio" and a joint solution should be offered.
(In reply to Maria Angeles Oteo (:oteo) from comment #24)
> After talking to many of you offline, I am not sure if all of us are on the
> same page about waht should be implemented for 2.0.
> 
> Vivien, is the solution you described in Comment 18 targetted for 2,0 or is
> it a longer term approach?
 
That's what I was hoping for. It seems like the UX work will takes a bit more time to  implement than expected. That said I will try to make a simple fix for that this week, and use your fallback plan if I fail.
Flags: needinfo?(21)
(In reply to Vivien Nicolas (:vingtetun) (:21) (NOT reading bugmails, needinfo? please) from comment #26)
  
> That's what I was hoping for. It seems like the UX work will takes a bit
> more time to  implement than expected. That said I will try to make a simple
> fix for that this week, and use your fallback plan if I fail.

Thanks a lot for your answer, Vivien.

To summarize, we will aim for the solution in comment 18 and if there is no time the following back-up solution will be used:

1/ Expose Attention Screen (AS) permission only to Loop in 2.0 (hopefully, when the ideal solution for handling AS is implemented it can be opened to any Privileged App)
2/ Change the current conflict policy for AS to use the telephony-channel permission instead of the telephony API.

Please, keep us posted about any progress or roadblocked as soon as you can. 
Of course, let us know if you need any help.
Depends on: 1019007
Attached patch multiple.attention.screens.patch (obsolete) — — Splinter Review 
This patch allow multiple attention screens to open at the same time. The last one to be open is the displayed on top of the others.

Then you can re-open your attention screen by re-opening the app, or by clicking the permanent notification for attention screen that are not displayed in your notification tray.

The permanent notification probably needs some love to make them prettier but also to show an indicator in the status bar for those. So it is easy for the user to see that something happens.

There is no clear answer to the competing sound stuff for 2 apps that use the telephony channel. So the call screen listen for the visibility change event, and if it goes into background while the screen is turned on, then it means that an other attention screen has took the precedence and the current calls are put on hold.

This is not the best UX in the world, but since the sound needs to be muted when the attention screen of loop comes in thats pretty similar.

Also loop can implement the same kind of heuristic in order to hold its call while a new phone call is received.

Sadly the way to read if the screen is enabled is certified only now (it uses to be accessible for other apps IIRC), but we can just make this property accessible for privileged app if needed and if Jonas/Ehsan are fine with it.

What do you think ?
Flags: needinfo?(oteo)
Moving to the appropriate component,
Component: General → DOM: Device Interfaces
Product: Firefox OS → Core
Attached patch multiple.attention.screens.patch (obsolete) — — Splinter Review 
Since I'm touching the attention screen, let me know what you think.
Attachment #8433394 - Flags: feedback?(etienne)
Vivien: Before we get into the sound stuff, I'd like to understand better what your patch does. It sounds like this basically implements the solution described in comment 21. Is that correct? I.e. we'll have the persistent-notification-in-notification-tray and all?
Howie, can you take a look at whether this feature work would move to work for devices under v2.0?
Assignee: nobody → hochang
(In reply to Jonas Sicking (:sicking) from comment #32)
> Vivien: Before we get into the sound stuff, I'd like to understand better
> what your patch does. It sounds like this basically implements the solution
> described in comment 21. Is that correct? I.e. we'll have the
> persistent-notification-in-notification-tray and all?

With this patch you can open multiple attention screens, and the last one is displayed on top of the previous one.

If you close the current attention screen, then the previous one is displayed.

If you are on the lockscreen this is the only way to switch between 2 attention screens. That's not the best UX fwiw. I can add permanent notifications on the lockscreen if needed.

If you are not on the lockscreen, you can open the notification tray, and there is a permanent notification for all the attention screens that are minimized. If you click on one, then the related attention screen becomes the current displayed one, and the notifications are updated.

If you are not on the lockscreen, and you have minimize the attention screen by clicking on the home button. Since the attention screen is minimized on top of the statusbar you have first to click the minizied version to expand the attention screen in order to access the notification tray. Not the best UX again but I was a bit afraid to remove minimized attention screen at this stage.
In the future we need to come up with a better UX, and ideally I would like to avoid the minimized attention screen as it resize windows and affect somes of the animations.

It would also be nice to have a kind of new icon in the statusbar when an attention screen is minimized.

That's more or less the bits for the attention screen.


For the sound, if the platform fix is too expensive I was thinking of a hack where the callscreen put the call on hold when a loop attention screen pops up. That's not the best UX as the call will be put on hold as soon as the attention screen pops up, so when the loop app will ring. In order to let loop do the same thing it required us to expose navigator.mozPower.screenEnabled to privileged app (read-only).

But that's a bit out of the scope of this bug.
Comment on attachment 8433394 [details] [diff] [review]
multiple.attention.screens.patch

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

Yeah I think this is the best approach for 2.0. 
But the lack of tests for the attention screen is starting to make me really uncomfortable (so I guess I'm implicitly volunteering ;)).

Maybe this was already answered in the comments, but how do we plan to differentiate a visibilitychange{hidden: true} triggered by another attention screen from one triggered when the screen is turned off?

::: apps/system/js/attention_screen.js
@@ +243,2 @@
>  
>        this.dispatchEvent('attentionscreenclose', { origin: origin });

not part of the patch, but it's weird to have this inside the condition, and I think nobody's listening for it.

@@ +280,5 @@
> +                  );
> +
> +    // Let's create the fake notification.
> +    var notification = document.createElement('div');
> +    notification.id = manifestURL;

We will do something better than that in the final version of the patch right? :)

@@ +289,5 @@
> +    icon.classList.add('icon');
> +    notification.appendChild(icon);
> +
> +    var message = document.createElement('div');
> +    message.appendChild(document.createTextNode(manifest.name));

note: we're probably going to need a late l10n string for this

@@ +299,5 @@
> +
> +    // Attach an event listener to the fake notification so the
> +    // attention screen is shown when the user clicks on it.
> +    notification.addEventListener('click',
> +                                  this._showFromNotification.bind(this));

can't we bind a frame parameter directly?

::: apps/system/js/dialer_agent.js
@@ +217,5 @@
> +  DialerAgent.prototype.showCallScreen = function da_showCallScreen() {
> +    if (this._callScreen) {
> +      AttentionScreen.show(this._callScreen);
> +    }
> +  };

we should add a tiny test for this
Attachment #8433394 - Flags: feedback?(etienne) → feedback+
(In reply to Etienne Segonzac (:etienne) from comment #35)
> Comment on attachment 8433394 [details] [diff] [review]
> multiple.attention.screens.patch
> 
> Review of attachment 8433394 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Yeah I think this is the best approach for 2.0. 
> But the lack of tests for the attention screen is starting to make me really
> uncomfortable (so I guess I'm implicitly volunteering ;)).
> 
> Maybe this was already answered in the comments, but how do we plan to
> differentiate a visibilitychange{hidden: true} triggered by another
> attention screen from one triggered when the screen is turned off?
> 

Not sure I'm going to land this stuff here. The idea was to rely on navigator.mozPower.screenEnabled. The visibility of the attention screen can be turned off when the screen is turned off and that should not hold the call. But if the screen is turned on, then it means that it has been setVisible(false) because of an other attention screen.

This is a bit flaky, but if we don't have any other solution for the sound, it may be OK for now.

> ::: apps/system/js/attention_screen.js
> @@ +243,2 @@
> >  
> >        this.dispatchEvent('attentionscreenclose', { origin: origin });
> 
> not part of the patch, but it's weird to have this inside the condition, and
> I think nobody's listening for it.
> 

I'm can open a followup to remove it. I just don't want to take any extra risk for 2.0 :)

> @@ +280,5 @@
> > +                  );
> > +
> > +    // Let's create the fake notification.
> > +    var notification = document.createElement('div');
> > +    notification.id = manifestURL;
> 
> We will do something better than that in the final version of the patch
> right? :)
> 

Well, it's a bit tricky as it can be multiple fake notifications (I developed with 3 attention screens) so I can't inline it in the DOM :(

Do you have an other idea ?

> @@ +289,5 @@
> > +    icon.classList.add('icon');
> > +    notification.appendChild(icon);
> > +
> > +    var message = document.createElement('div');
> > +    message.appendChild(document.createTextNode(manifest.name));
> 
> note: we're probably going to need a late l10n string for this

I'm pretty sure that the name is already localized since this is what is displayed on the homescreen. IIRC the |manifest| version returns by application is the current localized one.

> @@ +299,5 @@
> > +
> > +    // Attach an event listener to the fake notification so the
> > +    // attention screen is shown when the user clicks on it.
> > +    notification.addEventListener('click',
> > +                                  this._showFromNotification.bind(this));
> 
> can't we bind a frame parameter directly?
> 

Agreed it will be cleaner. Will do.

> ::: apps/system/js/dialer_agent.js
> @@ +217,5 @@
> > +  DialerAgent.prototype.showCallScreen = function da_showCallScreen() {
> > +    if (this._callScreen) {
> > +      AttentionScreen.show(this._callScreen);
> > +    }
> > +  };
> 
> we should add a tiny test for this

Sounds good.
Flags: needinfo?(oteo)
Attached patch multiple.attention.screens.patch (obsolete) — — Splinter Review 
Etienne, I still need to add a test for the DialerAgent part. In the meantime I don't think this prevent this part to be reviewed properly.
Attachment #8433264 - Attachment is obsolete: true
Attachment #8433394 - Attachment is obsolete: true
Attachment #8434863 - Flags: review?(etienne)
Attached patch multiple.attention.screens.patch (obsolete) — — Splinter Review 

        Attachment #8434863 -
        Attachment is obsolete: true

        Attachment #8434863 -
        Flags: review?(etienne)

        Attachment #8434892 -
        Flags: review?(etienne)

    
    

    
  
Comment on attachment 8434892 [details] [diff] [review]
multiple.attention.screens.patch

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

I'm confident you'll find a solution for the callscreen minimization issue, and I'll be ready for the final review round in 8 hours :)

Apart from that it's working very well, definitely deserves the "glorious hack" appellation.

::: apps/system/js/attention_screen.js
@@ +275,5 @@
> +
> +    var manifest = window.applications.getByManifestURL(manifestURL).manifest;
> +    var iconSrc = manifestURL.replace(
> +                    '/manifest.webapp',
> +                    manifest.icons[Object.keys(manifest.icons)[0]]

we should definitely support entry points here since the Dialer is part of the use case from day 1 :)

@@ +425,5 @@
> +      var frameOrigin = frame.dataset.frameOrigin;
> +      if (frameOrigin === origin ||
> +          (frameOrigin.startsWith('app://callscreen.gaiamobile.org/') &&
> +           origin.startsWith('app://communications.gaiamobile.org/dialer/'))) {
> +        this.show(frame);

Hum... we have an issue.
In the clock app, when the clock attention screen opens, pressing home will minimize the attention screen and show the homescreen (expected).

*But*

In the dialer, when the call screen opens, pressing home minimize the attention screen and then  maximize it back to be fully visible instantly.

I think it's because the communications app becomes active once the call screen goes away triggering a showForOrigin, but the clock always has a visible window.

::: apps/system/js/screen_manager.js
@@ +269,5 @@
>          // call setup, the user will be notified by a tone.
>          if (this._cpuWakeLock) {
>            // In case of user making an extra call, the attention screen
>            // may be hidden at top so we need to confirm it's shown again.
> +          DialerAgent.showCallScreen();

lowercase |dialerAgent.showCallScreen()|

(DialerAgent is the constructor)

::: apps/system/test/unit/dialer_agent_test.js
@@ +472,5 @@
>  
>      navigator.mozTelephony = MockNavigatorMozTelephony;
>    });
> +
> +  test('AttentionScreen should be showned if asked', function() {

nit: not sure, I think it's "should be shown when asked"

@@ +478,5 @@
> +    subject.start();
> +
> +    var attentionSpy = this.sinon.spy(MockAttentionScreen, 'show');
> +    subject.showCallScreen();
> +    assert.isTrue(MockAttentionScreen.show.calledOnce);

nit: you can even
```
    sinon.assert.calledWithMatch(MockAttentionScreen.show, {
      detail: {
        features: 'attention',
      }
    });

```

::: apps/system/test/unit/screen_manager_test.js
@@ +287,4 @@
>          ScreenManager._cpuWakeLock = stubCpuWakeLock;
>  
>          switchProperty(navigator, 'mozTelephony', stubTelephony, reals);
> +        switchProperty(window, 'DialerAgent', stubDialerAgent, reals);

|dialerAgent|

@@ +291,5 @@
>        });
>  
>        teardown(function() {
>          restoreProperty(navigator, 'mozTelephony', reals);
> +        restoreProperty(window, 'DialerAgent', reals);

|dialerAgent|

        Attachment #8434892 -
        Flags: review?(etienne)

    
    

    
  

      
      
      
      

        Attached patch
          
          
        multiple.attention.screens.patch (obsolete)
        — 
          — Splinter Review
      

    


  
Pretty sure the call screen minimization issue is a race with 2 events competing. The 'home' event that triggers 'hide', and the 'foreground' event trigger as a side effect of the screenmanager when it receive 'status-active' since event are synchronous.

I can only reproduce it if I have no lockscreen, open the dialer, call my voice mail, wait for the attention screen, turn off the screen, turn on the screen, hit home.

So in order to solve that I moved the status-active event into a setTimeout(..., 0).
So those 2 events does not compete anymore and are dispatched one after the other. So the 'home' events hide the minimize the attention screen, and hide the app correctly, and then the status-active event is dispatched and trigger whatever it needs to trigger.


For the entry points, as discussed on IRC, I have included the dialer icons.

        Attachment #8434892 -
        Attachment is obsolete: true

        Attachment #8435076 -
        Flags: review?(etienne)

    
    

    
  


  
Oups. I forgot to |git add -u| the setTimeout(..., 0);

        Attachment #8435076 -
        Attachment is obsolete: true

        Attachment #8435076 -
        Flags: review?(etienne)

        Attachment #8435079 -
        Flags: review?(etienne)

    
    

    
  
Comment on attachment 8396982 [details] [diff] [review]
bug988212.patch

Redirecting this r? to sicking as Fabrice is in vacation, and this is mostly a stamp to see if that's fine or not to do that.

        Attachment #8396982 -
        Flags: review?(fabrice) → review?(jonas)

    
    

    
  
Comment on attachment 8435079 [details] [diff] [review]
multiple.attention.screens.patch

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

Good to go!

        Attachment #8435079 -
        Flags: review?(etienne) → review+

    
    

    
  
(In reply to Etienne Segonzac (:etienne) from comment #43)
> Comment on attachment 8435079 [details] [diff] [review]
> multiple.attention.screens.patch
> 
> Review of attachment 8435079 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Good to go!

Landed the Gaia bits.

https://github.com/mozilla-b2g/gaia/commit/487af01e7a373b77da231dcf5e96c59efa5f1904
Assignee: hochang → 21
Status: UNCONFIRMED → ASSIGNED
Ever confirmed: true

    
    

    
  
https://hg.mozilla.org/mozilla-central/rev/be9fedef6271
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla32
Depends on: 1023794
Depends on: 1026167
Depends on: 1023794

    
    

    
  
Comment on attachment 8435079 [details] [diff] [review]
multiple.attention.screens.patch

This patch should have been flagged for ui-review?, especially given it's possible affect on alarm behavior in bug #1026167.

        Attachment #8435079 -
        Flags: ui-review?(rmacdonald)
Flags: in-moztrap?(jsmith)

          You need to log in
          before you can comment on or make changes to this bug.
        






  

    
  







  

    

      
Attachment

      

      
      
      
      
    

    

      

        

          

            
General

            

              Creator: 



            

            
Created: 

            
Updated: 

            
Size: