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

Status: RESOLVED FIXED

(Core :: Networking: HTTP, defect)

Comment 1

[Valentin Gosu [:valentin] (he/him) {{ AFK in July }}]

Attachment: Bug 1340486 - Remove duplication of code at nsHttpNegotiate/NTLMAuth::TestPref and MatchesBaseURI

Review commit: https://reviewboard.mozilla.org/r/113910/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/113910/

Comment 2

[Valentin Gosu [:valentin] (he/him) {{ AFK in July }}]

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 3

[Honza Bambas (:mayhemer)]

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.

Comment 4

[Valentin Gosu [:valentin] (he/him) {{ AFK in July }}]

Comment on attachment 8839205 [details]
Bug 1340486 - Remove duplication of code at nsHttpNegotiate/NTLMAuth::TestPref and MatchesBaseURI

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/113910/diff/1-2/

Comment 5

[Valentin Gosu [:valentin] (he/him) {{ AFK in July }}]

(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 6

[Honza Bambas (:mayhemer)]

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

Comment 7

[Valentin Gosu [:valentin] (he/him) {{ AFK in July }}]

Comment on attachment 8839205 [details]
Bug 1340486 - Remove duplication of code at nsHttpNegotiate/NTLMAuth::TestPref and MatchesBaseURI

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/113910/diff/2-3/

Comment 8

[Valentin Gosu [:valentin] (he/him) {{ AFK in July }}]

(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 9

[Honza Bambas (:mayhemer)]

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

Comment 10

[Honza Bambas (:mayhemer)]

(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 11

[Valentin Gosu [:valentin] (he/him) {{ AFK in July }}]

Comment on attachment 8839205 [details]
Bug 1340486 - Remove duplication of code at nsHttpNegotiate/NTLMAuth::TestPref and MatchesBaseURI

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/113910/diff/3-4/

Comment 12

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/858477eb9953
Remove duplication of code at nsHttpNegotiate/NTLMAuth::TestPref and MatchesBaseURI r=mayhemer

Comment 13

[Honza Bambas (:mayhemer)]

btw, HttpAuthUtils.cpp is missing the license header.

Comment 14

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/858477eb9953

Comment 15

[Valentin Gosu [:valentin] (he/him) {{ AFK in July }}]

https://hg.mozilla.org/integration/mozilla-inbound/rev/1d547b862e0d366d4cde08596daee767ac26d4ee
Bug 1340486 - Add licence header r=me DONTBUILD

Comment 16

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/1d547b862e0d