Closed Bug 899596 Opened 9 years ago Closed 4 years ago

[Wifi] get current wifi connection speed


(Firefox OS Graveyard :: General, defect)

Gonk (Firefox OS)
Not set


(Not tracked)



(Reporter: glai, Assigned: bochen)




(1 file, 4 obsolete files)

Create an interface for getting wifi connection speed currently.
Duplicate of this bug: 899595
Attached patch get WiFi link speed (obsolete) — Splinter Review
Attachment #787932 - Flags: review?(vchang)
Comment on attachment 787932 [details] [diff] [review]
get WiFi link speed

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

::: dom/wifi/WifiWorker.js
@@ +2816,5 @@
>      }).bind(this));
>    },
> +  getLinkSpeed: function() {
> +    return this._lastConnectionInfo.linkSpeed;

this._lastConnectionInfo could be null right, please add some sanity check here.

::: dom/wifi/nsIWifi.idl
@@ +59,5 @@
>      void getWifiScanResults(in nsIWifiScanResultsReady callback);
> +
> +    /**
> +     * Returns WiFi link speed from this._lastConnectionInfo.linkSpeed.
> +     */

Return Wifi connected speed in Mbit/s
Attachment #787932 - Flags: review?(vchang)
Assignee: nobody → glai
Assignee: glai → gavin09
Attachment #787932 - Attachment is obsolete: true
Attached patch (wip)get WiFi link speed (obsolete) — Splinter Review
Blocks: 854206
Hi Vincent, Gavin:

   I wonder whether we could send an observer notification (containing linkSpeed) to PowerStatsService.jsm whenever WifiWorker fires a connectionInfoUpdate event instead of creating a new method on the WifiWorker for querying linkSpeed.

   I check WifiWorker.js and it shows that the connectionInfoUpdate event is fired every 5 seconds. But, if the linkSpeed is not changed (as well as the change of RSSI is less than 10%), the event would not be fired. That means the linkSpeed may not be changed in some circumstance such like users leave his/her phone on the table. So, I think that it could be more efficient if we use notification to update a linkSpeed variable record inside PowerStatsService instead of querying the linkSpeed everytime when we want to calculate WiFi power consumption.

   Any suggestion about this?
Flags: needinfo?(vchang)
Flags: needinfo?(gavin09)
Hi Borting: 

  I think observer notification would be more efficient. The only concern I have is PowerStatsService does not have the link speed value and PowerStatsService does not receive the notification. Is there any possibilities that PowerStatsService does not have link speed value? For example, after calling the clear data function or PowerStatsService is just initialized. If it would have the chance PowerStatsService does not have the link speed value, I suggest the querying method could be remained as a secondary method. The observer notification is the primary method.
Flags: needinfo?(gavin09)
Hi Gavin:

  Your concern is right. I double checked the WiFiWorker.js and I found when we complete the connection with AP, the WiFiWorker will call getConnectionInformation() [1] which fires the first connectionInfoUpdate event. So, if we add the notification code befire firing event (around here [2]), I think PowerStatsService would obtain the linkSpeed before fxos receives traffic from AP.

  Is there any situation we cannot receive a linkSpeed update in advance?

Attached patch Obtain WiFi Link Speed (obsolete) — Splinter Review
Hi Vincent:

  Could you help review this patch? Thx a lot.

Update Notes:
1. Send out a notification containing linkSpeed before firing a connectionInfoUpdate event.
Assignee: gavin09 → bochen
Attachment #798889 - Attachment is obsolete: true
Attachment #8343644 - Flags: review?(vchang)
Flags: needinfo?(vchang)
Comment on attachment 8343644 [details] [diff] [review]
Obtain WiFi Link Speed

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

Using the wifi API seems more straightforward to me. I would prefer to use the original patch to get link
speed directly.
Attachment #8343644 - Flags: review?(vchang) → review-
Hi Vincent:

I've changed the getLinkSpeed method to the original one. Please help review this patch. Thanks!
Attachment #8343644 - Attachment is obsolete: true
Attachment #8349821 - Flags: review?(vchang)
Comment on attachment 8349821 [details] [diff] [review]

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

::: dom/wifi/WifiWorker.js
@@ +2484,5 @@
>        this._sendMessage(message, true, networks, msg);
>      }).bind(this));
>    },
> +  getLinkSpeed: function() {

Nit: We like to early return.  

if (!this._lastConnectionInfo.linkSpeed)
  return 0;

return this._lastConnectionInfo.linkSpeed;
Attachment #8349821 - Flags: review?(vchang)
fix nit.

Hi Vincent, can you help review this patch? Thanks a lot.
Attachment #8349821 - Attachment is obsolete: true
Attachment #8353900 - Flags: review?(vchang)
Comment on attachment 8353900 [details] [diff] [review]

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

Looks good. Thank you.
Attachment #8353900 - Flags: review?(vchang) → review+
Firefox OS is not being worked on
Closed: 4 years ago
Resolution: --- → WONTFIX
You need to log in before you can comment on or make changes to this bug.