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

Status: RESOLVED WONTFIX

(Core :: Networking, defect)

Description

[Thinker Li [:sinker]]

Watch on network manager for new network connections, and wakeup push-notification service to retrieve messages from notification server.

Comment 1

[Shelly Lin [:shelly]]

Attachment: V1: Add an oberserver to moniter the network status

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.

Comment 2

[Cervantes Yu [:cyu] [:cervantes]]

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.

Comment 3

[Shelly Lin [:shelly]]

Attachment: V2: Revised from feedback.

Coding style and error checking have revised base on the feedback, thanks :)

Comment 4

[Thinker Li [:sinker]]

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 5

[Thinker Li [:sinker]]

Comment on attachment 663950 [details] [diff] [review]
V2: Revised from feedback.

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

Just as I had mentioned above.