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

Status: RESOLVED FIXED

(Firefox OS Graveyard :: RIL, defect)

Comment 1

[Bevis Tseng[:bevis][:btseng](Exited)]

Attachment: Patch v1: Move CBS Settings implementation from RadioInterfaceLayer to CellBroadcastService.

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!

Comment 2

[Hsin-Yi Tsai (she/her) [:hsinyi]]

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.

Comment 3

[Bevis Tseng[:bevis][:btseng](Exited)]

(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.

Comment 4

[Bevis Tseng[:bevis][:btseng](Exited)]

Attachment: Patch v2: Move CBS Settings implementation from RadioInterfaceLayer to CellBroadcastService. r=htsai

Address nits in comment 2.

Comment 5

[Bevis Tseng[:bevis][:btseng](Exited)]

update try server result:
https://tbpl.mozilla.org/?tree=Try&rev=0afd5167a573

Comment 6

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/integration/b2g-inbound/rev/1a18c0aba319

Comment 7

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/1a18c0aba319