Remove static "set_fixed_cnonce" in favor of NonceGenerator objects....

Remove static "set_fixed_cnonce" in favor of NonceGenerator objects.

Trying to simplify cleanup for more unit tests.

BUG=None
TEST=net_unittests


Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=66439

[cbentzel, 2010-11-15 19:49:56]

I'm not sure if this is ready for final review. I am curious to know if you
consider this to be going to far to remove a static. 

I was adding more unit tests for digest handler and thought it would be a good
idea to reduce global dependencies on how the digest handler works. However,
this is adding new classes and virtual dispatch for unit test only purposes, so
I'm not sure if this style is OK.

[cbentzel, 2010-11-15 19:55:36]

Added Eric to reviewer list for his opinion.

[eroman, 2010-11-16 00:02:35]

http://codereview.chromium.org/5034001/diff/8001/net/http/http_auth_handler_d...
File net/http/http_auth_handler_digest.h (right):

http://codereview.chromium.org/5034001/diff/8001/net/http/http_auth_handler_d...
net/http/http_auth_handler_digest.h:41: std::string nonce_;
nit: const

http://codereview.chromium.org/5034001/diff/8001/net/http/http_auth_handler_d...
net/http/http_auth_handler_digest.h:49: virtual int
CreateAuthHandler(HttpAuth::ChallengeTokenizer* challenge,
Important: this creates a new dependency which I don't believe is documented in
HttpAuthHandlerFactory::CreateAuthHandler.

Notably, any auth handler created must NOT outlive the Factory that created it
(since it holds a pointer to memory scoped to the Factory).

I am not sure this is a restriction we want to add to the interface, however if
we have it it should at least be documented.

http://codereview.chromium.org/5034001/diff/8001/net/http/http_auth_handler_d...
net/http/http_auth_handler_digest.h:57: void set_nonce_generator(const
NonceGenerator* nonce_generator);
should mention ownership.

(even though it is a const pointer, we still delete it).

http://codereview.chromium.org/5034001/diff/8001/net/http/http_auth_handler_d...
net/http/http_auth_handler_digest.h:101: HttpAuthHandlerDigest(int nonce_count,
const NonceGenerator* nonce_generator);
Please mention ownerhsip model.

[wtc, 2010-11-16 01:47:41]

cbentzel: I think the general idea is good.  As for specific
implementation, you can also consider having only one
NonceGenerator class, and giving it an option to generate
fixed nonces.

[cbentzel, 2010-11-16 02:03:43]

Sorry Eric - this code wasn't ready for final review, but I was wondering if
you or wtc thought that this was overkill for removing a static or liked it.

I have been on projects that use heavy amounts of mocks and dependency
injection and other projects which abhor it. and I don't know where Chrome
[and the network stack] stands on this.

On Mon, Nov 15, 2010 at 7:02 PM, <eroman@chromium.org> wrote:

>
>
>
http://codereview.chromium.org/5034001/diff/8001/net/http/http_auth_handler_d...
> File net/http/http_auth_handler_digest.h (right):
>
>
>
http://codereview.chromium.org/5034001/diff/8001/net/http/http_auth_handler_d...
> net/http/http_auth_handler_digest.h:41: std::string nonce_;
> nit: const
>
>
>
http://codereview.chromium.org/5034001/diff/8001/net/http/http_auth_handler_d...
> net/http/http_auth_handler_digest.h:49: virtual int
> CreateAuthHandler(HttpAuth::ChallengeTokenizer* challenge,
> Important: this creates a new dependency which I don't believe is
> documented in HttpAuthHandlerFactory::CreateAuthHandler.
>
> Notably, any auth handler created must NOT outlive the Factory that
> created it (since it holds a pointer to memory scoped to the Factory).
>
> I am not sure this is a restriction we want to add to the interface,
> however if we have it it should at least be documented.
>
>
We do this in other factories. I'll make sure to document it.


>
>
http://codereview.chromium.org/5034001/diff/8001/net/http/http_auth_handler_d...
> net/http/http_auth_handler_digest.h:57: void set_nonce_generator(const
> NonceGenerator* nonce_generator);
> should mention ownership.
>
> (even though it is a const pointer, we still delete it).
>
>
>
http://codereview.chromium.org/5034001/diff/8001/net/http/http_auth_handler_d...
> net/http/http_auth_handler_digest.h:101: HttpAuthHandlerDigest(int
> nonce_count, const NonceGenerator* nonce_generator);
> Please mention ownerhsip model.
>
>
> http://codereview.chromium.org/5034001/
>

[cbentzel, 2010-11-16 21:13:51]

OK, this is ready for final review now. Thanks for comments so far.

I decided not to use the overridable nonce behavior wtc suggested: I end up
swapping virtual dispatch for checking a boolean, so doesn't seem to gain much.

[eroman, 2010-11-17 04:35:40]

lgtm

http://codereview.chromium.org/5034001/diff/26001/net/http/http_auth_handler_...
File net/http/http_auth_handler_factory.h (right):

http://codereview.chromium.org/5034001/diff/26001/net/http/http_auth_handler_...
net/http/http_auth_handler_factory.h:27: // The HttpAuthHandlerFactory object
_must_ outlive any of the HttpAuthHandler
thanks for adding the comment.

[wtc, 2010-11-17 20:21:50]

I only reviewed the .h files.  They look good to me except
for an excessive use of const.

I seem to recall that was the subject of a flame war on
chromium-dev, so I guess there are good reasons for this
practice.

http://codereview.chromium.org/5034001/diff/26001/net/http/http_auth_handler_...
File net/http/http_auth_handler_digest.h (right):

http://codereview.chromium.org/5034001/diff/26001/net/http/http_auth_handler_...
net/http/http_auth_handler_digest.h:77: scoped_ptr<const NonceGenerator>
nonce_generator_;
I know the C++ language allows deleting a const pointer, but
it is counter-intuitive to use a scoped_ptr<const Foo>.

I would change set_nonce_generator to take a non-const
pointer and define nonce_generator_ as
    scoped_ptr<NonceGenerator> nonce_generator_;

http://codereview.chromium.org/5034001/diff/26001/net/http/http_auth_handler_...
net/http/http_auth_handler_digest.h:172: const NonceGenerator* nonce_generator_;
In contrast, it is fine to use a const pointer here to
suggest this object doesn't own the NonceGenerator.

[cbentzel, 2010-11-18 12:06:37]

http://codereview.chromium.org/5034001/diff/26001/net/http/http_auth_handler_...
File net/http/http_auth_handler_digest.h (right):

http://codereview.chromium.org/5034001/diff/26001/net/http/http_auth_handler_...
net/http/http_auth_handler_digest.h:77: scoped_ptr<const NonceGenerator>
nonce_generator_;
On 2010/11/17 20:21:51, wtc wrote:
> I know the C++ language allows deleting a const pointer, but
> it is counter-intuitive to use a scoped_ptr<const Foo>.
> 
> I would change set_nonce_generator to take a non-const
> pointer and define nonce_generator_ as
>     scoped_ptr<NonceGenerator> nonce_generator_;

I tend to veer towards const-heaviness.

Flame war that justifies this case was here:
http://groups.google.com/a/chromium.org/group/chromium-dev/browse_thread/thre...

Basically, concensus was that scoped_refptr<const T> was OK, since deletion was
not considered mutation.

I can easily shift back to non-const nonce if you think it makes the contract
clearer.