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

RESOLVED FIXED in Firefox 60

Status

()

enhancement
P3
normal
RESOLVED FIXED
2 years ago
a year ago

People

(Reporter: Gijs, Assigned: supersonic12910, Mentored)

Tracking

({good-first-bug, perf})

53 Branch
Firefox 60
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox60 fixed)

Details

(Whiteboard: [photon-perf][lang=c++][lang=cpp])

Attachments

(3 attachments)

Reporter

Description

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

Updated

2 years ago
Priority: -- → P3
Reporter

Comment 1

a year ago
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.
Mentor: gijskruitbosch+bugs
Keywords: good-first-bug, perf
Whiteboard: [photon-perf][lang=c++][lang=cpp]
Assignee

Comment 2

a year ago
(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...
Reporter

Comment 3

a year ago
(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. :-)
Assignee

Comment 4

a year ago
(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...)
Assignee

Comment 5

a year ago
(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?
Reporter

Comment 6

a year ago
(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)
Reporter

Comment 8

a year ago
(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
Assignee

Comment 9

a year ago
Posted file R
Assignee

Comment 10

a year ago
(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
Reporter

Comment 11

a year ago
(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.
Flags: needinfo?(supersonic12910)
Assignee

Comment 12

a year ago
(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.
Reporter

Comment 13

a year ago
(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 ..."
Assignee

Updated

a year ago
Flags: needinfo?(supersonic12910)
Reporter

Updated

a year ago
Attachment #8945719 - Attachment is patch: true
Reporter

Comment 15

a year ago
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!
Attachment #8945719 - Flags: review+
Reporter

Updated

a year ago
Assignee: nobody → supersonic12910
Status: NEW → ASSIGNED
Keywords: checkin-needed
Assignee

Comment 16

a year ago
(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

a year ago
Pushed by btara@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/dd66a66c6a94
Removal of Netscape 4.x Registry reading block from Windows MIME/extension mapping, r=gijs
Keywords: checkin-needed
Reporter

Comment 18

a year ago
(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

a year ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/dd66a66c6a94
Status: ASSIGNED → RESOLVED
Last Resolved: a year ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 60
Assignee

Comment 20

a year ago
(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.
Reporter

Comment 21

a year ago
(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. :-)
Flags: needinfo?(supersonic12910)
Assignee

Comment 22

a year ago
(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.
Flags: needinfo?(supersonic12910)
You need to log in before you can comment on or make changes to this bug.