Closed
Bug 1340486
Opened 7 years ago
Closed 7 years ago
Remove duplication of code at nsHttpNegotiate/NTLMAuth::TestPref and MatchesBaseURI
Categories
(Core :: Networking: HTTP, defect)
Core
Networking: HTTP
Tracking
()
RESOLVED
FIXED
mozilla54
Tracking | Status | |
---|---|---|
firefox54 | --- | fixed |
People
(Reporter: mayhemer, Assigned: valentin)
References
Details
(Whiteboard: [necko-would-take])
Attachments
(1 file)
No description provided.
Reporter | ||
Updated•7 years ago
|
Assignee: nobody → valentin.gosu
Comment hidden (mozreview-request) |
Assignee | ||
Comment 2•7 years ago
|
||
mozreview-review |
Comment on attachment 8839205 [details] Bug 1340486 - Remove duplication of code at nsHttpNegotiate/NTLMAuth::TestPref and MatchesBaseURI https://reviewboard.mozilla.org/r/113910/#review115422 ::: netwerk/protocol/http/HttpAuthUtils.cpp:21 (Diff revision 1) > + if (NS_FAILED(uri->GetScheme(scheme))) > + return false; > + if (NS_FAILED(uri->GetAsciiHost(host))) > + return false; > + > + port = NS_GetRealPort(uri); NS_GetRealPort was only used in one of the two implementations, so I used that one. Let me know if you think this is wrong.
Reporter | ||
Comment 3•7 years ago
|
||
mozreview-review |
Comment on attachment 8839205 [details] Bug 1340486 - Remove duplication of code at nsHttpNegotiate/NTLMAuth::TestPref and MatchesBaseURI https://reviewboard.mozilla.org/r/113910/#review116790 r- for the public functions naming. otherwise thanks, but please next time don't waste your time on patches we don't necessarily need. ::: netwerk/protocol/http/HttpAuthUtils.h:14 (Diff revision 1) > +class nsIURI; > + > +namespace mozilla { namespace net { > + > +bool TestPref(nsIURI *uri, const char *pref); > +bool MatchesBaseURI(const nsCSubstring &matchScheme, these definitely need way better names when now public. ::: netwerk/protocol/http/HttpAuthUtils.cpp:21 (Diff revision 1) > + if (NS_FAILED(uri->GetScheme(scheme))) > + return false; > + if (NS_FAILED(uri->GetAsciiHost(host))) > + return false; > + > + port = NS_GetRealPort(uri); Yep, my omission! thanks.
Attachment #8839205 -
Flags: review?(honzab.moz) → review-
Comment hidden (mozreview-request) |
Assignee | ||
Comment 5•7 years ago
|
||
(In reply to Honza Bambas (:mayhemer) from comment #3) > > +bool TestPref(nsIURI *uri, const char *pref); > > +bool MatchesBaseURI(const nsCSubstring &matchScheme, > these definitely need way better names when now public. I put them in a new namespace called auth_utils
Reporter | ||
Comment 6•7 years ago
|
||
mozreview-review |
Comment on attachment 8839205 [details] Bug 1340486 - Remove duplication of code at nsHttpNegotiate/NTLMAuth::TestPref and MatchesBaseURI https://reviewboard.mozilla.org/r/113910/#review117088 ::: netwerk/protocol/http/HttpAuthUtils.h:11 (Diff revision 2) > +#define HttpAuthUtils_h__ > + > + > +class nsIURI; > + > +namespace mozilla { namespace net { namespace auth_utils { I thing just ::auth would do. ::: netwerk/protocol/http/HttpAuthUtils.h:13 (Diff revision 2) > + > +class nsIURI; > + > +namespace mozilla { namespace net { namespace auth_utils { > + > +bool TestPref(nsIURI *uri, const char *pref); TestPref doesn't say a bit what this is doing. there is also no comment, please add one. I think something "uri matches preference pattern" or something less wordy would do? ::: netwerk/protocol/http/HttpAuthUtils.h:14 (Diff revision 2) > +class nsIURI; > + > +namespace mozilla { namespace net { namespace auth_utils { > + > +bool TestPref(nsIURI *uri, const char *pref); > +bool MatchesBaseURI(const nsCSubstring &matchScheme, I think this one doesn't need to be public, please double check and remove from export if confirmed
Attachment #8839205 -
Flags: review?(honzab.moz) → review-
Comment hidden (mozreview-request) |
Assignee | ||
Comment 8•7 years ago
|
||
(In reply to Honza Bambas (:mayhemer) from comment #6) Thanks for the review. > > +bool MatchesBaseURI(const nsCSubstring &matchScheme, > I think this one doesn't need to be public, please double check and remove > from export if confirmed I was going to use it for unit tests, but as it's not used anywhere else, I removed it and I'll add it back in the other bug (eventually).
Reporter | ||
Comment 9•7 years ago
|
||
mozreview-review |
Comment on attachment 8839205 [details] Bug 1340486 - Remove duplication of code at nsHttpNegotiate/NTLMAuth::TestPref and MatchesBaseURI https://reviewboard.mozilla.org/r/113910/#review117098 Still a lot to update, but I think it doesn't need another review round. thanks. ::: netwerk/protocol/http/HttpAuthUtils.h:14 (Diff revision 3) > +class nsIURI; > + > +namespace mozilla { namespace net { namespace auth { > + > +/* Tries to match the given URI against the value of a given pref > + * The pref should be in pseudo-BNF format. you should carry the comment from the method implementation here. "pseudo-BNF" is just a general claim that there is a grammar for the pref, but doesn't say what grammar it is. ::: netwerk/protocol/http/HttpAuthUtils.cpp:18 (Diff revision 3) > +bool > +URIMatchesPrefPattern(nsIURI *uri, const char *pref) > +{ > + nsCOMPtr<nsIPrefBranch> prefs = do_GetService(NS_PREFSERVICE_CONTRACTID); > + if (!prefs) > + return false; if you have time for it, please enclose ifs as |if () { }| if not, just leave it. ::: netwerk/protocol/http/HttpAuthUtils.cpp:38 (Diff revision 3) > + char *hostList; > + if (NS_FAILED(prefs->GetCharPref(pref, &hostList)) || !hostList) > + return false; > + > + struct FreePolicy { void operator()(void* p) { free(p); } }; > + mozilla::UniquePtr<char[], FreePolicy> hostListScope; these need #includes honestly, there is a number of headers to include here ::: netwerk/protocol/http/HttpAuthUtils.cpp:69 (Diff revision 3) > + } > + > + return false; > +} > + > +bool put to a ::detail subnamespace
Attachment #8839205 -
Flags: review?(honzab.moz) → review+
Reporter | ||
Comment 10•7 years ago
|
||
(In reply to Valentin Gosu [:valentin] from comment #8) > (In reply to Honza Bambas (:mayhemer) from comment #6) > > Thanks for the review. > > > > +bool MatchesBaseURI(const nsCSubstring &matchScheme, > > I think this one doesn't need to be public, please double check and remove > > from export if confirmed > > I was going to use it for unit tests, but as it's not used anywhere else, I > removed it and I'll add it back in the other bug (eventually). isn't a test for the main function enough?
Comment hidden (mozreview-request) |
Comment 12•7 years ago
|
||
Pushed by valentin.gosu@gmail.com: https://hg.mozilla.org/integration/autoland/rev/858477eb9953 Remove duplication of code at nsHttpNegotiate/NTLMAuth::TestPref and MatchesBaseURI r=mayhemer
Reporter | ||
Comment 13•7 years ago
|
||
btw, HttpAuthUtils.cpp is missing the license header.
Comment 14•7 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/858477eb9953
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla54
Assignee | ||
Comment 15•7 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/1d547b862e0d366d4cde08596daee767ac26d4ee Bug 1340486 - Add licence header r=me DONTBUILD
Comment 16•7 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/1d547b862e0d
You need to log in
before you can comment on or make changes to this bug.
Description
•