[B2G RIL] Rename TelephonyProvider to TelephonyService

RESOLVED FIXED in 2.0 S3 (6june)

Status

RESOLVED FIXED
5 years ago
4 years ago

People

(Reporter: aknow, Assigned: vicamo)

Tracking

unspecified
2.0 S3 (6june)
x86_64
Linux
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)

(Reporter)

Description

5 years ago
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
...
(Assignee)

Updated

5 years ago
Whiteboard: [good first bug][lang=js][mentor-lang=zh]

Comment 1

5 years ago
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

5 years ago
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.
Attachment #8366441 - Flags: review?(vyang)
(Assignee)

Comment 3

5 years ago
(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
(Assignee)

Comment 4

5 years ago
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.
(Assignee)

Comment 5

5 years ago
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-

Comment 6

5 years ago
Created attachment 8370987 [details] [diff] [review]
bug-927320-fix

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 7

5 years ago
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-

Comment 8

5 years ago
Created attachment 8371163 [details] [diff] [review]
bug-927320-fix

Sorry about that. Everything should be fine now.
Attachment #8370987 - Attachment is obsolete: true
Attachment #8371163 - Flags: review?(vyang)

Comment 9

5 years ago
Created attachment 8371166 [details] [diff] [review]
bug-927320-fix

Didn't add the renamed files -_-
Attachment #8371163 - Attachment is obsolete: true
Attachment #8371163 - Flags: review?(vyang)
Attachment #8371166 - Flags: review?(vyang)
(Assignee)

Comment 10

5 years ago
(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.
(Assignee)

Comment 11

5 years ago
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-

Comment 12

5 years ago
Created attachment 8372263 [details] [diff] [review]
bug-927320-fix
Attachment #8371166 - Attachment is obsolete: true
Attachment #8372263 - Flags: review?(vyang)
(Assignee)

Comment 13

5 years ago
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
(Assignee)

Comment 15

5 years ago
Created attachment 8432335 [details] [diff] [review]
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}
Assignee: nobody → vyang
Attachment #8372263 - Attachment is obsolete: true
Attachment #8432335 - Flags: review?(szchen)
(Assignee)

Comment 16

5 years ago
Created attachment 8432341 [details] [diff] [review]
part 2/2: rename related variables
Attachment #8432341 - Flags: review?(szchen)
Attachment #8432341 - Flags: review?(btian)
(Assignee)

Comment 17

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

Updated

5 years ago
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+
(Reporter)

Updated

5 years ago
Attachment #8432335 - Flags: review?(szchen) → review+
(Reporter)

Comment 19

5 years ago
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+
(Assignee)

Updated

5 years ago
Blocks: 959978
https://hg.mozilla.org/mozilla-central/rev/8272c8646ec5
https://hg.mozilla.org/mozilla-central/rev/9575c4678bd5
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
(Assignee)

Updated

5 years ago
Target Milestone: --- → 2.0 S3 (6june)
blocking-b2g: backlog → ---
tracking-b2g: --- → backlog
You need to log in before you can comment on or make changes to this bug.