1357788 - Remove Netscape 4x registry reading code from Windows MIME/extension mapping in nsOSHelperAppService.cpp

Status: RESOLVED FIXED

(Firefox :: File Handling, enhancement, P3)

Description

[:Gijs (he/him)]

From a quick and possibly inaccurate check on Wikipedia, Netscape 4.x was last released some 15-odd years ago, but apparently we're still checking registry settings left by it. Let's not.

https://dxr.mozilla.org/mozilla-central/rev/722fdbff1efc308a22060e75b603311d23541bb5/uriloader/exthandler/win/nsOSHelperAppService.cpp#80-85

Comment 1

[:Gijs (he/him)]

To fix this bug, all that needs to happen is removing the GetExtensionFrom4xRegistryInfo forward declaration and definition from  uriloader/exthandler/win/nsOSHelperAppService.cpp , and removing its 1 callsite.

Comment 2

[Ashish Daulatabad]

(In reply to :Gijs (lower availability until Jan 27) from comment #1)
> To fix this bug, all that needs to happen is removing the
> GetExtensionFrom4xRegistryInfo forward declaration and definition from 
> uriloader/exthandler/win/nsOSHelperAppService.cpp , and removing its 1
> callsite.

Can I work on this bug?
But I really need to know if removing the function 'GetExtensionFrom4xRegistryInfo' from the code is enough...

Comment 3

[:Gijs (he/him)]

(In reply to Ashish Daulatabad from comment #2)
> (In reply to :Gijs (lower availability until Jan 27) from comment #1)
> > To fix this bug, all that needs to happen is removing the
> > GetExtensionFrom4xRegistryInfo forward declaration and definition from 
> > uriloader/exthandler/win/nsOSHelperAppService.cpp , and removing its 1
> > callsite.
> 
> Can I work on this bug?

Yes!

> But I really need to know if removing the function
> 'GetExtensionFrom4xRegistryInfo' from the code is enough...

You would also need to remove the forward declaration and the 1 callsite in the same file, and check things compile. :-)

Comment 4

[Ashish Daulatabad]

(In reply to :Gijs (lower availability until Jan 27) from comment #3)
> You would also need to remove the forward declaration and the 1 callsite in
> the same file, and check things compile. :-)

Do you mean ./mach build?? 
(I'm sorry for asking such silly questions...)

Comment 5

[Ashish Daulatabad]

(In reply to :Gijs (lower availability until Jan 27) from comment #3)
> (In reply to Ashish Daulatabad from comment #2)
> > (In reply to :Gijs (lower availability until Jan 27) from comment #1)
> > > To fix this bug, all that needs to happen is removing the
> > > GetExtensionFrom4xRegistryInfo forward declaration and definition from 
> > > uriloader/exthandler/win/nsOSHelperAppService.cpp , and removing its 1
> > > callsite.
> > 
> > Can I work on this bug?
> 
> Yes!
> 
> > But I really need to know if removing the function
> > 'GetExtensionFrom4xRegistryInfo' from the code is enough...
> 
> You would also need to remove the forward declaration and the 1 callsite in
> the same file, and check things compile. :-)

Done removing the forward declaration, function, and it's call-site. Can you tell me the next step?

Comment 6

[:Gijs (he/him)]

(In reply to Ashish Daulatabad from comment #4)
> (In reply to :Gijs (lower availability until Jan 27) from comment #3)
> > You would also need to remove the forward declaration and the 1 callsite in
> > the same file, and check things compile. :-)
> 
> Do you mean ./mach build?? 
> (I'm sorry for asking such silly questions...)

Yes, running a build would be a good way to ensure things compile. Have you already built Firefox? If so, you could probably just run:

./mach build uriloader/exthandler binaries

which will be significantly faster than running the entire build again.

(In reply to Ashish Daulatabad from comment #5)
> Done removing the forward declaration, function, and it's call-site. Can you
> tell me the next step?

Apart from building, you could attach a patch? Assuming you've run `./mach mercurial-setup`, you can commit your code and then use `hg export` to export a commited changeset to a file and attach it to this bug. (Use the "Attach file" link, which points to https://bugzilla.mozilla.org/attachment.cgi?bugid=1357788&action=enter for this bug)

Comment 7

[Ashish Daulatabad]

Attachment: Remove Netscape 4x registry reading code from Windows MIME/extension mapping in nsOSHelperAppService.cpp

Comment 8

[:Gijs (he/him)]

(In reply to Ashish Daulatabad from comment #7)
> Created attachment 8945440 [details] [diff] [review]
> Remove Netscape 4x registry reading code from Windows MIME/extension mapping
> in nsOSHelperAppService.cpp

You seem to have attached the entire file, instead of just a diff. Can you attach a diff / exported patch (changeset), please? :-)

Basically, if you have committed, you can do:

hg export -r . > my-patch.txt

and then attach my-patch.txt

Comment 9

[Ashish Daulatabad]

Attachment: R

Comment 10

[Ashish Daulatabad]

(In reply to Ashish Daulatabad from comment #9)
> Created attachment 8945518 [details]
> R

Remove Netscape 4x registry reading code from Windows MIME/extension mapping in nsOSHelperAppService.cpp

Comment 11

[:Gijs (he/him)]

(In reply to Ashish Daulatabad from comment #10)
> (In reply to Ashish Daulatabad from comment #9)
> > Created attachment 8945518 [details]
> > R
> 
> Remove Netscape 4x registry reading code from Windows MIME/extension mapping
> in nsOSHelperAppService.cpp

OK, this is a diff, but this is a diff of the previous set of changes in the repository. I guess you haven't yet committed your changes. Did you test if they compiled ?

To commit your changes, use something like:

hg commit -m "Bug 1357788 - Remove Netscape 4.x registry reading code from Windows MIME/extension mapping, r?gijs"


and then the 'hg export' line from comment 8 will produce the right set of changes. Does that make sense? Feel free to ask questions if something is unclear.

Comment 12

[Ashish Daulatabad]

(In reply to :Gijs (lower availability until Jan 27) from comment #11)
> To commit your changes, use something like:
> 
> hg commit -m "Bug 1357788 - Remove Netscape 4.x registry reading code from
> Windows MIME/extension mapping, r?gijs"
> 
> 
> and then the 'hg export' line from comment 8 will produce the right set of
> changes. Does that make sense? Feel free to ask questions if something is
> unclear.

Yes, done creating patch file. Thanks a million.
But I don't get about needinfo.

Comment 13

[:Gijs (he/him)]

(In reply to Ashish Daulatabad from comment #12)
> (In reply to :Gijs (lower availability until Jan 27) from comment #11)
> > To commit your changes, use something like:
> > 
> > hg commit -m "Bug 1357788 - Remove Netscape 4.x registry reading code from
> > Windows MIME/extension mapping, r?gijs"
> > 
> > 
> > and then the 'hg export' line from comment 8 will produce the right set of
> > changes. Does that make sense? Feel free to ask questions if something is
> > unclear.
> 
> Yes, done creating patch file. Thanks a million.

OK, but you haven't attached the updated file to the bug, I think? :-)

> But I don't get about needinfo.

You can clear the request for needinfo by ticking the checkbox underneath the comment box "I am providing the requested information ..."

Comment 14

[Ashish Daulatabad]

Attachment: Bug 1357788 : Removal of netscape 4.x registry reading code block from Windows MIME/extension mapping in nsOSHelperAppService.cpp

Comment 15

[:Gijs (he/him)]

Comment on attachment 8945719 [details] [diff] [review]
Bug 1357788 : Removal of netscape 4.x registry reading code block from Windows MIME/extension mapping in nsOSHelperAppService.cpp

Review of attachment 8945719 [details] [diff] [review]:
-----------------------------------------------------------------

This looks great, thanks!

Comment 16

[Ashish Daulatabad]

(In reply to :Gijs (lower availability until Jan 27) from comment #15)
> Comment on attachment 8945719 [details] [diff] [review]
> Bug 1357788 : Removal of netscape 4.x registry reading code block from
> Windows MIME/extension mapping in nsOSHelperAppService.cpp
> 
> Review of attachment 8945719 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> This looks great, thanks!

Now, what's next?

Comment 17

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/dd66a66c6a94
Removal of Netscape 4.x Registry reading block from Windows MIME/extension mapping, r=gijs

Comment 18

[:Gijs (he/him)]

(In reply to Ashish Daulatabad from comment #16)
> (In reply to :Gijs (lower availability until Jan 27) from comment #15)
> > Comment on attachment 8945719 [details] [diff] [review]
> > Bug 1357788 : Removal of netscape 4.x registry reading code block from
> > Windows MIME/extension mapping in nsOSHelperAppService.cpp
> > 
> > Review of attachment 8945719 [details] [diff] [review]:
> > -----------------------------------------------------------------
> > 
> > This looks great, thanks!
> 
> Now, what's next?

It gets (well, just got) landed on an inbound (kind of like a testing) branch. Assuming it passes tests, it gets merged to mozilla-central, and then it'll be in the next Firefox Nightly (see https://nightly.mozilla.org ). Then based on the release schedule, the code (removal) will eventually become part of the Firefox release. :-)

In general, you won't need to do anything else now, unless the change gets backed out because of test failures. Well done! Do you have other bugs lined up to work on?

Comment 19

[Andreea Pavel [:apavel]]

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

Comment 20

[Ashish Daulatabad]

(In reply to :Gijs from comment #18)
> Well done! Do you have other bugs lined
> up to work on?

No. I need to find one.

Comment 21

[:Gijs (he/him)]

(In reply to Ashish Daulatabad from comment #20)
> (In reply to :Gijs from comment #18)
> > Well done! Do you have other bugs lined
> > up to work on?
> 
> No. I need to find one.

You could look at bug 1357791? It's in the same bit of code, and would probably need a bit more thought than this one, but should be totally doable. :-)

Comment 22

[Ashish Daulatabad]

(In reply to :Gijs from comment #21)
> You could look at bug 1357791? It's in the same bit of code, and would
> probably need a bit more thought than this one, but should be totally
> doable. :-)

I'm on it.