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

RESOLVED WONTFIX

Status

()

Core
Networking
RESOLVED WONTFIX
6 years ago
2 years ago

People

(Reporter: sinker, Unassigned)

Tracking

Trunk
x86_64
Gonk (Firefox OS)
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 1 obsolete attachment)

(Reporter)

Description

6 years ago
Watch on network manager for new network connections, and wakeup push-notification service to retrieve messages from notification server.
(Reporter)

Updated

6 years ago
Depends on: 763198
Assignee: nobody → slin

Comment 1

6 years ago
Created attachment 662792 [details] [diff] [review]
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.
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?

Comment 3

6 years ago
Created attachment 663950 [details] [diff] [review]
V2: Revised from 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)
(Reporter)

Comment 4

6 years ago
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.
(Reporter)

Comment 5

6 years ago
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-

Updated

4 years ago
Assignee: slin → nobody
Status: NEW → RESOLVED
Last Resolved: 2 years ago
Resolution: --- → WONTFIX
You need to log in before you can comment on or make changes to this bug.