Closed Bug 1150688 Opened 5 years ago Closed 5 years ago

GMPInstallManager.jsm should use new code in nsUpdateService.js


(Firefox :: General, defect)

Not set



Firefox 41
Tracking Status
firefox40 --- affected
firefox41 --- fixed


(Reporter: spohl, Assigned: vijendrasingh161, Mentored)



(Whiteboard: [good first bug])


(1 file, 2 obsolete files)

There are (2?) places in GMPInstallManager.jsm that use code copied directly from nsUpdateService.js. Bug 1149334 will be changing some of this code to make it easier to debug. We should update the code in GMPInstallManager.jsm accordingly. This seems like a great first bug and I'll be happy to mentor someone if that would be helpful.
Hi Stephen! I would like to work on this bug. It will be my first time. Can you point me to the source code files? I already and have mozilla-central set up.
Hi Vijendra, thanks for your help! You will want to look at attachment 8587708 [details] [diff] [review] from bug 1149334, and nsUpdateService.js in particular in that attachment. Then, open mozilla-central/toolkit/modules/GMPInstallManager.jsm and find the places where we copied code from nsUpdateService.js that has changed in bug 1149334. Then, simply apply these same changes to GMPInstallManager.jsm and upload the patch here. If you have any questions or get stuck, don't hesitate to let me know here or on irc.
Mentor: spohl.mozilla.bugs
Whiteboard: [good first bug][mentor=:spohl] → [good first bug]
Stephen thanks for all your help. I created a patch by first locating code that was changed in nsUpdateService.js as per and then searching the code in GMPInstallManager.jsm. If the piece of code was present, I updated the code.
Please let me know if any further changes are required. Thanks again!
Comment on attachment 8613919 [details] [diff] [review]

Review of attachment 8613919 [details] [diff] [review]:

This looks great! The first and third chunk of code is what we want. Please remove the middle one and resubmit the patch. Please also add a proper commit message to your patch and we should be able to land this. When you upload your next patch, please set the review flag to '?' and add me as reviewer. Thanks!

::: toolkit/modules/GMPInstallManager.jsm
@@ +110,1 @@
>            {dwNumberOfProcessors: DWORD},

You can remove this entire chunk because there are no code changes here, we don't want to introduce the incorrect indentation towards the end of this chunk and there's also some extra whitespace. You did remove some other whitespace, but since we're not touching this section of the code we'll defer this to a different time.
Attachment #8613919 - Flags: feedback+
Assignee: nobody → vijendrasingh161
Thanks for the review and guidance. Please have a look.
Attachment #8613919 - Attachment is obsolete: true
Attachment #8614789 - Flags: review?(spohl.mozilla.bugs)
Comment on attachment 8614789 [details] [diff] [review]

Review of attachment 8614789 [details] [diff] [review]:

Almost there. Let's change the commit message to the following:

Bug 1150688 - Update sections of GMPInstallManager.jsm, which were copied from nsUpdateService.js, to reflect the changes to nsUpdateService.js from bug 1149334.

Also, please make sure that you've followed all the instructions on the wiki on how to write a patch that can be checked in by someone else:

In particular, please make sure that you have a '[ui]' section in your .hgrc file.

Please request review from me one more time once you've uploaded the patch and we can go ahead and land this. Thanks!
Attachment #8614789 - Flags: review?(spohl.mozilla.bugs) → feedback+
Changed the commit message and checked the instructions. Thanks!
Attachment #8614789 - Attachment is obsolete: true
Attachment #8615066 - Flags: review?(spohl.mozilla.bugs)
Comment on attachment 8615066 [details] [diff] [review]

Review of attachment 8615066 [details] [diff] [review]:

Great, thank you!

I started a try server build for good measure. Once the results come back green, I'll request checkin for you and we'll get this landed.
Attachment #8615066 - Flags: review?(spohl.mozilla.bugs) → review+
Forgot to request xpcshell and mochitests, which exercise GMPs. New try run:
Keywords: checkin-needed
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 41
You need to log in before you can comment on or make changes to this bug.