927320 - [B2G RIL] Rename TelephonyProvider to TelephonyService

Status: RESOLVED FIXED

(Firefox OS Graveyard :: RIL, defect)

Description

[Szu-Yu Chen [:aknow] (inactive)]

According to https://bugzilla.mozilla.org/show_bug.cgi?id=864485#c6
We would like to rename 'Provider' to 'Service'

nsIGonkTelephonyProvider
nsITelephonyProvider
TelephonyProvider.js
TelephonyProvider.manifest

Something need to change
1. filename (and reference part)
2. interface name (and reference part)
3. contract id (and reference part)
4. class name used in TelephonyProvider.js
...

Comment 1

[u482081]

Hello. Does anyone mind if I start working on this? My current bug (bug 956621) has been invalidated by someone else's work (on bug 945082).

Comment 2

[u482081]

Attachment: https://github.com/mozilla/gecko-dev/pull/4

Let me know if anything else needs to be changed, I'm happy to help.

Comment 3

[Vicamo Yang [:vicamo][:vyang]]

(In reply to Kevin Kofler from comment #2)
> Created attachment 8366441 [details] [review]
> https://github.com/mozilla/gecko-dev/pull/4
> 
> Let me know if anything else needs to be changed, I'm happy to help.

Hi, Kevin, thank you for taking this.  However I'm sorry to inform you that we need HG patches for landing in Gecko.  That GitHub is merely a read-only mirror.  Please see https://developer.mozilla.org/en-US/docs/Mercurial_FAQ#How_can_I_generate_a_patch_for_somebody_else_to_check-in_for_me.3F

Comment 4

[Vicamo Yang [:vicamo][:vyang]]

BTW, don't know if you speak Chinese?  I'm from Taiwan and we have a regular community meeting on Friday night.  If you need native language support, I'm glad to help there offlinely, too.

Comment 5

[Vicamo Yang [:vicamo][:vyang]]

Comment on attachment 8366441 [details] [review]
https://github.com/mozilla/gecko-dev/pull/4

https://github.com/mozilla/gecko-dev/pull/4/files#r9224449
Shouldn't rename nsITelephonyListener to nsITelephonyService.  Build failure.

Comment 6

[u482081]

Attachment: bug-927320-fix

I'm sorry about that unnecessary nsITelephonyListener > nsITelephonyService change. Here's an HG patch correcting that issue.

Comment 7

[u482081]

Comment on attachment 8370987 [details] [diff] [review]
bug-927320-fix

I forgot to rebase -_- I'll have a proper patch shortly. Thanks for your patience.

Comment 8

[u482081]

Attachment: bug-927320-fix

Sorry about that. Everything should be fine now.

Comment 9

[u482081]

Attachment: bug-927320-fix

Didn't add the renamed files -_-

Comment 10

[Vicamo Yang [:vicamo][:vyang]]

(In reply to Kevin Kofler from comment #9)
> Created attachment 8371166 [details] [diff] [review]
> bug-927320-fix
> 
> Didn't add the renamed files -_-

Hi, two quick nits.  First, for renamed files, please add '-M' to git commands or use 'hg mv' to generate patches.  It will generate a much smaller patch which is also easier to review.  Second, could you also help rename TELEPHONY_PROVIDER_* to TELEPHONY_SERVICE_*?  I see you have renamed GONK_TELEPHONY_PROVIDER, and it will be nice to have their names aligned.

Comment 11

[Vicamo Yang [:vicamo][:vyang]]

Comment on attachment 8371166 [details] [diff] [review]
bug-927320-fix

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

::: dom/telephony/ipc/TelephonyIPCProvider.h
@@ +20,5 @@
>                                       , public nsIObserver
>  {
>  public:
>    NS_DECL_ISUPPORTS
>    NS_DECL_NSITELEPHONYPROVIDER

NS_DECL_NSITELEPHONYSERVICE, this should have fail the compilation process.  Do you have B2G development environment setup[1]?  Maybe you should also consider applying for level 1 try account[2].

[1]: https://developer.mozilla.org/en-US/Firefox_OS/Building_and_installing_Firefox_OS
[2]: https://www.mozilla.org/hacking/commit-access-policy/

::: dom/telephony/test/marionette/test_dsds_default_service_id.js
@@ +5,5 @@
>  MARIONETTE_CONTEXT = "chrome";
>  
>  Cu.import("resource://gre/modules/Promise.jsm");
>  
>  const TELEPHONY_PROVIDER_CONTRACTID =

nit: s/TELEPHONY_PROVIDER_CONTRACTID/TELEPHONY_SERVICE_CONTRACTID/

Please also replace them in dom/telephony/ipc/TelephonyParent.cpp, dom/telephony/Telephony.cpp, dom/bluetooth/BluetoothRilListener.cpp

@@ +121,5 @@
>    return deferred.promise;
>  }
>  
>  getNumRadioInterfaces()
> +  .then(verify.bind(null, TELEPHONY_PROVIDER_CONTRACTID, "nsITelephonyService",

ditto

Comment 12

[u482081]

Attachment: bug-927320-fix

Comment 13

[Vicamo Yang [:vicamo][:vyang]]

Comment on attachment 8372263 [details] [diff] [review]
bug-927320-fix

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

I really want to verify this patch before giving comments, and I can't find an appropriate commit to apply it so it has to be rebased onto latest HEAD. (I've also found your Github gecko fork, but I still fail to apply this patch onto the same Gecko commit)

1) Have a rebase.  We'll eventually need that any way.
2) Use '-M' flag to minimize your Git patch.  See comment 10.
3) Naming consistency is really important when you're working on a huge project.  Similar names give a hint that things are highly related.  They could also serve as convenient keys to index, to search the source code.  So when you want to rename TelephonyProvider to TelephonyService, please also rename all the related names: s/Provider()/Service()/, s/provider/service/, ..., etc.  BTW, the original commit summary "Changed instances of TelephonyProvider to TelephonyService" is good enough.

