cast channel error cleanup

cast channel error cleanup.

BUG=429005
Committed: https://crrev.com/c84b223b5b792e637b0a14aa026da8cb343e2b77
Cr-Commit-Position: refs/heads/master@{#302714}

[vadimgo, 2014-10-31 21:51:17]

vadimgo@chromium.org changed reviewers:
+ kmarshall@chromium.org, mfoltz@chromium.org

[Kevin M, 2014-10-31 21:59:19]

lgtm

[vadimgo, 2014-10-31 22:04:33]

Patchset #3 (id:40001) has been deleted

[vadimgo, 2014-10-31 22:07:10]

Patchset #3 (id:60001) has been deleted

[mark a. foltz, 2014-10-31 22:52:35]

Thanks for cleaning this up.  Suggested a simplification.

https://codereview.chromium.org/690383002/diff/80001/chrome/test/data/extensi...
File
chrome/test/data/extensions/api_test/cast_channel/api/test_authority_keys.js
(right):

https://codereview.chromium.org/690383002/diff/80001/chrome/test/data/extensi...
chrome/test/data/extensions/api_test/cast_channel/api/test_authority_keys.js:146:
chrome.test.assertTrue(
chrome.test.assertLastError('foo')

https://codereview.chromium.org/690383002/diff/80001/chrome/test/data/extensi...
chrome/test/data/extensions/api_test/cast_channel/api/test_authority_keys.js:155:
chrome.test.assertTrue(!chrome.extension.lastError);
chrome.test.assertNoLastError();

https://codereview.chromium.org/690383002/diff/80001/chrome/test/data/extensi...
chrome/test/data/extensions/api_test/cast_channel/api/test_authority_keys.js:157:
// Test invalid authority keys.
Can we write this as two test cases - it would make the test output easier to
understand if one should fail.

You can write 

chrome.test.runTests([test1, test2]);

to run multiple tests in the same file.

https://codereview.chromium.org/690383002/diff/80001/extensions/browser/api/c...
File extensions/browser/api/cast_channel/cast_channel_api.cc (right):

https://codereview.chromium.org/690383002/diff/80001/extensions/browser/api/c...
extensions/browser/api/cast_channel/cast_channel_api.cc:187: return
channel_error_ == cast_channel::CHANNEL_ERROR_NONE;
It looks like we only use channel_error_ to indicate whether the function was
successful or not, and don't use the value (after putting it in the ChannelInfo
result).

IMO it would be cleaner to replace it with an error_ boolean that gets set by
SetResultFromError().

Maybe this then becomes:
return error_ || !GetError().empty()

https://codereview.chromium.org/690383002/diff/80001/extensions/browser/api/c...
extensions/browser/api/cast_channel/cast_channel_api.cc:239: void
CastChannelAsyncApiFunction::SetChannelError(const std::string& error,
With the change I mentioned this may no longer be necessary.

[vadimgo, 2014-10-31 23:32:39]

https://codereview.chromium.org/690383002/diff/80001/chrome/test/data/extensi...
File
chrome/test/data/extensions/api_test/cast_channel/api/test_authority_keys.js
(right):

https://codereview.chromium.org/690383002/diff/80001/chrome/test/data/extensi...
chrome/test/data/extensions/api_test/cast_channel/api/test_authority_keys.js:146:
chrome.test.assertTrue(
On 2014/10/31 22:52:34, mark a. foltz wrote:
> chrome.test.assertLastError('foo')

Done.

https://codereview.chromium.org/690383002/diff/80001/chrome/test/data/extensi...
chrome/test/data/extensions/api_test/cast_channel/api/test_authority_keys.js:155:
chrome.test.assertTrue(!chrome.extension.lastError);
On 2014/10/31 22:52:35, mark a. foltz wrote:
> chrome.test.assertNoLastError();

Done.

https://codereview.chromium.org/690383002/diff/80001/chrome/test/data/extensi...
chrome/test/data/extensions/api_test/cast_channel/api/test_authority_keys.js:157:
// Test invalid authority keys.
On 2014/10/31 22:52:34, mark a. foltz wrote:
> Can we write this as two test cases - it would make the test output easier to
> understand if one should fail.
> 
> You can write 
> 
> chrome.test.runTests([test1, test2]);
> 
> to run multiple tests in the same file.

Done.

https://codereview.chromium.org/690383002/diff/80001/extensions/browser/api/c...
File extensions/browser/api/cast_channel/cast_channel_api.cc (right):

https://codereview.chromium.org/690383002/diff/80001/extensions/browser/api/c...
extensions/browser/api/cast_channel/cast_channel_api.cc:187: return
channel_error_ == cast_channel::CHANNEL_ERROR_NONE;
On 2014/10/31 22:52:35, mark a. foltz wrote:
> It looks like we only use channel_error_ to indicate whether the function was
> successful or not, and don't use the value (after putting it in the
ChannelInfo
> result).
> 
> IMO it would be cleaner to replace it with an error_ boolean that gets set by
> SetResultFromError().
> 
> Maybe this then becomes:
> return error_ || !GetError().empty()

Yes, it is cleaner with a bool. The logic became

  return !is_channel_error_ && GetError().empty();

https://codereview.chromium.org/690383002/diff/80001/extensions/browser/api/c...
extensions/browser/api/cast_channel/cast_channel_api.cc:239: void
CastChannelAsyncApiFunction::SetChannelError(const std::string& error,
On 2014/10/31 22:52:35, mark a. foltz wrote:
> With the change I mentioned this may no longer be necessary.

Done.

[mark a. foltz, 2014-11-01 19:29:39]

lgtm % one comment.

You might want to check the corresponding code in the extension to see if
chrome.runtime.lastError is handled in the corresponding callbacks, since now we
are setting an error for all channel errors.

https://codereview.chromium.org/690383002/diff/100001/extensions/browser/api/...
File extensions/browser/api/cast_channel/cast_channel_api.cc (right):

https://codereview.chromium.org/690383002/diff/100001/extensions/browser/api/...
extensions/browser/api/cast_channel/cast_channel_api.cc:235: is_channel_error_ =
true;
It might be a good idea to set an error message since the extensions framework
will report chrome.runtime.lastError as an unknown error otherwise.

I.e. SetError("A channel error occurred, channel_error = " +
base::IntToString(error)).

Then you might be able to eliminate the boolean entirely.

[vadimgo, 2014-11-03 21:22:45]

We should probably open a work item for the extension to ensure that
chrome.cast.channel apis handle errors in a consistent manner.

https://codereview.chromium.org/690383002/diff/100001/extensions/browser/api/...
File extensions/browser/api/cast_channel/cast_channel_api.cc (right):

https://codereview.chromium.org/690383002/diff/100001/extensions/browser/api/...
extensions/browser/api/cast_channel/cast_channel_api.cc:235: is_channel_error_ =
true;
On 2014/11/01 19:29:39, mark a. foltz wrote:
> It might be a good idea to set an error message since the extensions framework
> will report chrome.runtime.lastError as an unknown error otherwise.
> 
> I.e. SetError("A channel error occurred, channel_error = " +
> base::IntToString(error)).
> 
> Then you might be able to eliminate the boolean entirely.

Done. Also uncovered an issue with my changes in SetResultFromSocket, which was
using error_ (defined as a protected member in a base class).

[mark a. foltz, 2014-11-04 22:31:28]

lgtm

[mark a. foltz, 2014-11-04 22:31:30]

lgtm

lgtm

[vadimgo, 2014-11-04 22:37:33]

The CQ bit was checked by vadimgo@chromium.org

[commit-bot: I haz the power, 2014-11-04 22:38:33]

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

[commit-bot: I haz the power, 2014-11-04 23:56:36]

Committed patchset #5 (id:120001)

[commit-bot: I haz the power, 2014-11-04 23:57:43]

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