[B2G RIL] Rename TelephonyProvider to TelephonyService

RESOLVED FIXED in 2.0 S3 (6june)

Status

defect
RESOLVED FIXED
6 years ago
5 years ago

People

(Reporter: aknow, Assigned: vicamo)

Tracking

unspecified
2.0 S3 (6june)
x86_64
Linux
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(tracking-b2g:backlog)

Details

(Whiteboard: [good first bug][lang=js][mentor-lang=zh][p=1])

Attachments

(2 attachments, 5 obsolete attachments)

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
...
Whiteboard: [good first bug][lang=js][mentor-lang=zh]
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).
Let me know if anything else needs to be changed, I'm happy to help.
Attachment #8366441 - Flags: review?(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
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 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.
Attachment #8366441 - Flags: review?(vyang) → review-
Posted patch bug-927320-fix (obsolete) — Splinter Review
I'm sorry about that unnecessary nsITelephonyListener > nsITelephonyService change. Here's an HG patch correcting that issue.
Attachment #8366441 - Attachment is obsolete: true
Attachment #8370987 - Flags: review?(vyang)
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.
Attachment #8370987 - Flags: review?(vyang) → review-
Posted patch bug-927320-fix (obsolete) — Splinter Review
Sorry about that. Everything should be fine now.
Attachment #8370987 - Attachment is obsolete: true
Attachment #8371163 - Flags: review?(vyang)
Posted patch bug-927320-fix (obsolete) — Splinter Review
Didn't add the renamed files -_-
Attachment #8371163 - Attachment is obsolete: true
Attachment #8371163 - Flags: review?(vyang)
Attachment #8371166 - Flags: review?(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 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
Attachment #8371166 - Flags: review?(vyang) → review-
Posted patch bug-927320-fix (obsolete) — Splinter Review
Attachment #8371166 - Attachment is obsolete: true
Attachment #8372263 - Flags: review?(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.
Attachment #8372263 - Flags: review?(vyang)
Put this bug into backlog.
blocking-b2g: --- → backlog
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}
Assignee: nobody → vyang
Attachment #8372263 - Attachment is obsolete: true
Attachment #8432335 - Flags: review?(szchen)
Attachment #8432341 - Flags: review?(szchen)
Attachment #8432341 - Flags: review?(btian)
`grep -nri provider dom/telephony` should now return zero lines.  Try build on all platforms: https://tbpl.mozilla.org/?tree=Try&rev=8ce19909d93b
Whiteboard: [good first bug][lang=js][mentor-lang=zh] → [good first bug][lang=js][mentor-lang=zh][p=1]
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.
Attachment #8432341 - Flags: review?(btian) → review+
Attachment #8432335 - Flags: review?(szchen) → review+
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.*'
Attachment #8432341 - Flags: review?(szchen) → review+
https://hg.mozilla.org/mozilla-central/rev/8272c8646ec5
https://hg.mozilla.org/mozilla-central/rev/9575c4678bd5
Status: NEW → RESOLVED
Closed: 5 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → 2.0 S3 (6june)
blocking-b2g: backlog → ---
You need to log in before you can comment on or make changes to this bug.