Closed
Bug 1343545
Opened 8 years ago
Closed 8 years ago
Remove nsNetUtilInlines.h
Categories
(Core :: Networking, enhancement)
Core
Networking
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.
Assignee | ||
Comment 1•8 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.
Comment 2•8 years ago
|
||
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•8 years ago
|
||
Does the fixing of [Bug 1332639] have any implications for this bug?
Comment 4•8 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•8 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•8 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•8 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 9•8 years ago
|
||
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•8 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?
Comment 11•8 years ago
|
||
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•8 years ago
|
Assignee: nobody → nick
Reporter | ||
Comment 12•8 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•8 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•8 years ago
|
||
bugherder |
Status: NEW → RESOLVED
Closed: 8 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.
Description
•