Add WARN_UNUSED_RESULT to crypto/hmac.h

Add WARN_UNUSED_RESULT to crypto/hmac.h

BUG=none
TEST=none


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

[Ryan Sleevi, 2011-07-28 02:12:51]

wtc: crypto/, OWNERS for net/
dilmah: chrome/browser
rickcam: chrome/common/net
wez: remoting/

abarth: PTAL at the changes to net/http, since this does introduce a functional
change that I want to make sure remains compliant with the MAC cookie
specification. Previously, it was possible under certain inputs or certain
environmental situations that generating the HMAC would fail. The result of such
a failure would be to DCHECK (which nops in release mode) and to add an empty
Authorization header.

The new behaviour is that if the HMAC generation fails, it continues on with
trying the next available cookie (if any), and only adds the Authorization
header on success. This seems perfectly reasonable behaviour, but I wanted to
make sure you were OK.

[abarth-chromium, 2011-07-28 02:22:07]

Yep.  That's the right thing to do.

[Sergey Ulanov, 2011-07-28 03:26:31]

One nit regarding comments. Otherwise remouting changes LGTM.

http://codereview.chromium.org/7522014/diff/4001/remoting/protocol/secure_p2p...
File remoting/protocol/secure_p2p_socket.cc (right):

http://codereview.chromium.org/7522014/diff/4001/remoting/protocol/secure_p2p...
remoting/protocol/secure_p2p_socket.cc:261: // 6. Compare the hash values.
Update the comments

[Denis Lagno, 2011-07-28 08:13:37]

LGTM

http://codereview.chromium.org/7522014/diff/5001/net/http/http_mac_signature.cc
File net/http/http_mac_signature.cc (right):

http://codereview.chromium.org/7522014/diff/5001/net/http/http_mac_signature....
net/http/http_mac_signature.cc:102: if (id_.empty() || method_.empty())
nit: imho it is better to merge DCHECK(condition) and subsequent if(!condition)

like this:

if (id_.empty()) {
  NOTREACHED() << "Call AddStateInfo first.";
  return false;
}
if (method_.empty()) {
  NOTREACHED() << "Call AddHttpInfo first.";
  return false;
}

http://codereview.chromium.org/7522014/diff/5001/net/http/http_mac_signature.h
File net/http/http_mac_signature.h (right):

http://codereview.chromium.org/7522014/diff/5001/net/http/http_mac_signature....
net/http/http_mac_signature.h:55: std::string GenerateNormalizedRequest(const
std::string& age,
nit: now this method looks inconsistent among surrounding methods.

on the one hand, all surrounding methods are conceptually similar.
on the other hand now they use different styles of returning result.

http://codereview.chromium.org/7522014/diff/5001/remoting/protocol/secure_p2p...
File remoting/protocol/secure_p2p_socket.cc (right):

http://codereview.chromium.org/7522014/diff/5001/remoting/protocol/secure_p2p...
remoting/protocol/secure_p2p_socket.cc:169: CHECK(msg_hasher_.Sign(
nit: I understand that CHECK is executed always, both in debug and non-debug
environments.
However, still I consider it is bad practice to put code with side-effects into
CHECK.
I'd prefer to store return value into variable and then CHECK this variable.

[Rick Campbell, 2011-07-28 17:39:24]

LGTM (for chrome/common/net)

[wtc, 2011-07-28 18:11:41]

LGTM.

http://codereview.chromium.org/7522014/diff/5001/net/http/http_mac_signature.h
File net/http/http_mac_signature.h (right):

http://codereview.chromium.org/7522014/diff/5001/net/http/http_mac_signature....
net/http/http_mac_signature.h:42: // Generates a value suitable for the
Authorization header of an HTTP
The first sentence should read:
  Generates the Authorization header for use in an HTTP request.

http://codereview.chromium.org/7522014/diff/5001/net/url_request/url_request_...
File net/url_request/url_request_http_job.cc (right):

http://codereview.chromium.org/7522014/diff/5001/net/url_request/url_request_...
net/url_request/url_request_http_job.cc:83: authorization_header);
Nit: this should fit on the previous line.

[commit-bot: I haz the power, 2011-07-30 05:18:05]

Change committed as 94826