Closed Bug 980395 Opened 10 years ago Closed 10 years ago
.moz Mobile Connection uses from the FTU app
46 bytes, text/x-github-pull-request
|Details | Review|
The FTU app still uses navigator.mozMobileConnection in one place and in unit-tests; those uses should be removed and made to rely only on navigator.mozMobileConnections.
Hi Gabriele I like to work on this.Can i get this assigned? Thanks
(In reply to kumar rishav from comment #1) > Hi Gabriele > I like to work on this.Can i get this assigned? Sure, here you go.
Assignee: nobody → rishav006
Status: NEW → ASSIGNED
Hi Gabriele I have made required changes.Have a look on this PR ? Thanks
Attachment #8406300 - Flags: review?(gsvelto)
Comment on attachment 8406300 [details] [review] Bug 980395 - Remove navigator.mozMobileConnection uses from the FTU app (In reply to kumar rishav from comment #3) > I have made required changes.Have a look on this PR ? I can't review your PR as I'm not a peer so I'll give you a feedback. Once this is ready you'll have to ask for a FTU peer to review your patch. I've left some comments on the PR but currently it is incomplete, with your changes the unit tests don't run at all. You have to make sure the unit-tests run and do so correctly, ask for feedback again once you have them running properly. I suggest you to run the tests w/o your changes and understand what they're doing before doing further changes. Also you should remove the apps/communications/ftu/test/unit/mock_navigator_moz_mobile_connection.js file as part of the PR as it won't be needed anymore once all the changes have been made.
Attachment #8406300 - Flags: review?(gsvelto) → feedback-
Hi Gabriele I have made the suggested changes and unit test is running successfully. Have a look and give your feedback.Also i have asked fernando to review the PR. Once again Thanks a lot Gabriele :)
Comment on attachment 8406397 [details] [review] Bug 980395 - Remove navigator.mozMobileConnection uses from the FTU app Looks good to me, thanks for taking this.
Attachment #8406397 - Flags: feedback?(gsvelto) → feedback+
Comment on attachment 8406397 [details] [review] Bug 980395 - Remove navigator.mozMobileConnection uses from the FTU app Quick and easy for me, all the review was done already :p (thanks Gabriele). And thanks kumar for taking this, code looks good. Just a small detail for readability (comment on github). So, r+ proven the changes (just style, shouldn't give any problems), and merge once travis gets green again.
Attachment #8406397 - Flags: review?(fernando.campo) → review+
Hi Fernando Campo I have done the changes as per your suggestion.Have a look on final PR? Thanks Hi gabriele After getting this patch merged,can you assign me other bugs i have commented before? Thanks
Thanks to both, Travis was green so I merged the PR to gaia/master 72c42e5568397425c8b8a96ad0aa8f6778d026f5 https://github.com/mozilla-b2g/gaia/commit/72c42e5568397425c8b8a96ad0aa8f6778d026f5
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.