Support specifying the GSSAPI library that will be used.

Support specifying the GSSAPI library that will be used.

This preference can be set either via command-line or via group policy.


BUG=53625
TEST=unittests: ConfigurationPolicyPrefStore*; net_unittests: HttpAuthHandlerNegotiateTest.*:HttpAuthGSSAPIPOSIXTest.*; manually: start Chrome with command-line switch --gssapi-library-name=XYZ and see if this results in the Chrome process loading /usr/lib/whatever/XYZ as soon as an authenticated HTTP site is encountered.

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

[Jakob Kummerow (corp), 2010-11-05 01:45:15]

Second part of my starter project: introduce a new preference for specifying the
GSSAPI library that shall be used. 

In the absence of a central static, early-initialized, thread-safe preference
storing mechanism, I found it most viable to solve this with a single static
private member variable in GSSAPISharedLibrary. 

The patch applies to current (as of earlier today) trunk (revision
645e7b604f23257d8d08eb285ab1700c80c48264) + my first CL (issue 3517018 patch set
4), since I assume that the latter will be landed before this CL. 
[That's also why the line counts in the overview are a bit off; this patch
actually comprises just 57 insertions and 3 deletions in total.)

Please review. 
Also, as before I need someone with SVN access to dispatch a try job. I'm happy
to provide a patch containing the combined diffs wrt. trunk for that purpose.

[cbentzel, 2010-11-05 11:38:17]

Thanks for doing this Jakob! Two changes I'd like to see are:

a) If the library is explicitly specified, do not fall back to system libraries
even if the library can't be dlopen'ed or doesn't have the correct symbols.

b) I'd like to change the way the value gets passed down. Here's a suggestion:
  - Add a constructor to GSSAPISharedLibrary with a library path argument.
  - Add a library path argument to HttpAuthHandlerRegistryFactory::Create
  - In HttpAuthHandlerRegistryFactory::Create, create a GSSAPISharedLibrary if
POSIX build and a path specified. Feed it into the
HttpAuthHandlerNegotiate::Factory which is created.
  - Adjust IOThread::CreateDefaultAuthHandlerFactory to handle this.

This will prevent the need for static member variables, and is consistent with
how other configuration variables are handled in the authentication code.

[danno, 2010-11-05 13:39:10]

LGTM with one nit (if you address Chris' feedback).

http://codereview.chromium.org/4560001/diff/1/2
File chrome/app/policy/policy_templates.grd (right):

http://codereview.chromium.org/4560001/diff/1/2#newcode409
chrome/app/policy/policy_templates.grd:409: GSSAPI Library Name
s/Library Name/library name/

[Jakob Kummerow (corp), 2010-11-08 14:41:49]

On 2010/11/05 11:38:17, cbentzel wrote:
> a) If the library is explicitly specified, do not fall back to system
libraries
> even if the library can't be dlopen'ed or doesn't have the correct symbols.

Done. 

> b) I'd like to change the way the value gets passed down. Here's a suggestion:
>   - Add a constructor to GSSAPISharedLibrary with a library path argument.
>   - Add a library path argument to HttpAuthHandlerRegistryFactory::Create
>   - In HttpAuthHandlerRegistryFactory::Create, create a GSSAPISharedLibrary if
> POSIX build and a path specified. Feed it into the
> HttpAuthHandlerNegotiate::Factory which is created.
>   - Adjust IOThread::CreateDefaultAuthHandlerFactory to handle this.

Here you go. The patch is about twice its previous size now, but it does work
without static member variables. 

Please review, run try jobs, and merge when you're happy. I can of course rebase
against current trunk if necessary.

http://codereview.chromium.org/4560001/diff/1/2
File chrome/app/policy/policy_templates.grd (right):

http://codereview.chromium.org/4560001/diff/1/2#newcode409
chrome/app/policy/policy_templates.grd:409: GSSAPI Library Name
On 2010/11/05 13:39:10, danno wrote:
> s/Library Name/library name/

Done.

[cbentzel, 2010-11-08 18:44:06]

Would it make life easier if I added the gssapi library handling and you take
over the prefservice end of things?

http://codereview.chromium.org/4560001/diff/6001/7007
File chrome/browser/policy/configuration_policy_store_interface.h (right):

http://codereview.chromium.org/4560001/diff/6001/7007#newcode61
chrome/browser/policy/configuration_policy_store_interface.h:61:
kPolicyGSSAPILibraryName
Nit: probably end with a comma to be consistent.

