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

RESOLVED FIXED in Firefox 60

Status

()

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
good-first-bug, perf
Points:
---

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
status-firefox60: --- → fixed
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.