Closed Bug 787172 Opened 12 years ago Closed 12 years ago

B2G RIL: Wait for the system app to load before turning on the radio

Categories

(Core :: DOM: Device Interfaces, defect)

Product:
Core
Component:
DOM: Device Interfaces
Platform:
ARM
Gonk (Firefox OS)
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla18
Project Flags:
blocking-basecamp +

People

(Reporter: philikon, Assigned: philikon)

Details

Attachments

(1 file)

v1
4.94 KB, patch
vingtetun
: review+
Details | Diff | Splinter Review 
As soon as the RIL starts and retrieves the ril.radio.enabled setting from the SettingsDB, we turn on the radio. We could then very quickly receive text messages, STK infos, etc. that would all go into the ether if Gaia is slow at starting up. With switching to system messages for the notification, we only need the system app to be ready, but that doesn't change the fact that the set up is still race-y.

After talking to Vivien and Fabrice, the best solution for now is probably to have the system app send a mozContent event to indicate that it's loaded. shell.js can then convert this to an observer notification that RadioInterfaceLayer can observe and use to turn on the radio, etc.
Assignee: nobody → philipp
Attached patch v1 — — Splinter Review 
Gaia pull request (which will be necessary for this to not regress) is coming shortly...
Attachment #657004 - Flags: review?(21)
Couldn't we buffer these in gecko too?  It would be nice to have the radio coming up in parallel to UI startup.
(In reply to Chris Jones [:cjones] [:warhammer] from comment #3)
> Couldn't we buffer these in gecko too?  It would be nice to have the radio
> coming up in parallel to UI startup.

I suppose we could. But the radio comes on really quickly. As a human I can't tell a difference at all. By the time I'm shown the initial lock screen, the radio is already on and registered with the network. So this is really about ordering things to prevent races, which seems simpler to me than buffering.
Blocking, fewer races is a good thing!
blocking-basecamp: ? → +
Comment on attachment 657004 [details] [diff] [review]
v1

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

But I think the name should be more generic. (I don't have any good suggestions though)

::: b2g/chrome/content/shell.js
@@ +480,5 @@
>        case 'select-choicechange':
>          FormsHelper.handleEvent(detail);
>          break;
> +      case 'system-app-ready':
> +        Services.obs.notifyObservers(null, 'system-app-ready', null);

Maybe you want a more generic-name than system-app-ready?
Attachment #657004 - Flags: review?(21) → review+
(In reply to Vivien Nicolas (:vingtetun) from comment #6)
> > +      case 'system-app-ready':
> > +        Services.obs.notifyObservers(null, 'system-app-ready', null);
> 
> Maybe you want a more generic-name than system-app-ready?

Hmm, or maybe be more specific about what we're ready for: listening to system messages. So maybe:

  'system-message-listener-ready'

?
https://hg.mozilla.org/mozilla-central/rev/bf1fa7a6cd1b
Status: NEW → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
This patch blocks all RIL related test cases, waiting for signals from GAIA.
(In reply to Vicamo Yang [:vicamo][:vyang] from comment #10)
> This patch blocks all RIL related test cases, waiting for signals from GAIA.

Well, shoot. Ok, time to back out.
(In reply to Philipp von Weitershausen [:philikon] from comment #11)
> (In reply to Vicamo Yang [:vicamo][:vyang] from comment #10)
> > This patch blocks all RIL related test cases, waiting for signals from GAIA.
> 
> Well, shoot. Ok, time to back out.

Ok, I can't reproduce this. Vicamo, can you please file a bug so we can discuss your problem there? Thanks!
(In reply to Philipp von Weitershausen [:philikon] from comment #12)
> (In reply to Philipp von Weitershausen [:philikon] from comment #11)
> > (In reply to Vicamo Yang [:vicamo][:vyang] from comment #10)
> > > This patch blocks all RIL related test cases, waiting for signals from GAIA.
> > 
> > Well, shoot. Ok, time to back out.
> 
> Ok, I can't reproduce this. Vicamo, can you please file a bug so we can
> discuss your problem there? Thanks!

Sorry, it seems to be a false alarm. With Gaia upgraded, marionette tests no longer get blocked.
Hi guys!

Actually, we've already had Bug 783149 (System Message API - Buffer messages in shell.js until the system app has started), which is aimed to buffer the system messages for 10 seconds starting from the first content loading.

I'm wondering why we still need this? I guess this one should automatically be fixed even if we don't need the extra 'system-message-listener-ready' mechanism (noticiably, the Bug 783149 was checked in on 8/31 and this bug was reported on 8/30 ;)).

Listening to the 'system-message-listener-ready' event could be unstable because the shell.js has not yet been ready to catch that event before it's coming. Eventually, the radio would have no chance anymore to send system messages because that event has been missing.
(In reply to Gene Lian [:gene] from comment #14)
> Actually, we've already had Bug 783149 (System Message API - Buffer messages
> in shell.js until the system app has started), which is aimed to buffer the
> system messages for 10 seconds starting from the first content loading.
> 
> I'm wondering why we still need this? I guess this one should automatically
> be fixed even if we don't need the extra 'system-message-listener-ready'
> mechanism (noticiably, the Bug 783149 was checked in on 8/31 and this bug
> was reported on 8/30 ;)).

We could certainly do the buffering. Having Gaia and Gecko notify each other just seemed a bit cleaner to me. I would defer the decision to Fabrice and Vivien.

> Listening to the 'system-message-listener-ready' event could be unstable
> because the shell.js has not yet been ready to catch that event before it's
> coming. Eventually, the radio would have no chance anymore to send system
> messages because that event has been missing.

I'm not sure I understand your concern here. Please file a new bug for this.
(In reply to Philipp von Weitershausen [:philikon] from comment #15)
> > Listening to the 'system-message-listener-ready' event could be unstable
> > because the shell.js has not yet been ready to catch that event before it's
> > coming. Eventually, the radio would have no chance anymore to send system
> > messages because that event has been missing.
> 
> I'm not sure I understand your concern here. Please file a new bug for this.

Are you perhaps referring to the issue that's discussed in bug 790906?
(In reply to Philipp von Weitershausen [:philikon] from comment #16)
> (In reply to Philipp von Weitershausen [:philikon] from comment #15)
> > > Listening to the 'system-message-listener-ready' event could be unstable
> > > because the shell.js has not yet been ready to catch that event before it's
> > > coming. Eventually, the radio would have no chance anymore to send system
> > > messages because that event has been missing.
> > 
> > I'm not sure I understand your concern here. Please file a new bug for this.
> 
> Are you perhaps referring to the issue that's discussed in bug 790906?

Yes! That's it. ;) (ha sorry again we're discussing in a closed issue thread)
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: