1150688 - GMPInstallManager.jsm should use new code in nsUpdateService.js

Status: RESOLVED FIXED

(Firefox :: General, defect)

Description

[Stephen A Pohl [:spohl] (OOO until 9/11)]

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.

Comment 1

[Vijendra Singh]

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 https://developer.mozilla.org/en-US/docs/Introduction and have mozilla-central set up.

Comment 2

[Stephen A Pohl [:spohl] (OOO until 9/11)]

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.

Comment 3

[Vijendra Singh]

Attachment: GMPInstallManagerWithUpdatedCode.patch

Stephen thanks for all your help. I created a patch by first locating code that was changed in nsUpdateService.js as per https://bugzilla.mozilla.org/show_bug.cgi?id=1149334 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 4

[Stephen A Pohl [:spohl] (OOO until 9/11)]

Comment on attachment 8613919 [details] [diff] [review]
GMPInstallManagerWithUpdatedCode.patch

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.

Comment 5

[Vijendra Singh]

Attachment: GMPInstallManagerWithUpdatedCode-Edited.patch

Thanks for the review and guidance. Please have a look.

Comment 6

[Stephen A Pohl [:spohl] (OOO until 9/11)]

Comment on attachment 8614789 [details] [diff] [review]
GMPInstallManagerWithUpdatedCode-Edited.patch

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:
https://developer.mozilla.org/en-US/docs/Mercurial_FAQ#How_can_I_generate_a_patch_for_somebody_else_to_check-in_for_me.3F

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!

Comment 7

[Vijendra Singh]

Attachment: GMPInstallManagerWithUpdatedCode-Edited.patch

Changed the commit message and checked the instructions. Thanks!

Comment 8

[Stephen A Pohl [:spohl] (OOO until 9/11)]

Comment on attachment 8615066 [details] [diff] [review]
GMPInstallManagerWithUpdatedCode-Edited.patch

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.

Comment 9

[Stephen A Pohl [:spohl] (OOO until 9/11)]

Try run:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=073a59ded6e4

Comment 10

[Stephen A Pohl [:spohl] (OOO until 9/11)]

Forgot to request xpcshell and mochitests, which exercise GMPs. New try run:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=862fc77db6eb

Comment 11

[Pulsebot]

https://hg.mozilla.org/integration/fx-team/rev/feb054f75ff2

Comment 12

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/feb054f75ff2

Comment 13

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/feb054f75ff2