http://codereview.chromium.org/4560001/diff/6001/7009
File chrome/common/chrome_switches.cc (right):

http://codereview.chromium.org/4560001/diff/6001/7009#newcode620
chrome/common/chrome_switches.cc:620: const char kGSSAPILibraryName[]           
 = "gssapi-library-name";
perhaps call it gssapi-library-path?

http://codereview.chromium.org/4560001/diff/6001/7015
File net/http/http_auth_gssapi_posix.cc (right):

http://codereview.chromium.org/4560001/diff/6001/7015#newcode400
net/http/http_auth_gssapi_posix.cc:400: gssapi_library_name_ =
gssapi_library_name;
Do this in iniializer list rather than constructor body.

http://codereview.chromium.org/4560001/diff/6001/7015#newcode428
net/http/http_auth_gssapi_posix.cc:428: if (gssapi_library_name_.compare("")) {
if (!gssapi_library_name_.empty()) would be clearer.

http://codereview.chromium.org/4560001/diff/6001/7015#newcode430
net/http/http_auth_gssapi_posix.cc:430: static const char* kTempLibraryName[] =
{
This shouldn't be static, it could change each time LoadSharedLibrary is called.
Just make it a const with scope of this function block.

http://codereview.chromium.org/4560001/diff/6001/7016
File net/http/http_auth_gssapi_posix.h (right):

http://codereview.chromium.org/4560001/diff/6001/7016#newcode105
net/http/http_auth_gssapi_posix.h:105: explicit GSSAPISharedLibrary(std::string
gssapi_library_name);
const std::string& argument please.

http://codereview.chromium.org/4560001/diff/6001/7017
File net/http/http_auth_gssapi_posix_unittest.cc (right):

http://codereview.chromium.org/4560001/diff/6001/7017#newcode79
net/http/http_auth_gssapi_posix_unittest.cc:79: GSSAPILibrary* gssapi = new
GSSAPISharedLibrary("");
Use a scoped_ptr rather than explicit delete here.

http://codereview.chromium.org/4560001/diff/6001/7017#newcode86
net/http/http_auth_gssapi_posix_unittest.cc:86: GSSAPILibrary* gssapi = new
GSSAPISharedLibrary("/this/library/does/not/exist");
Nit: 80 char line

http://codereview.chromium.org/4560001/diff/6001/7017#newcode86
net/http/http_auth_gssapi_posix_unittest.cc:86: GSSAPILibrary* gssapi = new
GSSAPISharedLibrary("/this/library/does/not/exist");
Use a scoped_ptr, this is getting leaked in this test.

http://codereview.chromium.org/4560001/diff/6001/7018
File net/http/http_auth_handler_factory.cc (right):

http://codereview.chromium.org/4560001/diff/6001/7018#newcode60
net/http/http_auth_handler_factory.cc:60:
negotiate_factory->set_host_resolver(host_resolver);
This won't compile on non OS_POSIX/OS_WIN directories.

http://codereview.chromium.org/4560001/diff/6001/7019
File net/http/http_auth_handler_factory.h (right):

http://codereview.chromium.org/4560001/diff/6001/7019#newcode186
net/http/http_auth_handler_factory.h:186: std::string gssapi_library_name,
const std::string&

http://codereview.chromium.org/4560001/diff/6001/7020
File net/http/http_auth_handler_negotiate.cc (right):

http://codereview.chromium.org/4560001/diff/6001/7020#newcode279
net/http/http_auth_handler_negotiate.cc:279: auth_library_(auth_library) {
if you pass in an auth_library, please make behavior consistent between Win and
POSIX. Right now on Win it will ignore the passed in auth_library, whereas on
POSIX it will use it (and own it). 

My recommendation: move the default constructor back to the old mechanism, and
have the caller take care of ownership of the library.

http://codereview.chromium.org/4560001/diff/6001/7021
File net/http/http_auth_handler_negotiate.h (right):

http://codereview.chromium.org/4560001/diff/6001/7021#newcode48
net/http/http_auth_handler_negotiate.h:48: // ignored, so callers can simply
pass NULL in that case.
Could you just leave the Factory() call and use set_library() instead.

http://codereview.chromium.org/4560001/diff/6001/7021#newcode81
net/http/http_auth_handler_negotiate.h:81: // Callers beware: the passed
|*auth_library| will be deleted when this
It's possible we can delete (i.e make the factory the owner). If this is done,
it needs to be cross platform. Use a scoped_ptr to make it happen.

[Jakob Kummerow (corp), 2010-11-09 10:12:47]

I addressed your comments. Please review again. 

> Would it make life easier if I added the gssapi library handling and you take
> over the prefservice end of things?

Not sure what you mean. Isn't this CL in a state that you can agree with now?

http://codereview.chromium.org/4560001/diff/6001/7007
File chrome/browser/policy/configuration_policy_store_interface.h (right):

http://codereview.chromium.org/4560001/diff/6001/7007#newcode61
chrome/browser/policy/configuration_policy_store_interface.h:61:
kPolicyGSSAPILibraryName
On 2010/11/08 18:44:06, cbentzel wrote:
> Nit: probably end with a comma to be consistent.

Done.

http://codereview.chromium.org/4560001/diff/6001/7009
File chrome/common/chrome_switches.cc (right):

http://codereview.chromium.org/4560001/diff/6001/7009#newcode620
chrome/common/chrome_switches.cc:620: const char kGSSAPILibraryName[]           
 = "gssapi-library-name";
On 2010/11/08 18:44:06, cbentzel wrote:
> perhaps call it gssapi-library-path?

Calling it "gssapi-library-name" makes sense as it's totally OK to pass just a
library (file) name, e.g. "libgssapi_krb5.so.2.2". Passing full paths is OK, too
-- but I'd say "gssapi-library-path" suggests that you *need* to specify the
full path. 
What about calling it just "gssapi-library"?

http://codereview.chromium.org/4560001/diff/6001/7015
File net/http/http_auth_gssapi_posix.cc (right):

http://codereview.chromium.org/4560001/diff/6001/7015#newcode400
net/http/http_auth_gssapi_posix.cc:400: gssapi_library_name_ =
gssapi_library_name;
On 2010/11/08 18:44:06, cbentzel wrote:
> Do this in iniializer list rather than constructor body.

Done.

http://codereview.chromium.org/4560001/diff/6001/7015#newcode428
net/http/http_auth_gssapi_posix.cc:428: if (gssapi_library_name_.compare("")) {
On 2010/11/08 18:44:06, cbentzel wrote:
> if (!gssapi_library_name_.empty()) would be clearer.

Done.

http://codereview.chromium.org/4560001/diff/6001/7015#newcode430
net/http/http_auth_gssapi_posix.cc:430: static const char* kTempLibraryName[] =
{
On 2010/11/08 18:44:06, cbentzel wrote:
> This shouldn't be static, it could change each time LoadSharedLibrary is
called.
> Just make it a const with scope of this function block.

Done.

http://codereview.chromium.org/4560001/diff/6001/7016
File net/http/http_auth_gssapi_posix.h (right):

http://codereview.chromium.org/4560001/diff/6001/7016#newcode105
net/http/http_auth_gssapi_posix.h:105: explicit GSSAPISharedLibrary(std::string
gssapi_library_name);
On 2010/11/08 18:44:06, cbentzel wrote:
> const std::string& argument please. 

Done.

http://codereview.chromium.org/4560001/diff/6001/7017
File net/http/http_auth_gssapi_posix_unittest.cc (right):

http://codereview.chromium.org/4560001/diff/6001/7017#newcode79
net/http/http_auth_gssapi_posix_unittest.cc:79: GSSAPILibrary* gssapi = new
GSSAPISharedLibrary("");
On 2010/11/08 18:44:06, cbentzel wrote:
> Use a scoped_ptr rather than explicit delete here.

Done.

http://codereview.chromium.org/4560001/diff/6001/7017#newcode86
net/http/http_auth_gssapi_posix_unittest.cc:86: GSSAPILibrary* gssapi = new
GSSAPISharedLibrary("/this/library/does/not/exist");
On 2010/11/08 18:44:06, cbentzel wrote:
> Nit: 80 char line

Done.

http://codereview.chromium.org/4560001/diff/6001/7017#newcode86
net/http/http_auth_gssapi_posix_unittest.cc:86: GSSAPILibrary* gssapi = new
GSSAPISharedLibrary("/this/library/does/not/exist");
On 2010/11/08 18:44:06, cbentzel wrote:
> Use a scoped_ptr, this is getting leaked in this test.

Done.

http://codereview.chromium.org/4560001/diff/6001/7018
File net/http/http_auth_handler_factory.cc (right):

http://codereview.chromium.org/4560001/diff/6001/7018#newcode60
net/http/http_auth_handler_factory.cc:60:
negotiate_factory->set_host_resolver(host_resolver);
On 2010/11/08 18:44:06, cbentzel wrote:
> This won't compile on non OS_POSIX/OS_WIN directories.

Done.

http://codereview.chromium.org/4560001/diff/6001/7019
File net/http/http_auth_handler_factory.h (right):

http://codereview.chromium.org/4560001/diff/6001/7019#newcode186
net/http/http_auth_handler_factory.h:186: std::string gssapi_library_name,
On 2010/11/08 18:44:06, cbentzel wrote:
> const std::string& 

Done.

http://codereview.chromium.org/4560001/diff/6001/7020
File net/http/http_auth_handler_negotiate.cc (right):

http://codereview.chromium.org/4560001/diff/6001/7020#newcode279
net/http/http_auth_handler_negotiate.cc:279: auth_library_(auth_library) {
On 2010/11/08 18:44:06, cbentzel wrote:
> if you pass in an auth_library, please make behavior consistent between Win
and
> POSIX. Right now on Win it will ignore the passed in auth_library, whereas on
> POSIX it will use it (and own it). 
> 
> My recommendation: move the default constructor back to the old mechanism, and
> have the caller take care of ownership of the library.

I don't agree with this recommendation, since the caller is a static method. I
have changed it instead according to your other comments: Factory() ctor without
args, use set_library(...) instead, and let the factory own the library.

http://codereview.chromium.org/4560001/diff/6001/7021
File net/http/http_auth_handler_negotiate.h (right):

http://codereview.chromium.org/4560001/diff/6001/7021#newcode48
net/http/http_auth_handler_negotiate.h:48: // ignored, so callers can simply
pass NULL in that case.
On 2010/11/08 18:44:06, cbentzel wrote:
> Could you just leave the Factory() call and use set_library() instead. 

Done.

http://codereview.chromium.org/4560001/diff/6001/7021#newcode81
net/http/http_auth_handler_negotiate.h:81: // Callers beware: the passed
|*auth_library| will be deleted when this
On 2010/11/08 18:44:06, cbentzel wrote:
> It's possible we can delete (i.e make the factory the owner). If this is done,
> it needs to be cross platform. Use a scoped_ptr to make it happen. 

Done.

[cbentzel, 2010-11-09 16:18:03]

http://codereview.chromium.org/4560001/diff/20001/21021
File net/http/http_auth_handler_negotiate.h (right):

http://codereview.chromium.org/4560001/diff/20001/21021#newcode80
net/http/http_auth_handler_negotiate.h:80: auth_library_.reset(auth_library);
This is still doing the wrong thing in the Windows case - it will cause a double
delete of the default SSPI library if it is ever set (since the SSPI library is
a singleton which also deletes the lone instance.)

I recommend one of two ways to fix this:
  - Make it so the default library is not a singleton, and have it be owned by
the factory.
  - Change the code back to the way it was, and hold onto the GSSAPI library in
the IOThread::Globals

[Jakob Kummerow (corp), 2010-11-10 10:50:19]

I agree that the previous patch set would have segfaulted on Windows; sorry for
not noticing that earlier. 

Please review, start try jobs, and dcommit if happy.

http://codereview.chromium.org/4560001/diff/20001/net/http/http_auth_handler_...
File net/http/http_auth_handler_negotiate.h (right):

http://codereview.chromium.org/4560001/diff/20001/net/http/http_auth_handler_...
net/http/http_auth_handler_negotiate.h:80: auth_library_.reset(auth_library);
On 2010/11/09 16:18:03, cbentzel wrote:
> This is still doing the wrong thing in the Windows case - it will cause a
double
> delete of the default SSPI library if it is ever set (since the SSPI library
is
> a singleton which also deletes the lone instance.)
> 
> I recommend one of two ways to fix this:
>   - Make it so the default library is not a singleton, and have it be owned by
> the factory.
>   - Change the code back to the way it was, and hold onto the GSSAPI library
in
> the IOThread::Globals

Done.
I went with the first suggestion, as I consider it more elegant to have
ownership of the library at the same place where it's used.

[danno, 2010-11-10 16:52:01]

Mostly comments about comments. :-)

http://codereview.chromium.org/4560001/diff/26001/chrome/app/policy/policy_te...
File chrome/app/policy/policy_templates.grd (right):

http://codereview.chromium.org/4560001/diff/26001/chrome/app/policy/policy_te...
chrome/app/policy/policy_templates.grd:412: Specifies which GSSAPI library to
use for HTTP Authentication. If no setting is provided, <ph
name="PRODUCT_NAME">$1<ex>Google Chrome</ex></ph> will fall back to using a
default library name.
How is it specified? Full path? Just library name?

http://codereview.chromium.org/4560001/diff/26001/chrome/common/pref_names.cc
File chrome/common/pref_names.cc (right):

http://codereview.chromium.org/4560001/diff/26001/chrome/common/pref_names.cc...
chrome/common/pref_names.cc:1044: // String that specifies the name of a custom
GSSAPI library to load.
Just: 
// Specifies the name of a custom GSSAPI library to load.

http://codereview.chromium.org/4560001/diff/26001/net/http/http_auth_gssapi_p...
File net/http/http_auth_gssapi_posix.cc (right):

http://codereview.chromium.org/4560001/diff/26001/net/http/http_auth_gssapi_p...
net/http/http_auth_gssapi_posix.cc:430: // We have a user-specified library
name, so we ignore hard-coded values.
Try to avoid using "we" or ""I" in the code and instead passive voice.
This comment is also self-evident from looking at the code, so you might be able
to avoid it altogether.

http://codereview.chromium.org/4560001/diff/26001/net/http/http_auth_gssapi_p...
File net/http/http_auth_gssapi_posix.h (right):

http://codereview.chromium.org/4560001/diff/26001/net/http/http_auth_gssapi_p...
net/http/http_auth_gssapi_posix.h:103: // Pass an empty string for
|gssapi_library_name| to fall back to hard-coded
// If |gssapi_library_name| is empty, hard-coded default library names are used.

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

http://codereview.chromium.org/4560001/diff/26001/net/http/http_auth_handler_...
net/http/http_auth_handler_factory.h:178: // be loaded. This parameter is
disregarded on Windows.
... be loaded on all platforms except Windows.

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

http://codereview.chromium.org/4560001/diff/26001/net/http/http_auth_handler_...
net/http/http_auth_handler_negotiate.h:77: // factory is destructed.
Instead of "callers beware:..." you will often find this pattern elsewhere in
the code: "... assumes ownership of |auth_library|." So:

Sets the system library to use, thereby assuming ownership of |auth_library|.

http://codereview.chromium.org/4560001/diff/26001/net/http/http_auth_handler_...
File net/http/http_auth_handler_negotiate_unittest.cc (right):

http://codereview.chromium.org/4560001/diff/26001/net/http/http_auth_handler_...
net/http/http_auth_handler_negotiate_unittest.cc:214: // |auth_library_| will be
passed to |factory_| and will then be owned by it.
 |auth_library_| is passed to |factory_|, which assumes ownership of it.

[Jakob Kummerow (corp), 2010-11-10 17:48:39]

I addressed danno's comments; will wait for cbentzel's comments before uploading
a new patch set.

http://codereview.chromium.org/4560001/diff/26001/chrome/app/policy/policy_te...
File chrome/app/policy/policy_templates.grd (right):

http://codereview.chromium.org/4560001/diff/26001/chrome/app/policy/policy_te...
chrome/app/policy/policy_templates.grd:412: Specifies which GSSAPI library to
use for HTTP Authentication. If no setting is provided, <ph
name="PRODUCT_NAME">$1<ex>Google Chrome</ex></ph> will fall back to using a
default library name.
On 2010/11/10 16:52:01, danno wrote:
> How is it specified? Full path? Just library name?

Either. I clarified that in the description.

http://codereview.chromium.org/4560001/diff/26001/chrome/common/pref_names.cc
File chrome/common/pref_names.cc (right):

http://codereview.chromium.org/4560001/diff/26001/chrome/common/pref_names.cc...
chrome/common/pref_names.cc:1044: // String that specifies the name of a custom
GSSAPI library to load.
On 2010/11/10 16:52:01, danno wrote:
> Just: 
> // Specifies the name of a custom GSSAPI library to load.

Discussed offline, no change necessary (firstly for consistency, secondly the
comment is actually useful since "String that..." describes the value that will
be associated with this key, whose type is not apparent from the code).

http://codereview.chromium.org/4560001/diff/26001/net/http/http_auth_gssapi_p...
File net/http/http_auth_gssapi_posix.cc (right):

http://codereview.chromium.org/4560001/diff/26001/net/http/http_auth_gssapi_p...
net/http/http_auth_gssapi_posix.cc:430: // We have a user-specified library
name, so we ignore hard-coded values.
On 2010/11/10 16:52:01, danno wrote:
> Try to avoid using "we" or ""I" in the code and instead passive voice.
> This comment is also self-evident from looking at the code, so you might be
able
> to avoid it altogether.

Done.

http://codereview.chromium.org/4560001/diff/26001/net/http/http_auth_gssapi_p...
File net/http/http_auth_gssapi_posix.h (right):

http://codereview.chromium.org/4560001/diff/26001/net/http/http_auth_gssapi_p...
net/http/http_auth_gssapi_posix.h:103: // Pass an empty string for
|gssapi_library_name| to fall back to hard-coded
On 2010/11/10 16:52:01, danno wrote:
> // If |gssapi_library_name| is empty, hard-coded default library names are
used.

Done.

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

http://codereview.chromium.org/4560001/diff/26001/net/http/http_auth_handler_...
net/http/http_auth_handler_factory.h:178: // be loaded. This parameter is
disregarded on Windows.
On 2010/11/10 16:52:01, danno wrote:
> ... be loaded on all platforms except Windows.

Done.

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

http://codereview.chromium.org/4560001/diff/26001/net/http/http_auth_handler_...
net/http/http_auth_handler_negotiate.h:77: // factory is destructed.
On 2010/11/10 16:52:01, danno wrote:
> Instead of "callers beware:..." you will often find this pattern elsewhere in
> the code: "... assumes ownership of |auth_library|." So:
> 
> Sets the system library to use, thereby assuming ownership of |auth_library|. 

Done.

http://codereview.chromium.org/4560001/diff/26001/net/http/http_auth_handler_...
File net/http/http_auth_handler_negotiate_unittest.cc (right):

http://codereview.chromium.org/4560001/diff/26001/net/http/http_auth_handler_...
net/http/http_auth_handler_negotiate_unittest.cc:214: // |auth_library_| will be
passed to |factory_| and will then be owned by it.
On 2010/11/10 16:52:01, danno wrote:
>  |auth_library_| is passed to |factory_|, which assumes ownership of it.

Done.

[cbentzel, 2010-11-11 03:37:51]

LGTM, with some nits. 

Also, adding Will Chan for the IO thread changes (although these are pretty
straightforward).

http://codereview.chromium.org/4560001/diff/26001/net/http/http_auth_gssapi_p...
File net/http/http_auth_gssapi_posix.cc (right):

http://codereview.chromium.org/4560001/diff/26001/net/http/http_auth_gssapi_p...
net/http/http_auth_gssapi_posix.cc:426: const char** kLibraryNames;
Nit: this should be library_names now [it's no longer a compile time constant].

http://codereview.chromium.org/4560001/diff/26001/net/http/http_auth_gssapi_p...
net/http/http_auth_gssapi_posix.cc:428: const char* kTempLibraryName[1];
Nit: name this something like const char* user_specified_library[1];

http://codereview.chromium.org/4560001/diff/26001/net/http/http_auth_gssapi_p...
net/http/http_auth_gssapi_posix.cc:435: static const char* kTempLibraryNames[] =
{
Nit: name kDefaultLibraryNames (there's nothing "Temporary" about these).

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

http://codereview.chromium.org/4560001/diff/26001/net/http/http_auth_handler_...
net/http/http_auth_handler_factory.cc:54: negotiate_factory->set_library(new
GSSAPISharedLibrary(""));
I've occasionally seen 

std::string() 

instead of "", presumably to make it more obvious that the function does not
have a char* argument.

http://codereview.chromium.org/4560001/diff/26001/net/http/http_auth_sspi_win.cc
File net/http/http_auth_sspi_win.cc (right):

http://codereview.chromium.org/4560001/diff/26001/net/http/http_auth_sspi_win...
net/http/http_auth_sspi_win.cc:12: #include "base/singleton.h"
I think base/singleton.h include can be removed now.

[willchan no longer on Chromium, 2010-11-11 03:43:37]

IOThread changes lgtm.

On 2010/11/11 03:37:51, cbentzel wrote:
> LGTM, with some nits. 
> 
> Also, adding Will Chan for the IO thread changes (although these are pretty
> straightforward).
> 
>
http://codereview.chromium.org/4560001/diff/26001/net/http/http_auth_gssapi_p...
> File net/http/http_auth_gssapi_posix.cc (right):
> 
>
http://codereview.chromium.org/4560001/diff/26001/net/http/http_auth_gssapi_p...
> net/http/http_auth_gssapi_posix.cc:426: const char** kLibraryNames;
> Nit: this should be library_names now [it's no longer a compile time
constant].
> 
>
http://codereview.chromium.org/4560001/diff/26001/net/http/http_auth_gssapi_p...
> net/http/http_auth_gssapi_posix.cc:428: const char* kTempLibraryName[1];
> Nit: name this something like const char* user_specified_library[1];
> 
>
http://codereview.chromium.org/4560001/diff/26001/net/http/http_auth_gssapi_p...
> net/http/http_auth_gssapi_posix.cc:435: static const char* kTempLibraryNames[]
=
> {
> Nit: name kDefaultLibraryNames (there's nothing "Temporary" about these).
> 
>
http://codereview.chromium.org/4560001/diff/26001/net/http/http_auth_handler_...
> File net/http/http_auth_handler_factory.cc (right):
> 
>
http://codereview.chromium.org/4560001/diff/26001/net/http/http_auth_handler_...
> net/http/http_auth_handler_factory.cc:54: negotiate_factory->set_library(new
> GSSAPISharedLibrary(""));
> I've occasionally seen 
> 
> std::string() 
> 
> instead of "", presumably to make it more obvious that the function does not
> have a char* argument.
> 
>
http://codereview.chromium.org/4560001/diff/26001/net/http/http_auth_sspi_win.cc
> File net/http/http_auth_sspi_win.cc (right):
> 
>
http://codereview.chromium.org/4560001/diff/26001/net/http/http_auth_sspi_win...
> net/http/http_auth_sspi_win.cc:12: #include "base/singleton.h"
> I think base/singleton.h include can be removed now.

[Jakob Kummerow (corp), 2010-11-11 11:10:52]

Nits fixed. Ready for landing!

http://codereview.chromium.org/4560001/diff/26001/net/http/http_auth_gssapi_p...
File net/http/http_auth_gssapi_posix.cc (right):

http://codereview.chromium.org/4560001/diff/26001/net/http/http_auth_gssapi_p...
net/http/http_auth_gssapi_posix.cc:426: const char** kLibraryNames;
On 2010/11/11 03:37:51, cbentzel wrote:
> Nit: this should be library_names now [it's no longer a compile time
constant].

Done.

http://codereview.chromium.org/4560001/diff/26001/net/http/http_auth_gssapi_p...
net/http/http_auth_gssapi_posix.cc:428: const char* kTempLibraryName[1];
On 2010/11/11 03:37:51, cbentzel wrote:
> Nit: name this something like const char* user_specified_library[1];

Done.

http://codereview.chromium.org/4560001/diff/26001/net/http/http_auth_gssapi_p...
net/http/http_auth_gssapi_posix.cc:435: static const char* kTempLibraryNames[] =
{
On 2010/11/11 03:37:51, cbentzel wrote:
> Nit: name kDefaultLibraryNames (there's nothing "Temporary" about these).

Done.

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

http://codereview.chromium.org/4560001/diff/26001/net/http/http_auth_handler_...
net/http/http_auth_handler_factory.cc:54: negotiate_factory->set_library(new
GSSAPISharedLibrary(""));
On 2010/11/11 03:37:51, cbentzel wrote:
> I've occasionally seen 
> 
> std::string() 
> 
> instead of "", presumably to make it more obvious that the function does not
> have a char* argument. 

Done.

http://codereview.chromium.org/4560001/diff/26001/net/http/http_auth_sspi_win.cc
File net/http/http_auth_sspi_win.cc (right):

http://codereview.chromium.org/4560001/diff/26001/net/http/http_auth_sspi_win...
net/http/http_auth_sspi_win.cc:12: #include "base/singleton.h"
On 2010/11/11 03:37:51, cbentzel wrote:
> I think base/singleton.h include can be removed now.

Done.
Same for http_auth_gssapi_posix.h, btw.

[danno, 2010-11-11 11:54:58]

LGTM. I've submitted try jobs and will commit if all green.