Comment 14

[Ken Chang[:kenkai]]

Put this bug into backlog.

Comment 15

[Vicamo Yang [:vicamo][:vyang]]

Attachment: part 1/2: rename files

rename dom/telephony/gonk/{TelephonyProvider.js => TelephonyService.js}
rename dom/telephony/gonk/{TelephonyProvider.manifest => TelephonyService.manifest}
rename dom/telephony/ipc/{TelephonyIPCProvider.cpp => TelephonyIPCService.cpp}
rename dom/telephony/ipc/{TelephonyIPCProvider.h => TelephonyIPCService.h}
rename dom/telephony/{nsIGonkTelephonyProvider.idl => nsIGonkTelephonyService.idl}
rename dom/telephony/{nsITelephonyProvider.idl => nsITelephonyService.idl}

Comment 16

[Vicamo Yang [:vicamo][:vyang]]

Attachment: part 2/2: rename related variables

Comment 17

[Vicamo Yang [:vicamo][:vyang]]

`grep -nri provider dom/telephony` should now return zero lines.  Try build on all platforms: https://tbpl.mozilla.org/?tree=Try&rev=8ce19909d93b

Comment 18

[Ben Tian (inactive)]

Comment on attachment 8432341 [details] [diff] [review]
part 2/2: rename related variables

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

r=me on BluetoothRilListener.cpp changes. Thanks.

Comment 19

[Szu-Yu Chen [:aknow] (inactive)]

Comment on attachment 8432341 [details] [diff] [review]
part 2/2: rename related variables

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

LGTM.
However I didn't look into the diff thoroughly.
Just simply apply these two patches and make sure the following commands return nothing

  $ ack -i "telephony.*provider"
  $ ack -i provider dom/telephony/
  $ find . -iregex '.*telephony.*provider.*'

Comment 20

[Vicamo Yang [:vicamo][:vyang]]

Full try before push: https://tbpl.mozilla.org/?tree=Try&rev=5d8e8cb57034

Comment 21

[Vicamo Yang [:vicamo][:vyang]]

https://hg.mozilla.org/integration/b2g-inbound/rev/8272c8646ec5
https://hg.mozilla.org/integration/b2g-inbound/rev/9575c4678bd5

Comment 22

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/8272c8646ec5
https://hg.mozilla.org/mozilla-central/rev/9575c4678bd5