Move pub's web socket API over to use JSON RPC 2.0.

Move pub's web socket API over to use JSON RPC 2.0.

R=rnystrom@google.com
BUG=17492
Committed: https://code.google.com/p/dart/source/detail?r=34269

[Bob Nystrom, 2014-03-21 17:42:25]

Bunch of suggestions then LGTM.

https://codereview.chromium.org/207563002/diff/1/sdk/lib/_internal/pub/lib/sr...
File sdk/lib/_internal/pub/lib/src/barback/web_socket_api.dart (right):

https://codereview.chromium.org/207563002/diff/1/sdk/lib/_internal/pub/lib/sr...
sdk/lib/_internal/pub/lib/src/barback/web_socket_api.dart:56: /// The method
name is "urlToAssetId" and it takes a "url" parameter for the URL
Long line.

https://codereview.chromium.org/207563002/diff/1/sdk/lib/_internal/pub/lib/sr...
sdk/lib/_internal/pub/lib/src/barback/web_socket_api.dart:101: url =
Uri.parse(urlString);
Given that Uri is in Dart core, it might be nice to have json_rpc_2 support
.toUrl directly. Seems like a common use case.

https://codereview.chromium.org/207563002/diff/1/sdk/lib/_internal/pub/lib/sr...
sdk/lib/_internal/pub/lib/src/barback/web_socket_api.dart:183: var assetPath =
params["path"].asString;
<3 <3 <3

https://codereview.chromium.org/207563002/diff/1/sdk/lib/_internal/pub/test/s...
File sdk/lib/_internal/pub/test/serve/utils.dart (right):

https://codereview.chromium.org/207563002/diff/1/sdk/lib/_internal/pub/test/s...
sdk/lib/_internal/pub/test/serve/utils.dart:303: /// contain Future, in which
case this will wait until they've completed before
"Future" -> "Futures".

https://codereview.chromium.org/207563002/diff/1/sdk/lib/_internal/pub/test/s...
sdk/lib/_internal/pub/test/serve/utils.dart:306: /// The result of a successful
response is validating using [result], which may
"validating" -> "validated".

https://codereview.chromium.org/207563002/diff/1/sdk/lib/_internal/pub/test/s...
sdk/lib/_internal/pub/test/serve/utils.dart:307: /// be a matcher or a map
containing matchers and futures. This will wait until
Link to matcher and map.

https://codereview.chromium.org/207563002/diff/1/sdk/lib/_internal/pub/test/s...
sdk/lib/_internal/pub/test/serve/utils.dart:318: assert((errorCode == null &&
errorMessage == null) != (result == null));
Given the interdependencies between the disjoint parameter sets, maybe this
should be two separate methods:

expectWebSocketResponse()
expectWebSocketError()

https://codereview.chromium.org/207563002/diff/1/sdk/lib/_internal/pub/test/s...
sdk/lib/_internal/pub/test/serve/utils.dart:331: syncFuture(() => result == null
? null : awaitObject(result))
I think you can just do awaitObject(result)

If result is null, awaitObject() will conveniently wrap it in a future for you.

https://codereview.chromium.org/207563002/diff/1/sdk/lib/_internal/pub/test/s...
sdk/lib/_internal/pub/test/serve/utils.dart:348: "Result: $value");
Neat.

https://codereview.chromium.org/207563002/diff/1/sdk/lib/_internal/pub/test/s...
File sdk/lib/_internal/pub/test/serve/web_socket/path_to_urls_errors_test.dart
(right):

https://codereview.chromium.org/207563002/diff/1/sdk/lib/_internal/pub/test/s...
sdk/lib/_internal/pub/test/serve/web_socket/path_to_urls_errors_test.dart:64:
errorCode: 1,
Make a constant for this (or publicize the one in web_socket_api.dart so this
can import it).

https://codereview.chromium.org/207563002/diff/1/sdk/lib/_internal/pub/test/s...
File sdk/lib/_internal/pub/test/serve/web_socket/path_to_urls_multiple_test.dart
(right):

