Closed Bug 1340486 Opened 3 years ago Closed 3 years ago

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

Categories

(Core :: Networking: HTTP, defect)

Product:
Core
Component:
Networking: HTTP
Type:
defect
Priority:
Not set

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla54
Tracking Flags:
Tracking Status
firefox54 --- fixed

People

(Reporter: mayhemer, Assigned: valentin)

References

Details

(Whiteboard: [necko-would-take])

Attachments

(1 file)

Bug 1340486 - Remove duplication of code at nsHttpNegotiate/NTLMAuth::TestPref and MatchesBaseURI
59 bytes, text/x-review-board-request
mayhemer
: review+
Details 
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.
https://hg.mozilla.org/mozilla-central/rev/858477eb9953
Status: NEW → RESOLVED
Closed: 3 years ago
status-firefox54: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla54
Duplicate of this bug: 1340485
You need to log in before you can comment on or make changes to this bug.