Closed Bug 1074092 Opened 10 years ago Closed 10 years ago

[CBS] Move CBS Settings implementation from RadioInterfaceLayer to CellBroadcastService.

Categories

(Firefox OS Graveyard :: RIL, defect)

x86_64
Linux
defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED
2.1 S6 (10oct)

People

(Reporter: bevis, Assigned: bevis)

References

Details

Attachments

(1 file, 1 obsolete file)

      No description provided.
This patch is to move the CBS Settings related implementation from RadioInterfaceLayer to CellBroadcastService.js.

Hi Hsinyi,

May I have your review for this change?

Thanks!
Attachment #8500898 - Flags: review?(htsai)
Comment on attachment 8500898 [details] [diff] [review]
Patch v1: Move CBS Settings implementation from RadioInterfaceLayer to CellBroadcastService.

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

Looks good to me, thank you.

::: dom/cellbroadcast/gonk/CellBroadcastService.js
@@ +134,5 @@
> +    return Array.isArray(aSettings) ? aSettings[aClientId] : aSettings;
> +  },
> +
> +  /**
> +   * Helper function to set CellBroadcastDisabled to Each RadioInterface.

nit: s/Each/each/

@@ +148,5 @@
> +    }
> +  },
> +
> +  /**
> +   * Helper function to set CellBroadcastSearchList to Each RadioInterface.

ditto.

@@ +154,5 @@
> +  setCellBroadcastSearchList: function(aSettings) {
> +    let numOfRilClients = gRadioInterfaceLayer.numRadioInterfaces;
> +    let responses = [];
> +    for (let clientId = 0; clientId < numOfRilClients; clientId++) {
> +      let newSearchList = this._getSettingValueByClientId(clientId, aSettings);

nit: I'd suggest the name "_retrieveSettingValueByClient" because "_get" confuses me a little. I can't stop thinking that we already know the settings, why do we need "_get" :P

@@ +160,5 @@
> +                                                          this._cellBroadcastSearchList);
> +
> +      if ((newSearchList == oldSearchList) ||
> +          (newSearchList && oldSearchList &&
> +            newSearchList.gsm == oldSearchList.gsm &&

nit: align line #163, 164 and 165

@@ +167,5 @@
> +      }
> +
> +      gRadioInterfaceLayer
> +        .getRadioInterface(clientId).sendWorkerMessage("setCellBroadcastSearchList",
> +                                                       { searchList: newSearchList },

nit: indention

@@ +298,5 @@
> +    switch (aName) {
> +      case kSettingsCellBroadcastSearchList:
> +        if (DEBUG) {
> +          debug("'" + kSettingsCellBroadcastSearchList +
> +            "' is now " + JSON.stringify(aResult));

nit: indention.

@@ +306,5 @@
> +        break;
> +      case kSettingsCellBroadcastDisabled:
> +        if (DEBUG) {
> +          debug("'" + kSettingsCellBroadcastDisabled +
> +            "' is now " + JSON.stringify(aResult));

ditto.
Attachment #8500898 - Flags: review?(htsai) → review+
(In reply to Hsin-Yi Tsai [:hsinyi] from comment #2)
> Comment on attachment 8500898 [details] [diff] [review]
> Patch v1: Move CBS Settings implementation from RadioInterfaceLayer to
> CellBroadcastService.

Thanks, I'll address these nits in next update.
https://hg.mozilla.org/mozilla-central/rev/1a18c0aba319
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → 2.1 S6 (10oct)
You need to log in before you can comment on or make changes to this bug.