Ignore uninteresting events when recording the last errors to report

Ignore uninteresting events when recording the last errors to report
back via chrome.cast.channel.onError.

BUG=410109
Committed: https://crrev.com/e30e427bb652b4b38210aa8fde24f9729e7d6638
Cr-Commit-Position: refs/heads/master@{#299367}

[mark a. foltz, 2014-09-17 20:51:51]

mfoltz@chromium.org changed reviewers:
+ imcheng@chromium.org

[imcheng, 2014-09-17 22:34:09]

https://codereview.chromium.org/580923002/diff/1/extensions/browser/api/cast_...
File extensions/browser/api/cast_channel/logger.cc (right):

https://codereview.chromium.org/580923002/diff/1/extensions/browser/api/cast_...
extensions/browser/api/cast_channel/logger.cc:109: case
proto::ERROR_STATE_CHANGED:
Are other state changes (read/write/connect states) interesting? It seems to me
that an event is considered interesting if it has at least one of net return
value (!= net::IO_PENDING), challenge reply type, or NSS error code.

[mark a. foltz, 2014-10-09 23:51:42]

Sorry for the delay in getting back to this.  PTAL

https://codereview.chromium.org/580923002/diff/1/extensions/browser/api/cast_...
File extensions/browser/api/cast_channel/logger.cc (right):

https://codereview.chromium.org/580923002/diff/1/extensions/browser/api/cast_...
extensions/browser/api/cast_channel/logger.cc:109: case
proto::ERROR_STATE_CHANGED:
On 2014/09/17 22:34:08, imcheng1 wrote:
> Are other state changes (read/write/connect states) interesting? It seems to
me
> that an event is considered interesting if it has at least one of net return
> value (!= net::IO_PENDING), challenge reply type, or NSS error code.

Changed this to just check for errors set as you mention.  That makes a lot more
sense, thanks!

[imcheng, 2014-10-10 19:17:04]

LGTM

[mark a. foltz, 2014-10-10 19:34:04]

The CQ bit was checked by mfoltz@chromium.org

[commit-bot: I haz the power, 2014-10-10 19:35:05]

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

[commit-bot: I haz the power, 2014-10-10 19:35:06]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-10-10 19:35:07]

No LGTM from a valid reviewer yet. Only full committers are accepted.
Even if an LGTM may have been provided, it was from a non-committer or
a provisional committer, _not_ a full super star committer.
See http://www.chromium.org/getting-involved/become-a-committer
Note that this has nothing to do with OWNERS files.

[mark a. foltz, 2014-10-10 21:11:37]

mfoltz@chromium.org changed reviewers:
+ miu@chromium.org

[mark a. foltz, 2014-10-10 21:11:37]

Yuri,
Do you mind taking a look to get a review by a committer?
m.

[miu, 2014-10-11 02:16:44]

lgtm % possible change to consider:

https://codereview.chromium.org/580923002/diff/20001/extensions/browser/api/c...
File extensions/browser/api/cast_channel/logger.cc (right):

https://codereview.chromium.org/580923002/diff/20001/extensions/browser/api/c...
extensions/browser/api/cast_channel/logger.cc:302: if
(IsErrorEvent(socket_event)) {
This is up to you, but IMO it hurts readability to have the logic in
IsErrorEvent() defined so far away from where it is relevant in the code.  My
suggestion:

auto& last_errors = it->second->last_errors;
if (socket_event.has_net_return_value() &&
    socket_event.net_return_value() < net::ERR_IO_PENDING)
  last_errors.net_return_value = socket_event.net_return_value();
if (socket_event.has_challenge_reply_error_type())
  last_errors.challenge_reply_error_type =
socket_event.challenge_reply_error_type();	
if (socket_event.has_nss_error_code())
  last_errors.nss_error_code = socket_event.nss_error_code();
if (last_errors.net_return_value != net::OK ||
    last_errors.challenge_reply_error_type != proto::CHALLENGE_REPLY_ERROR_NONE
||
    last_errors.nss_error_code != 0)
  last_errors.event_type = socket_event.type();

[mark a. foltz, 2014-10-13 20:32:56]

https://codereview.chromium.org/580923002/diff/20001/extensions/browser/api/c...
File extensions/browser/api/cast_channel/logger.cc (right):

https://codereview.chromium.org/580923002/diff/20001/extensions/browser/api/c...
extensions/browser/api/cast_channel/logger.cc:302: if
(IsErrorEvent(socket_event)) {
On 2014/10/11 02:16:44, miu wrote:
> This is up to you, but IMO it hurts readability to have the logic in
> IsErrorEvent() defined so far away from where it is relevant in the code.  My
> suggestion:
> 
> auto& last_errors = it->second->last_errors;
> if (socket_event.has_net_return_value() &&
>     socket_event.net_return_value() < net::ERR_IO_PENDING)
>   last_errors.net_return_value = socket_event.net_return_value();
> if (socket_event.has_challenge_reply_error_type())
>   last_errors.challenge_reply_error_type =
> socket_event.challenge_reply_error_type();	
> if (socket_event.has_nss_error_code())
>   last_errors.nss_error_code = socket_event.nss_error_code();
> if (last_errors.net_return_value != net::OK ||
>     last_errors.challenge_reply_error_type !=
proto::CHALLENGE_REPLY_ERROR_NONE
> ||
>     last_errors.nss_error_code != 0)
>   last_errors.event_type = socket_event.type();

Not a fan of making the existing code block longer; instead factored out your
suggestion into its own function.

[mark a. foltz, 2014-10-13 20:33:24]

The CQ bit was checked by mfoltz@chromium.org

[commit-bot: I haz the power, 2014-10-13 20:34:21]

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

[commit-bot: I haz the power, 2014-10-13 21:31:55]

Committed patchset #3 (id:140001)

[commit-bot: I haz the power, 2014-10-13 21:32:33]

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