Closed Bug 1343545 Opened 7 years ago Closed 7 years ago

Remove nsNetUtilInlines.h

Categories

(Core :: Networking, enhancement)

enhancement
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla54
Tracking Status
firefox54 --- fixed

People

(Reporter: benjamin, Assigned: njfox, Mentored)

References

Details

(Keywords: good-first-bug)

Attachments

(1 file)

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.
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.
Does the fixing of [Bug 1332639] have any implications for this bug?
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!
(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.
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?
Nevermind, looks like mcmanus will be reviewing this one. I'll push shortly after running it through the try server.
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)
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
Assignee: nobody → nick
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+
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
https://hg.mozilla.org/mozilla-central/rev/2eef7989fbb6
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla54
You need to log in before you can comment on or make changes to this bug.