Closed Bug 786841 Opened 11 years ago Closed 8 years ago

[B2G] wakeup push-notification while a network interface is available


(Core :: Networking, defect)

Gonk (Firefox OS)
Not set





(Reporter: sinker, Unassigned)




(1 file, 1 obsolete file)

Watch on network manager for new network connections, and wakeup push-notification service to retrieve messages from notification server.
Depends on: 763198
Assignee: nobody → slin
Add an observer to observe the status changed of network connection, if changed from offline to online, re-register the UA for push notification service.
Attachment #662792 - Flags: feedback?(cyu)
Comment on attachment 662792 [details] [diff] [review]
V1: Add an oberserver to moniter the network status

Review of attachment 662792 [details] [diff] [review]:

::: dom/src/notification/nsDOMPushManager.cpp
@@ +414,5 @@
>    mWindow(do_GetWeakReference(aWindow)) {
> +  // Setup an observer to watch the status change of network
> +  nsCOMPtr<nsIObserverService> observerService = services::GetObserverService();
> +  if (!observerService) {
> +    //Error check here

Just bail out here, or you will crash in the following line.

@@ +419,5 @@
> +  }
> +  nsresult rv;
> +  rv = observerService->AddObserver(this, NETWORK_OFFLINE_STATUS_CHANGED, false);
> +  if (NS_FAILED(rv)) {
> +    //Error check here

There is little to do if AddObserver() returns failure here. I think output some warning or just don't check the return value is OK here.

@@ +714,5 @@
> +                          const char *aTopic,
> +                          const PRUnichar *aData) {
> +  if (strcmp(aTopic, NETWORK_OFFLINE_STATUS_CHANGED) != 0) {
> +    return NS_OK;
> +  }

The obvious bug here is you didn't check whether you observed the change from offline to online or vice versa.
Attachment #662792 - Flags: feedback?(tlee)
Attachment #662792 - Flags: feedback?(cyu)
Attachment #662792 - Flags: feedback?
Coding style and error checking have revised base on the feedback, thanks :)
Attachment #662792 - Attachment is obsolete: true
Attachment #662792 - Flags: feedback?(tlee)
Attachment #662792 - Flags: feedback?
Attachment #663950 - Flags: review?(tlee)
Shelly, I think it is totally wrong to add the observer on DOMPushManager.  DOMPushManager is created for each instance of content process, that means you will add event listener for N times if there are N content process.  That means the push notification service will be wake-up for N times for every time of reconnecting to the network.  So, please add a new component for your code.
Comment on attachment 663950 [details] [diff] [review]
V2: Revised from feedback.

Review of attachment 663950 [details] [diff] [review]:

Just as I had mentioned above.
Attachment #663950 - Flags: review?(tlee) → review-
Assignee: slin → nobody
Closed: 8 years ago
Resolution: --- → WONTFIX
You need to log in before you can comment on or make changes to this bug.