[WebSocketExtensionParser] Have Consume.* methods return bool instead of using has_error()

[WebSocketExtensionParser] Have Consume.* methods return bool instead of using has_error()

Suggested by eroman at
https://codereview.chromium.org/1047623002/diff/90001/net/websockets/websocket_extension_parser.cc#newcode25

R=yhirano,eroman
BUG=none

Committed: https://crrev.com/d90d2932227e484d8b5377201357a103b6639500
Cr-Commit-Position: refs/heads/master@{#324863}

[tyoshino (SeeGerritForStatus), 2015-04-01 07:34:01]

tyoshino@chromium.org changed reviewers:
+ yhirano@chromium.org

[tyoshino (SeeGerritForStatus), 2015-04-01 07:34:10]

Low priority

[yhirano, 2015-04-06 09:14:27]

lgtm

[tyoshino (SeeGerritForStatus), 2015-04-06 09:20:21]

tyoshino@chromium.org changed reviewers:
+ eroman@chromium.org

[tyoshino (SeeGerritForStatus), 2015-04-06 09:20:28]

+eroman

[eroman, 2015-04-06 19:07:52]

LGTM, thanks for the refactor!

https://codereview.chromium.org/1050013002/diff/40001/net/websockets/websocke...
File net/websockets/websocket_extension_parser.cc (right):

https://codereview.chromium.org/1050013002/diff/40001/net/websockets/websocke...
net/websockets/websocket_extension_parser.cc:88: value_string =
value.as_string();
FYI: Before value.as_string() was called unconditionally even if ConsumeToken()
failed. I think the new behavior is correct, but just wanted to raise this
difference.

https://codereview.chromium.org/1050013002/diff/40001/net/websockets/websocke...
net/websockets/websocket_extension_parser.cc:129: return false;
optional: You might inverse the test above to do:

 if (....)
   return false;

 ++current_;

https://codereview.chromium.org/1050013002/diff/40001/net/websockets/websocke...
net/websockets/websocket_extension_parser.cc:131: if (token->empty())
optinal:

  return !token->empty();

https://codereview.chromium.org/1050013002/diff/40001/net/websockets/websocke...
net/websockets/websocket_extension_parser.cc:152: if (Consume(c))
optional: I find the opposite more readable:

if (!Consume(c)) {
  ....
  return false;
}

return true;

https://codereview.chromium.org/1050013002/diff/40001/net/websockets/websocke...
File net/websockets/websocket_extension_parser.h (right):

https://codereview.chromium.org/1050013002/diff/40001/net/websockets/websocke...
net/websockets/websocket_extension_parser.h:39: // TODO(tyoshino): Make the
following methods return success/fail.
You can now remove this TODO

https://codereview.chromium.org/1050013002/diff/40001/net/websockets/websocke...
net/websockets/websocket_extension_parser.h:40: bool Consume(char c);
Something else you may consider is to add WARN_UNUSED_RESULT to all of these
methods.

This can be something you do temporarily to help ensure that the refactor
updates all callsites correctly, or something you leave in the code to make sure
future consumers test for the error.

This comment is optional and up to your judgement, but I have found it very
useful when testing my own code.

[tyoshino (SeeGerritForStatus), 2015-04-13 14:11:59]

Thanks for review and suggestions!

https://codereview.chromium.org/1050013002/diff/40001/net/websockets/websocke...
File net/websockets/websocket_extension_parser.cc (right):

https://codereview.chromium.org/1050013002/diff/40001/net/websockets/websocke...
net/websockets/websocket_extension_parser.cc:88: value_string =
value.as_string();
On 2015/04/06 19:07:52, eroman wrote:
> FYI: Before value.as_string() was called unconditionally even if
ConsumeToken()
> failed. I think the new behavior is correct, but just wanted to raise this
> difference.

Thanks. value_string is not used when has_error_ is set. If ConsumeToken()
fails, it set has_error_. So, it's fine.

https://codereview.chromium.org/1050013002/diff/40001/net/websockets/websocke...
net/websockets/websocket_extension_parser.cc:129: return false;
On 2015/04/06 19:07:52, eroman wrote:
> optional: You might inverse the test above to do:
> 
>  if (....)
>    return false;
> 
>  ++current_;

Done.

https://codereview.chromium.org/1050013002/diff/40001/net/websockets/websocke...
net/websockets/websocket_extension_parser.cc:131: if (token->empty())
On 2015/04/06 19:07:52, eroman wrote:
> optinal:
> 
>   return !token->empty();

Done.

https://codereview.chromium.org/1050013002/diff/40001/net/websockets/websocke...
net/websockets/websocket_extension_parser.cc:152: if (Consume(c))
On 2015/04/06 19:07:52, eroman wrote:
> optional: I find the opposite more readable:
> 
> if (!Consume(c)) {
>   ....
>   return false;
> }
> 
> return true;

Done.

https://codereview.chromium.org/1050013002/diff/40001/net/websockets/websocke...
File net/websockets/websocket_extension_parser.h (right):

https://codereview.chromium.org/1050013002/diff/40001/net/websockets/websocke...
net/websockets/websocket_extension_parser.h:39: // TODO(tyoshino): Make the
following methods return success/fail.
On 2015/04/06 19:07:52, eroman wrote:
> You can now remove this TODO

Done.

https://codereview.chromium.org/1050013002/diff/40001/net/websockets/websocke...
net/websockets/websocket_extension_parser.h:40: bool Consume(char c);
On 2015/04/06 19:07:52, eroman wrote:
> Something else you may consider is to add WARN_UNUSED_RESULT to all of these
> methods.
> 
> This can be something you do temporarily to help ensure that the refactor
> updates all callsites correctly, or something you leave in the code to make
sure
> future consumers test for the error.
> 
> This comment is optional and up to your judgement, but I have found it very
> useful when testing my own code.

Done.

[tyoshino (SeeGerritForStatus), 2015-04-13 14:12:28]

The CQ bit was checked by tyoshino@chromium.org

[tyoshino (SeeGerritForStatus), 2015-04-13 14:12:28]

The patchset sent to the CQ was uploaded after l-g-t-m from eroman@chromium.org,
yhirano@chromium.org
Link to the patchset: https://codereview.chromium.org/1050013002/#ps60001
(title: "Addressed #7, Rebased")

[commit-bot: I haz the power, 2015-04-13 14:12:53]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1050013002/60001

[eroman, 2015-04-13 16:36:31]

lgtm

[commit-bot: I haz the power, 2015-04-13 16:53:40]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2015-04-13 16:55:49]

Patchset 4 (id:??) landed as
https://crrev.com/d90d2932227e484d8b5377201357a103b6639500
Cr-Commit-Position: refs/heads/master@{#324863}