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)
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•8 years ago
|
Assignee: nobody → valentin.gosu
Comment hidden (mozreview-request) |
Assignee | ||
Comment 2•8 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•8 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•8 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•8 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•8 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•8 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•8 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•8 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•8 years ago
|
||
btw, HttpAuthUtils.cpp is missing the license header.
Comment 14•8 years ago
|
||
bugherder |
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla54
Assignee | ||
Comment 15•8 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/1d547b862e0d366d4cde08596daee767ac26d4ee
Bug 1340486 - Add licence header r=me DONTBUILD
Comment 16•8 years ago
|
||
bugherder |
You need to log in
before you can comment on or make changes to this bug.
Description
•