https://codereview.chromium.org/207563002/diff/1/sdk/lib/_internal/pub/test/s...
sdk/lib/_internal/pub/test/serve/web_socket/path_to_urls_multiple_test.dart:33:
expectWebSocketCall("pathToUrls", {"path": path.join("web", "index.html")},
Nit, but I think this would be a bit easier to read as a multi-line map.

https://codereview.chromium.org/207563002/diff/1/sdk/lib/_internal/pub/test/s...
File sdk/lib/_internal/pub/test/serve/web_socket/path_to_urls_test.dart (right):

https://codereview.chromium.org/207563002/diff/1/sdk/lib/_internal/pub/test/s...
sdk/lib/_internal/pub/test/serve/web_socket/path_to_urls_test.dart:56:
expectWebSocketCall("pathToUrls", {"path": p.join("web", "index.html")},
Just for consistency's sake how about making these all multi-line since some of
them are?

[nweiz, 2014-03-21 20:27:38]

https://codereview.chromium.org/207563002/diff/1/sdk/lib/_internal/pub/lib/sr...
File sdk/lib/_internal/pub/lib/src/barback/web_socket_api.dart (right):

https://codereview.chromium.org/207563002/diff/1/sdk/lib/_internal/pub/lib/sr...
sdk/lib/_internal/pub/lib/src/barback/web_socket_api.dart:56: /// The method
name is "urlToAssetId" and it takes a "url" parameter for the URL
On 2014/03/21 17:42:25, Bob Nystrom wrote:
> Long line.

Done.

https://codereview.chromium.org/207563002/diff/1/sdk/lib/_internal/pub/lib/sr...
sdk/lib/_internal/pub/lib/src/barback/web_socket_api.dart:101: url =
Uri.parse(urlString);
On 2014/03/21 17:42:25, Bob Nystrom wrote:
> Given that Uri is in Dart core, it might be nice to have json_rpc_2 support
> .toUrl directly. Seems like a common use case.

Will do in a follow-up CL.

https://codereview.chromium.org/207563002/diff/1/sdk/lib/_internal/pub/test/s...
File sdk/lib/_internal/pub/test/serve/utils.dart (right):

https://codereview.chromium.org/207563002/diff/1/sdk/lib/_internal/pub/test/s...
sdk/lib/_internal/pub/test/serve/utils.dart:303: /// contain Future, in which
case this will wait until they've completed before
On 2014/03/21 17:42:25, Bob Nystrom wrote:
> "Future" -> "Futures".

Done.

https://codereview.chromium.org/207563002/diff/1/sdk/lib/_internal/pub/test/s...
sdk/lib/_internal/pub/test/serve/utils.dart:306: /// The result of a successful
response is validating using [result], which may
On 2014/03/21 17:42:25, Bob Nystrom wrote:
> "validating" -> "validated".

Done.

https://codereview.chromium.org/207563002/diff/1/sdk/lib/_internal/pub/test/s...
sdk/lib/_internal/pub/test/serve/utils.dart:307: /// be a matcher or a map
containing matchers and futures. This will wait until
On 2014/03/21 17:42:25, Bob Nystrom wrote:
> Link to matcher and map.

Done.

https://codereview.chromium.org/207563002/diff/1/sdk/lib/_internal/pub/test/s...
sdk/lib/_internal/pub/test/serve/utils.dart:318: assert((errorCode == null &&
errorMessage == null) != (result == null));
On 2014/03/21 17:42:25, Bob Nystrom wrote:
> Given the interdependencies between the disjoint parameter sets, maybe this
> should be two separate methods:
> 
> expectWebSocketResponse()
> expectWebSocketError()

Done.

https://codereview.chromium.org/207563002/diff/1/sdk/lib/_internal/pub/test/s...
sdk/lib/_internal/pub/test/serve/utils.dart:331: syncFuture(() => result == null
? null : awaitObject(result))
On 2014/03/21 17:42:25, Bob Nystrom wrote:
> I think you can just do awaitObject(result)
> 
> If result is null, awaitObject() will conveniently wrap it in a future for
you.

Done.

https://codereview.chromium.org/207563002/diff/1/sdk/lib/_internal/pub/test/s...
File sdk/lib/_internal/pub/test/serve/web_socket/path_to_urls_errors_test.dart
(right):

https://codereview.chromium.org/207563002/diff/1/sdk/lib/_internal/pub/test/s...
sdk/lib/_internal/pub/test/serve/web_socket/path_to_urls_errors_test.dart:64:
errorCode: 1,
On 2014/03/21 17:42:25, Bob Nystrom wrote:
> Make a constant for this (or publicize the one in web_socket_api.dart so this
> can import it).

Done.

https://codereview.chromium.org/207563002/diff/1/sdk/lib/_internal/pub/test/s...
File sdk/lib/_internal/pub/test/serve/web_socket/path_to_urls_multiple_test.dart
(right):

https://codereview.chromium.org/207563002/diff/1/sdk/lib/_internal/pub/test/s...
sdk/lib/_internal/pub/test/serve/web_socket/path_to_urls_multiple_test.dart:33:
expectWebSocketCall("pathToUrls", {"path": path.join("web", "index.html")},
On 2014/03/21 17:42:25, Bob Nystrom wrote:
> Nit, but I think this would be a bit easier to read as a multi-line map.

Done.

https://codereview.chromium.org/207563002/diff/1/sdk/lib/_internal/pub/test/s...
File sdk/lib/_internal/pub/test/serve/web_socket/path_to_urls_test.dart (right):

https://codereview.chromium.org/207563002/diff/1/sdk/lib/_internal/pub/test/s...
sdk/lib/_internal/pub/test/serve/web_socket/path_to_urls_test.dart:56:
expectWebSocketCall("pathToUrls", {"path": p.join("web", "index.html")},
On 2014/03/21 17:42:25, Bob Nystrom wrote:
> Just for consistency's sake how about making these all multi-line since some
of
> them are?

Done.

[nweiz, 2014-03-21 20:28:05]

Committed patchset #2 manually as r34269 (presubmit successful).