Remove nsNetUtilInlines.h

RESOLVED FIXED in Firefox 54

Status

()

RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: benjamin, Assigned: njfox, Mentored)

Tracking

({good-first-bug})

unspecified
mozilla54
good-first-bug
Points:
---

Firefox Tracking Flags

(firefox54 fixed)

Details

Attachments

(1 attachment)

(Reporter)

Description

2 years ago
After bug 1332639, nsNetUtilInlines.h isn't actually inlines any more. It previously existed because we could compile nsNetUtils.h both with MOZILLA_INTERNAL_API for in-libxul and without for external code.

External code linkage is no longer possible, so the only code that uses nsNetUtilsInlines.h is nsNetUtils.cpp.

The goal of this bug is to move any the code that's still needed out of nsNetUtilInlines.h and move it into nsNetUtils.cpp. This is mostly mechanical code moving and making sure Firefox still builds, so I'm going to call this a pretty good first bug and mentor it. Anything that is #ifndef MOZILLA_INTERNAL_API is dead and can just be removed.

Here's DXR links to the relevant code:
https://dxr.mozilla.org/mozilla-central/source/netwerk/base/nsNetUtilInlines.h
https://dxr.mozilla.org/mozilla-central/source/netwerk/base/nsNetUtil.cpp

When you are done, you will need to remove nsNetUtilInlines.h from network/base/moz.build and then `hg rm` it from the tree.
(Assignee)

Comment 1

2 years ago
Hi! I'm new to bugzilla and I'm interested in taking this on. Let me know if you'd willing to let me work on it and I will get started this afternoon.
nick - you don't need to ask permission to work on a bug - go ahead! It is nice to put a comment in there (just like you did) so that anyone else considering the same bug will know there is recent activity and can coordinate.
(Assignee)

Comment 3

2 years ago
Does the fixing of [Bug 1332639] have any implications for this bug?

Comment 4

2 years ago
Hey! I've tried removing the MOZILLA_INTERNAL_API definitions but the second I do that, I can't seem to build it. I'm doing something wrong? Would love a little help!
(Reporter)

Comment 5

2 years ago
(In reply to nick from comment #3)
> Does the fixing of [Bug 1332639] have any implications for this bug?

It means that change was merged from mozilla-inbound to mozilla-central.

(In reply to Vineet Reddy from comment #4)
> Hey! I've tried removing the MOZILLA_INTERNAL_API definitions but the second
> I do that, I can't seem to build it. I'm doing something wrong? Would love a
> little help!

I'm not around on IRC this morning, but you probably need to ask a more detailed question: e.g. provide the exact error message you have a question about, and maybe include your draft patch. The #introduction channel on IRC might be able to help also.
(Assignee)

Comment 6

2 years ago
I believe I've fixed this bug, but I'm not sure who to mark as the reviewer. Will you be reviewing this patch Benjamin?
(Assignee)

Comment 7

2 years ago
Nevermind, looks like mcmanus will be reviewing this one. I'll push shortly after running it through the try server.
Comment hidden (mozreview-request)
Comment on attachment 8842859 [details]
Bug 1343545 - Moved necessary code out of nsNetUtilInlines.h and removed the file

I'm fine with delegating the review to benjamin as he has mentored it.

I will note you have a file called commit-message in the diff again. Can't do that :)
Attachment #8842859 - Flags: review?(mcmanus) → review?(benjamin)
(Assignee)

Comment 10

2 years ago
Thanks Patrick. I'm not really sure what I'm doing wrong when I commit and push to review, as I'm just running the following commands:

<edit and save source code>
hg commit -m "Bug 123456 - .... r=mcmanus"
hg push review

Are these steps incorrect?
I'm not sure how you're doing it - but other review board requests don't have a new file with the commit message added.. I presume it would add a file to the repo if committed like that.

e.g.https://reviewboard.mozilla.org/r/102020/diff/7#index_header
(Reporter)

Updated

2 years ago
Assignee: nobody → nick
(Reporter)

Comment 12

2 years ago
mozreview-review
Comment on attachment 8842859 [details]
Bug 1343545 - Moved necessary code out of nsNetUtilInlines.h and removed the file

https://reviewboard.mozilla.org/r/116618/#review118786

r=me, and thank you for taking this!

::: commit-message-af1ed:1
(Diff revision 1)
> +Bug 1343545 - Moved necessary code out of nsNetUtilInlines.h and removed the file r=mcmanus

A note for the future. When you first push these to mozreview, it's best practice to mark it as "r?reviewer" instead of "r=reviewer". That way everyone knows that it's pending review. Autoland will convert r? to r= for you later.
Attachment #8842859 - Flags: review?(benjamin) → review+

Comment 13

2 years ago
Pushed by bsmedberg@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/2eef7989fbb6
Moved necessary code out of nsNetUtilInlines.h and removed the file r=bsmedberg

Comment 14

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/2eef7989fbb6
Status: NEW → RESOLVED
Last Resolved: 2 years ago
status-firefox54: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla54
You need to log in before you can comment on or make changes to this bug.