Closed Bug 1340486 Opened 8 years ago Closed 8 years ago

Remove duplication of code at nsHttpNegotiate/NTLMAuth::TestPref and MatchesBaseURI

Categories

(Core :: Networking: HTTP, defect)

defect
Not set
normal

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.
Assignee: nobody → valentin.gosu
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.
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-
(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
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-
(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).
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+
(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?
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
btw, HttpAuthUtils.cpp is missing the license header.
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla54
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: